The Code Cleanup Project

News and discussion about beta versions and CVS changes
User avatar
gRegor
Posts: 738
Joined: Tue May 14, 2002 3:17 am
Location: Bellingham, WA
Contact:

Postby gRegor » Tue Nov 23, 2010 6:48 pm

After many years, I guess I'm back at it. At least for a little bit. :)

I was committing a change I had discussed on the mailing list in COMMENT.php and since it's a short file I went ahead and made a subsequent commit cleaning it up.

I added backtick characters around the field names in the query `like this`, capitalized the SQL keywords, then added braces for all if conditions and made their format uniform.
— gRegor
Wild Soundscape
Posts: 127
Joined: Tue Aug 24, 2010 7:09 pm

Postby Wild Soundscape » Wed Nov 24, 2010 2:23 pm

Could I suggest a major improvement would be proper layout of nested braces?

Code: Select all

if (x)
  {
  something=1;
  }


rather than

Code: Select all

if (x) {
something=1;
}


The latter, although I know originated by K&R around 1979, is really difficult to follow, particularly when deeply nested.
ftruscot
Nucleus Guru
Nucleus Guru
Posts: 7430
Joined: Wed Feb 22, 2006 6:19 pm
Location: Massachusetts
Contact:

Postby ftruscot » Wed Nov 24, 2010 4:13 pm

It would be good to be consistent on braces. Everyone seems to have their own preference. It doesn't matter to me much since my editor helps identify brace pairs with red highlighting and dotted connecting lines.
Is your question not solved yet?
Search our FAQ,
read the Documentation, or
browse the list of available plugins.

Check out my plugins
User avatar
gRegor
Posts: 738
Joined: Tue May 14, 2002 3:17 am
Location: Bellingham, WA
Contact:

Postby gRegor » Wed Nov 24, 2010 6:54 pm

Heh, yeah that can be its own holy war. :)

My preference is the latter method, which is what I've been using when cleaning up.

I've seen this in a fair bit of Nucleus code:

Code: Select all

if (x)
{
   something=1;
}

// note: braces at same indentation as 'if'


and I guess I don't mind doing it that way if that's the consensus. Indenting the braces further than the 'if' doesn't make sense to me, though.

Personally, I've not found my method hard to follow. I usually have a blank line between anything nested, and of course the indentation.

Example:

Code: Select all

if (x) {

   something = 1;

   if (y) {
      something_else = 1;
   }

}
— gRegor
User avatar
gRegor
Posts: 738
Joined: Tue May 14, 2002 3:17 am
Location: Bellingham, WA
Contact:

Postby gRegor » Wed Nov 24, 2010 6:58 pm

Probably more important is making sure all control structures are using braces. There's a lot of simple if / else structures that don't have braces at all. :)
— gRegor
ftruscot
Nucleus Guru
Nucleus Guru
Posts: 7430
Joined: Wed Feb 22, 2006 6:19 pm
Location: Massachusetts
Contact:

Postby ftruscot » Wed Nov 24, 2010 7:01 pm

Yes. That is pretty random, too. I'm guilty of not using them sometimes if the if only executes one statement, but other times I use the braces in the same situation. It would be good to have some conventions written out. Maybe they exist and I've missed them...
Is your question not solved yet?

Search our FAQ,

read the Documentation, or

browse the list of available plugins.



Check out my plugins
User avatar
gRegor
Posts: 738
Joined: Tue May 14, 2002 3:17 am
Location: Bellingham, WA
Contact:

Postby gRegor » Wed Nov 24, 2010 8:00 pm

I thought there were some for plugins - I referenced that in the first post in this thread. That's been years ago. :) I don't remember.

Perhaps PHPBB's guidelines would be a good place to start / copy:

http://area51.phpbb.com/docs/coding-gui ... .html#code
— gRegor
ftruscot
Nucleus Guru
Nucleus Guru
Posts: 7430
Joined: Wed Feb 22, 2006 6:19 pm
Location: Massachusetts
Contact:

Postby ftruscot » Wed Dec 01, 2010 7:08 pm

I found this list of guidelines for core coding in the wiki: http://wakka.xiffy.nl/contributing:codingguidelines

We should probably do any cleaning up to match to these guidelines. It's funny how much they differ from the phpbb3 guidelines in certain aspects. The phpbb3 guidelines also touch on more things than the nucleus ones.
Is your question not solved yet?

Search our FAQ,

read the Documentation, or

browse the list of available plugins.



Check out my plugins
User avatar
gRegor
Posts: 738
Joined: Tue May 14, 2002 3:17 am
Location: Bellingham, WA
Contact:

Postby gRegor » Wed Dec 01, 2010 10:19 pm

I'm going to be bold (Wikipedia language) and suggest updating those guidelines to be more in line with phpBB's guidelines. I'll work on this on a new wiki page, but summary:

- variable names all lowercase, separate words by underscore (I really don't like lowerCamelCase for variables, easily confused with functions)

- lowerCamelCase for functions / methods

- braces on their own line, in line with the control structure (I concede readability and easier brace-matching)

- single space between tokens

- single quote strings unless variable interpolation is necessary

- other things as I think of them / review the phpBB guidelines
— gRegor
ftruscot
Nucleus Guru
Nucleus Guru
Posts: 7430
Joined: Wed Feb 22, 2006 6:19 pm
Location: Massachusetts
Contact:

Postby ftruscot » Wed Dec 01, 2010 10:41 pm

I can go with whatever. I agree that consistency is important.

It will be hard to change function names and even variable names is some cases (like global variables where they are used in plugins and other external code). But it's worth trying.

I think we can continue on this and agree on some standards, but I suggest we don't try to clean up the code until after the release of 3.60 in the next few weeks. One thing to consider going forward is incorporating some PHP5-specific standards, particularly in classes, since we no longer officially support php4.
Is your question not solved yet?

Search our FAQ,

read the Documentation, or

browse the list of available plugins.



Check out my plugins
User avatar
gRegor
Posts: 738
Joined: Tue May 14, 2002 3:17 am
Location: Bellingham, WA
Contact:

Postby gRegor » Wed Dec 01, 2010 10:46 pm

Sounds good, Frank.
— gRegor

Return to “Core Development”