Improved code indenting, style, comments, etc.

[spaceman]

I've been up to my neck in osCommerce code over the last 3 months, and my general impression is that the code I see:

 

1. Could be more consistently indented (preferrably with tabs, not spaces)

2. Would benefit from much better explanatory commenting of functions, code sections, etc.

3. In conjunction with #2, the code could have more spaces between major functional areas in a a script. Let the code breathe!

 

Look, the code is really clever and everything and I'm incredibly grateful to be working with it, but IMHO it's much harder than it should be for an 'average' or better php/mysql developer like myself to tweak and develop simply because it's poorly self-documented within scripts.

 

Maybe this comment is unjustified because v2.2 isn't yet at the stable/production stage. Perhaps this tidying up of code is something that's done around the beta stage? I honestly don't know.

[Mark Evans]

1. Could be more consistently indented (preferrably with tabs, not spaces)

 

It was decided internally to use spaces rather than tabs see the STANDARD file in the root catalog folder for the coding standards we use :)

 

2. Would benefit from much better explanatory commenting of functions, code sections, etc.

 

Better documentation will come, we are jusy busy working on 2.2 at the min :)

 

Feel free to document the functions / classes yourself and then send them over to us ;)

 

3. In conjunction with #2, the code could have more spaces between major functional areas in a a script. Let the code breathe!

 

Which parts?

[spaceman]

1. Fair enough.

 

2. OK, but I believe it's best done as an ongoing process by the core developers, since they are 'at the coal face' when it comes to knowing exactly what a script is doing and how the major elements are structured. But I know how programmers hate documenting (speaking from first hand experience)!

 

3. Where do you want me to start? :wink: Let's take a quick look at /catalog/default.php.

i. There's nothing at the top of the script to give a broad overview of what's going on in the script

ii. Why is the first bit of php scripting indented 2 spaces already? (minor point)

iii. Looking at the first code section:

 

// the following cPath references come from application_top.php

 $category_depth = 'top';

 if ($cPath) {

   $categories_products_query = tep_db_query("select count(*) as total from " . TABLE_PRODUCTS_TO_CATEGORIES . " where categories_id = '" . $current_category_id . "'");

   $cateqories_products = tep_db_fetch_array($categories_products_query);

   if ($cateqories_products['total'] > 0) {

     $category_depth = 'products'; // display products

   } else {

     $category_parent_query = tep_db_query("select count(*) as total from " . TABLE_CATEGORIES . " where parent_id = '" . $current_category_id . "'");

     $category_parent = tep_db_fetch_array($category_parent_query);

     if ($category_parent['total'] > 0) {

       $category_depth = 'nested'; // navigate through the categories

     } else {

       $category_depth = 'products'; // category has no products, but display the 'no products' message

     }

   }

 }

 

"]// the following cPath references come from application_top.php". So? What does this mean? OK, so I know cpath is to do with categories and so we're probably working out here if we're navigating into a nested category or not, but my point is that developers shouldn't have to read and understand the code before they can get a high-level grasp of what a section is there to achieve. My observation on the coding style (as dictated by the 'standard' documentation) throughout osCommerce is that it's almost always really difficult to know what a block of code is doing without first studying and understanding the code itself in detail. Most 'comments' in default.php consist of a few words (eg. "// optional Product List Filter") with php scripting stacked all around it. In fact, not a single comment in default.php runs over 1 line.

 

How about clearly delineated comments to indicate major structural sections of the code?

 

I hope you find these comments to be constructive. One of the strengths and joys of osCommerce is that myself and others like me are allowed to take a look under the bonnet/hood, and naturally what does it for me might not do it for another with a different level of experience and/or ability. So it's a tough call I know.

 

In summary, I would say that

i. It's almost impossible to have too many comments in a script - but very easy to have too little.

ii. Lots of comments have the tiniest of impacts on script size/efficiency, but can have a large impact on making it easier to maintain and extend.

iii. A few extra blank lines in a script to separate and distinguish major code sections from minor ones can have a significant impact on readability - even without comments.

[jimporter]

Couldn't agree more with the above posts.

 

Having been a commercial programmer in the past but not having done any serious programming for a number of years while I concentrated on management I amazed how all programmers from many disciplines do not coment their code.

 

It is OK for programmers who are in the same code everyday not to comment their code as they will be intimate with the functionality. However it does not matter whether you are a beginner or experienced when you have not been into a piece of code for many months a high level of comments is a must.

 

I would think that in an open source environment where there are many people coming fresh to the code all the time it would be even more important. I certainly would find this useful as overall I find the level of documentation poor (only to be expected from programmers as they like writing code not writing about it).

 

The only problem with documentation as a whole and inline comments/documents in particular is that they must be updated, which means extra work, which means it probably won't get done and incorrect comments are as much use as none at all.

 

Jim

[zzfritz]

Mark failed to mention above that there are some published coding standards among the team, which are found here:

 

http://cvs.sourceforge.net/cgi-bin/viewcvs...atalog/STANDARD

 

As he says, they are not complete, but they should be reviewed by anyone seriously trying to understand or contribute to the code base. I have commented elsewhere on that subject, with Harald's subsequent approval:

 

http://www.oscommerce.com/forums/viewtopic.php...standards#98512

[jimporter]

Interesting and informative as the coding standards are I think that they further justify the points made previously regarding the level of comments i.e. there is no mention at all.

 

Jim

[Harald Ponce de Leon]

Interesting and informative as the coding standards are I think that they further justify the points made previously regarding the level of comments i.e. there is no mention at all.

 

The comments section was removed from the standards file on the last commit as we are internally discussing what standard to use for the comment style.

 

What has been in use is phpxref, though will probably be replaced with phpdocs, phpedit docs, or some other format.

 

What we're trying to avoid is bloating the codebase due to the comments - an alternative is writing (most of) the comments externally to the codebase (say, in a database).

 

This internal discussion has not been top priority though, so it may take another milestone release before something is done.

[jimporter]

Harald,

 

This is symptomatic of very small teams of techies (been there done that got the t-shirt) who understand there small codebase very well i.e. they always find a reason not to do documentation.

 

Inline commenting should for the reasons given previously be included with no excuses as it benefits all, and it should not take a team of people to decide how to do it. The longer it is left the less likely it is that it will happen.

 

I understand the pressures for just writing lines of code as I have seen the complainers on other threads but this discipline should be treated as part of the rigour of coding.

 

For an interesting discussion on the topic see http://freshmeat.net/articles/view/238/

 

Regards

Jim

 

PS Reformed smokers are always the worst when it comes to hating smoking. Same goes for Techies turned Managers!!!

[zzfritz]

Geez, this discussion is giving me flashbacks to the 1970s when we truly believed we were getting a handle on modularity, transportability and distributed development teams. At that time, I was being propelled into software development management because these issues were not understood by staff who lacked experience with the emerging higher level languages. The tug of war between development and documentation is, apparently, a fact of life that transcends the moment.

 

Thirty years later, the machines run faster and the tools are more clever but the development issues remain the same.

[Waiting]

Comment it and be done. Why all the excuses?

 

Everybody uses inline commenting for their code. Why do you guys have to be different?

 

I agree, that it most likely will never be done. If it hasnt begun already, why would they go thru and put it in all the files?

[burt]

Hardcore coders do it without comment :wink:

[mattice]

If it hasnt begun already, why would they go thru and put it in all the files?

Please feel free to download the latest CVS, comment the code and send it back to us.

We appreciate your continuous efforts to improve this project!

Thank you,

 

Mattice

[mattice]

Hardcore coders do it without comment :wink:

 

Not to mention they use their real name... ;)