Forum OpenACS Development: Tipping of coding practices ?

Collapse
Posted by Malte Sussdorff on
Shall we TIP recommendations for good coding practises. And if they are approved, shall we make it mandatory to follow them for all packages in the main trunk. And if we make it mandatory, shall we offer a grace period of e.g. 2 releases before a package *has* to be cleaned up?
Collapse
Posted by Dirk Gomez on

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.

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.

Collapse
Posted by Andrew Piskorski on
Tom, in that case fine, but your own special definition of "documentation" is different from any normal usage of the word I've heard before, which confused me. I guess what you were getting at is that Dirk's original bullet item, "document code", is confusing and far, far too vague. Sorry, I misunderstood.

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

Collapse
Posted by Tom Jackson on

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.

Collapse
Posted by Malte Sussdorff on
Though code *can* be documentation, OpenACS is a very good example for code *should not* be documentation. We have a lot of ancient code within the system, that does not adhere to any standards set forth. Within the community we defined some standards by just adopting them, though some people still forget to do so, which is okay. My original question was only driving in the direction of getting a document of coding practises out (sad to hear Andrew has not time to do it, as he has a lot of ideas on that topic). And once we have these practises, we can encourage / enforce all *new* code to follow these practises.

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.

Collapse
Posted by Joel Aufrecht on
Jade's Things to remember when developing on OpenACS seem like a good place to start for coding standards.
Collapse
Posted by Jade Rubick on
Just for clarification, I did not create that list. It was assembled by Jun or someone else. Many people contributed to it. I just have it linked in from rubick.com

I think there's a thread a way back that describes putting that together.

Collapse
Posted by Andrew Piskorski on
Btw, even if I had the time to devote to drafting a coding standards doc (which I don't), I think I've personally been too out of touch with much of the OpenACS codebase for too long to really do a good job. I think real life examples drawn from working with and fixing the code are key here.

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

Collapse
Posted by bill kellerman on
looking at archives, arsdigita seemed to be a good and useful balance of explaining what problem is being solved and why it is being solved that way -- from any particular viewpoint in the system.  why?  probably because they had someone to enforce the rules and salaries depending on it.

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.

Collapse
Posted by Jeff Davis on
Here is a style resource page I pulled together.

Also, here is a draft of the beginnings of a style guide chapter with the list being the basic ground I plan to cover (and with plenty of explicit examples).

Collapse
Posted by Jun Yamog on
Hello,

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?