Forum OpenACS Development: Tipping of coding practices ?
I don't think it needs to be tipped. Who would disagree on using good coding practices? Noone.
What we need is a project and a project leader/supervisor for such a code cleanup adventure. Here's a list of to-dos:
- Move pages towards using ad_form or formbuilder. This is more secure because they generate hashes that make sure that your POST request came from your webserver. It reduces code, makes code more readable etc.
- Move pages towards using listbuilder. Way less code, more UI consistency, more code consistency
- Audit code for unused procs and code snippets and remove/depecrate them.
- Document code.
- Move apart table/index creation and package creation, so packages can be recompiled easily.
I have not enough time to coordinate this, so if someone else steps up I would gladly do a bit of worker-bee work here and there.
I'll sink my time into a performance and scalability project for which I am writing up some text currently. Stay tuned on this one.
I take it you missed the tcl style thread a month or two ago. :)
Hell, that wasn't even general style, that was just about whether we should follow the same proc parameter conventions as the rest of the tcl world...
And my concern stands: even if we had a long discussion on best practices and this and that, we most urgently need someone to lead this project.
I am rather sure that my proposals wouldn't be challenged.
Probably as long as you keep them to yourself they won't be. Another way to avoid that result is to keep the discussion so general no one knows for sure what the proposal is. Or just make changes without asking first.
I assume your coding practices are contained in the bullet list. As a comparison. The tcl coding practices are many pages in length and contain actual examples of good and bad style.
Mostly the bullets are way to general to be useful. But lets start by combining a few of your bullets. Are the formbuilder procs and the usage of the wrapper ad_form documented enough to require their usage?
Also, my long standing complaint about ad_form is that it encourages mixing html with the tcl pages, making it difficult to maintain code once it goes into a client project. I know folks are going to shout me down one more time on this, but when you put html attributes into the tcl page, that 'is' html code. Just because the '<' and '>' are not on the page doesn't mean you have removed html coding, thus presentation, from the tcl page. So one thing missing in 'use ad_form' is 'how to use ad_form', which I believe can be used in the way I am suggesting, but seldom is used that way.
Overall though, I don't understand the requirement to use ad_form and listbuilder. These are big complex tools that make the OpenACS toolkit more difficult to understand for new users. In contrast the db_* procedures simplify life. You don't need a rule to get folks to use them because you can understand them, most of the time, pretty quickly.
Code review: who can complain about that? Question: how do you determine that an api is 'unused'?
Your last idea of moving table/index apart from pl sounds good initially, but really in a lot of cases it isn't necessary. Indexes do have to be recreated if you make datamodel changes, maybe they should be separate from tables? Personally I haven't run into any problems in this area.
And what exactly is specific about 'document code'? I thought code was documentation. Documentation isn't needed until something doesn't work like you expect. Then you really need it. Chances are, the person who wrote the code understands the limitations and never ran into the problem, so it isn't documented anyway. Bottom line is that what the code writer thinks of as documentation might not be enough for the casual user. Better than just documenting code is to write it so it is easy to understand. We have a toolkit filled with procs that go on page after page. If this code was broken down into more understandable chunks it might be easier to figure on the occasion it doesn't work the way you think it should. You could also try using meaningful variable names. Also, not overloading a variable with several usages in a proc helps a lot in figuring out what a proc does.
I know I have a bunch of dumb ideas and views. I assume folks will question them. That is a good thing. Sorry to digress.
The points I sketched up are the guidelines I follow when working with OpenACS.
Thanks for your criticism, I hope someone who is willing to lead such a project will pick it up.
Dirk's point is a good one though, unless there are some people willing to do something besides just post on the forums about this it's unlikely to matter one way or the other. I will try to pull a draft together based on the various threads we have had in the past but I will be sorely disappointed if those of you who seem to care about this stuff are not willing to wade in and fix it.
The things can think of off the top of my head are datamodel create scripts, using namespaces, seperation of markup and code (and especially removing tcl from .adp files), package structure (which mostly needs to be updated, to be in line with tip 12), the ad_proc, ad_page_contract, ad_library javadoc tags, and declaring things public or private.
Also, I think we need a TIP for how we are going to remove deprecated functions (a la set_form_variables_string_trim_DoubleApos and util_PrettySexManWoman), since I would really like to clean out some of the really crufty stuff. I think remove anything two releases after it is -warn (i.e. remove the 4.6.3 -warn things in 5.1 and the 5.0 -warn things in 5.2) but that is probably something we need to talk about more and make a formal decision on.
Tom, If you seriously "thought code was documentation" (rather than just engaging in annoying hyperbole), then you are seriously confused. Code, at best, can only tell you what is being done, never why. Explaining the why is, among other things, what comments and documentation are for.
RE specific draft guidelines. I certainly appreciate these guidelines. I have since "refactored" my own code to implement your proposed standard layout.
I don't disagree that pl is easier to maintain in separate files from table create. My point is that it is much more likely that an index would need to be recompiled than for pl. You need three files, not two.
Andrew, documentation is anything that helps the user understand what a program does, or helps a developer fix a bug or add features. For compiled binary files, code isn't much documentation. However source files, including the code, are documentation. I don't think I am seriously confused on this matter, although I wouldn't mind claiming the moniker 'Annoyingly Hyperbolic'.
OpenACS is blessed with a helpful user community willing to explain less than obvious features of the toolkit. This helps make up for the lack of other types of documentation.
Instead of "document code" I would draft a guideline saying something like:
"Write clear, well commented code. Your comments in the code should explain WHY the code does something, wherever the why is not immediately obvious. If HOW the code does its work is especially tricky or confusing, then you should explain the how in a comment as well."
Jeff, yes, I take your and Dirk's point, but since I'm not going to volunteer (not anytime soon anyway) to write the coding standards doc and refactor lots of code, I might as well post to the forums. :)
Andrew, I really fail to understand why you seem to think that code is _not_ documentation. Since that is the only conclusion I can draw to your objection of my usage. Most of the code I have written I use. Example code which uses code is very good documentation of how, when and where to use an api.
Sometimes I think I fell into a worm hole here. If you look back at my original statements, you will see that a continue talking about documentation that clearly isn't code, so I understand that code is not the only documentation, just one form. If you overlook writing good code, by following a consistent style, it really doesn't matter how much external documentation you write about what the code does. As soon as something goes wrong, no one is going to want to fix it. There are many OpenACS packages which are documented, but are such a mess they are next to impossible to fix.
On the other hand, writing a good document on what you intend to do, what features you think a piece of software should have, what data model you want to use and why, will really save your butt if you have to put a project aside for any length of time to work on something else. I'm finally starting to get to a point where the excitement of a new project gets put into brainstroming, and not into furious writing of code to get something out the door.
Dirks point of cleaning up the code base is a very good one. I assume we might be able to do so during one of the OACS festivals with a lot of "worker bees" helping out. After all this is a good way for fresh developers to take a look at the code *and* learn the coding principles set forth.
My fear is that the principles will never be written down, unless I start to write this document. My hope is that Jade and Joel could devide this task between themselves. My goal is to not let this idea die and get documented coding principles out. Someday.
I think there's a thread a way back that describes putting that together.
Perhaps it would be useful for developers to post their specific lessons learned (and guidance they drew from them) into a forums thread, to serve as raw material. On the other hand, maybe that would just turn into a giant Developer time suck yet go nowhere. :)
i still think getting a bunch of people to agree on a required best solution is a sinkhole. there needs to be some kind of motivation to define and follow the standard. it needs to be loose enough to fluctuate based on the ideas of the community as a whole.
1. define the group that will care about this stuff.
2. get everyone to agree on a set of recommended standards for docs, code, and processes. it doesn't have to be perfect, just something everyone can agree on at that time. you can follow these, but you don't have to.
3. if you choose not to follow the standards, you are an evil "unsupported outsider" unless you can build a case to the standards group for why your method should replace or be incorporated into the current standards.
4. those that do follow the standards get community priority in support, placement, recomendations, and distribution. this is heroic.
5. the group requires periodic checking of existing code to make sure it fits current standards. if not, you get bumped to evil-unsupported-outsider status.
6. make the whole program very visible to the community.
I have given the Things to Remember to one of the guys that has doc privs. I forgot, whom and when. Anyway I would be glad to remaintain it as one of the pages here. I also need to revise some of it.
Would it be worth be putting here? If yes, can someone grant privs? And where would we put it?