Forum OpenACS Development: Re: Tipping of coding practices ?

Collapse
Posted by Jonathan Ellis on
Who would disagree on using good coding practices?

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...

Collapse
Posted by Dirk Gomez on
I guess I did. I am rather sure that my proposals wouldn't be challenged.

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.

Collapse
Posted by Tom Jackson on
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.

Collapse
Posted by Dirk Gomez on
We are deviating...but didn't I make detailed enough proposals just two postings up in this thread? I specifically refered to those ones. How detailed do you want them to be? Can you please give an example.
Collapse
Posted by Tom Jackson on

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.

Collapse
Posted by Dirk Gomez on
Maybe my wording wasn't good enough, but I didn't intend to speak about coding practices but about "who is going to get this done?". My criticism was/is geared towards a fact that we don't need a TIP if there is firstly noone to lead the work and secondly noone to do the work.

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.

Collapse
Posted by Jeff Davis on
I think we need to develop a "coding standards" document as the first step of a code cleanup adventure (and to the extent that there are controversial issues there might need to be some TIPs; I would define controversial as something someone cares enough about to write a TIP on). It's definitely the case that many of the bad examples in the toolkit are copied into new packages on a regular basis and cleaning things up would make the toolkit both more robust and more pleasant to work with.

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.

Collapse
Posted by Andrew Piskorski on
Tom, PL/SQL (or PL/pgSQL) code is always better off in separate files from "create table" statements. There is never any drawback to organizing things that way, and if you ever need to write upgrade scripts for the PL/SQL code, you will soon find just how much easier and cleaner it is to have the PL/SQL in its own files. Back in July, Malte at least seemed to agree with me, on that, and at the time I also made some very specific draft guidelines (which were mostly ignored).

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.

Collapse
Posted by Randy O'Meara on
Andrew,

RE specific draft guidelines. I certainly appreciate these guidelines. I have since "refactored" my own code to implement your proposed standard layout.

Thank you,

Randy

Collapse
Posted by Tom Jackson on

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.