[WIP] SMF Coding Guidelines

[Joshua Dickerson]

I have been working on the coding guidelines for a while. Now, it is your turn to help me out. Give me feedback. The !!! means that there are areas that need more work done.

What I want you to look for:

• Grammar and spelling mistakes
• Paragraph and structural improvements for the document
• Suggestions for the guidelines in general

The document was created using Open Office. If the doc doesn't look like the PDF and you are using another document editor, use the PDF. Also, the styling guideline is attached. If you have any suggestions for styling give 'em to me.

Thanks for your help.

[Daniel15]

Firstly, in my opinion, all URLs should be listed as footnotes on the page (like the note on tabs on page 7). So, if someone prints the page out, they know where the text links to

There may be times when certain guidelines in this document can not be adhered to. This is understandable and these are only meant to be guidelines.

Maybe you should mark things that are definitely required? I'm sure that not *everything* in there is just a guideline!

The term 'foobar' (or 'foo' & 'bar') is used throughout this document as an example. It is not an actual function, variable, or example. It is simply a placeholder. After the section on naming conventions, the examples may not adhere to the naming conventions. They are only examples.

Perhaps link to RFC3092?

Structure is then broken up in to several parts - template functions (*_above() & *_below()),

Usually, all functions in the template file are template functions. the *_above() and *_below() are layer functions

Exception: Do not use camel case when naming tables, columns, or indexes in databases.
They do not work in all database systems.

Perhaps state that SMF 1.1 uses camel case in some things, but we are changing this for SMF Development Edition.

Never use reserved words.

Since you speak about multiple database support, this should also link to reserved words for SQLite and PostgreSQL.

Each of the following keywords should be on their own line:

What about for simple queries like SELECT * FROM {$db_prefix}something?

  There are some special variables that we use such as $modSettings, $context,  $sourcedir, etc. You can find out more about those in the documentation.

What documentation? Perhaps link to it?

Multiple line comments should use /* */ and each are on their own line.

In addition to this, most of the time, the first line has a TAB directly after the /*, and each line after this starts with a tab:

Code Select Expand


/* Line 1
Line 2
Line 3
*/


It is not a function, it is a language construct. So, you don't need parenthesis around it's output.

The only it's vs. its mistake in the whole document

// Short string with variable.
echo "this $is a $short $string";

IMHO, we shouldn't show this in an example - Code like this tends to be very ugly!

Do not use <a name="anchor"> for anchors. Use <{tag} id="anchor"> since all ids are  unique they act as anchors. Where {tag} is usually a header tag.

Sentence fragment! The end bit should be reworded. Perhaps something like this:

Do not use <a name="anchor"> for anchors. Use <{tag} id="anchor"> (where {tag} is usually a header tag). This works because all ids are unique.

or similar.

Tables are for tabular data! Think of a table as a spreadsheet. Would your data work well  in a spreadsheet? Are you only using a table because it makes it easier to layout data?

Look at how SMF uses tables

All integers should be defined as integers before you start working with them

Should be:

All integers should be cast to integers before you start working with them

All variables being passed to the database must be escaped!.... It escapes all $_REQUEST variables for you.

So what do we actually need to escape, if it's done automatically?

Do not hard code the groups that you want to give access to. Use allowedTo() and
  isAllowedTo() where allowedTo() should be used for a basic check and isAllowedTo()
  should be used as a check/error combination.

Perhaps link to the tutorial on using allowedTo and isAllowedTo (it's on this board somewhere).

Before you start to work with input passed through a form from a user, you should always check their session

How do we do this?

[Smurfbutcher Bob]

Looking darned good, Groundup.

Suggestions -

"Database" -> "Queries"

Do not rely on the default value. If you are doing an insert and the table has a column that you have no value for, don't include that column.

Confusing - SQL default values, or undeclared vars within PHP?

"PHP" -> "Whitespace"

Space before and after the equal sign. $var = '';

Suggestion:
"Space before and after binary operators, etc."
Not sure of the wording, but you'll probably want a space before/after a unary (and it's var) as well, e.g. x++ et al.

-> Functions / lang const
Suggest parenthising of IIFs for clarity. Inspired by your example from variables:
$something = $var == 'foo' ? true : false;  // bad
$something = ($var == 'foo' ? true : false);  // good
echo $something, $somethingelse, ($var == 'foo' ? 'Yes' : 'No'), 'Hello World', $yetanotherthing; // good
Also note that ? and : are effectively binary in the grammar, so they get spaces.

Output -> HTML

Use <p> for paragraphs. Use <br /> only for a single line break. Two or more define a paragraph so use that.

Suggest clarification:
Two or more <br /> tags constitutes a paragraph, so use a <p></p> block instead.

Related fields should be grouped with a <fieldset> tag. All fieldsets should have a legend

Needs closing period.

Security -> Wash

All variables being passed to the database must be escaped <snip> In QueryString.php,
there is a function called cleanRequest(). <snip> when you are working with $_REQUEST variables, you need to unescape them with stripslashes().

Clarify a little - Joe Modder will interpret this to mean he should stripslash prior to usage in a WHERE clause or INSERT,  while Jack Modder will interpret this as stripslashing the results of a query, prior to display.

Also, perhaps discuss where bbcode_parse / preparse fits in as far as clean/strip, SQL, and risk. (I've almost finished a replacement for the defunct "AjaxChat" mod, and I'm considering releasing it... so I'm curious to see how I did on this issue  )

All in all, this doc rocks. Well done!

[Joshua Dickerson]

I've looked over the posts and there are some things that I need to change. Without using a lot of BBC I am going to respond to a couple of things. Then I am probably going to make a few changes...

• Just about everything is a guideline.
  
• I didn't know RFC 3092 existed, might add a link.
  
• A template is based on layers. All layers would then be considered template function. Those two functions are only examples because there are other template functions that exist or may exist in a theme.
  
• This coding guideline is how things should be coded from now on.
  
• I had a list of the reserved words. I need to update that. I think I posted them on the team boards.
  
• IMO, simple queries should be like a complex query for consistency.
  
• Some of the documentation isn't available yet
  
• Yeah, makes sense
  
• Probably one of those days I didn't sleep
  
• You think echo 'a ', $b, ' c ', $d, ' e '; looks better than echo "a $b c $d e ";?
  
• Not sure about the grammar of that one.
  
• eh, the default theme doesn't use tables properly so don't look to SMF for that.
  
• woops, cast.
  
• Yeah, I guess I should clarify that. Just for clarification right now - you will most likely need to stripslashes() from $_REQUEST vars. You need to re-escape them after that.
  
• For the last two comments, I will add a little to it.
  

smurfbutcher:

• Okay, for the database default value.
  
• I will take care of some of those things tomorrow. Headed to bed
  

[Daniel15]

You think echo 'a ', $b, ' c ', $d, ' e '; looks better than echo "a $b c $d e ";?

Definately! I have never seen SMF itself use things like "a $b c $d e "; ([Unknown] hated it ), and I've never used it myself either.
Is it just me that doesn't like it?

[Peter Duggan]

Must just qualify the following comments by saying that, while I've read groundup's original draft and have every intention of going through the one attached here, I've not yet had time to do this properly. But there are a couple of comments in this topic that leaped out from a quick skim, so I'll just add my tuppence worth to these in the meantime:

Do not use <a name="anchor"> for anchors. Use <{tag} id="anchor"> since all ids are  unique they act as anchors. Where {tag} is usually a header tag.

Sentence fragment! The end bit should be reworded. Perhaps something like this:

Do not use <a name="anchor"> for anchors. Use <{tag} id="anchor"> (where {tag} is usually a header tag). This works because all ids are unique.

or similar.

Despite what they might teach you in English, incomplete sentences are not necessarily bad (IMHO it depends largely on context). So keep what you had or go with Daniel's version, but sort that double space before 'unique' if sticking with the original!

Tables are for tabular data! Think of a table as a spreadsheet. Would your data work well  in a spreadsheet? Are you only using a table because it makes it easier to layout data?

Look at how SMF uses tables

And look at how SMF is hopefully going to use (or not use) tables in future? I like the spreadsheet analogy and think it makes sense with future-proofing in mind.

Output -> HTML

Use <p> for paragraphs. Use <br /> only for a single line break. Two or more define a paragraph so use that.

Suggest clarification:
Two or more <br /> tags constitutes a paragraph, so use a <p></p> block instead.

I'm still bothered by this (despite SMF's 'paragraphs' being styled by chance or design with the 'conventional' double line break) and feel that it's bound to contribute to that myth unless qualified by a statement about the semantic (as opposed to presentational) meaning of <p> tags.

[Dannii]

Despite what they might teach you in English, incomplete sentences are not necessarily bad (IMHO it depends largely on context).

Ellipsis is essential in spoken speech and can be used in written language too, but I'd agree to change those lines.

I'm still bothered by this (despite SMF's 'paragraphs' being styled by chance or design with the 'conventional' double line break) and feel that it's bound to contribute to that myth unless qualified by a statement about the semantic (as opposed to presentational) meaning of <p> tags.

I'm not so sure that two line breaks defines a paragraph...

[Joshua Dickerson]

Peter, if there is a block that you want to have with extra whitespace above or below it, you should use padding on the tag.

One line break:
foo
bar

Two line breaks:
foo

bar
(constitutes a new paragraph)

Three or more line breaks:
foo


bar
(presentational - no semantic value) So you use <p> and add extra padding.

[Peter Duggan]

I'm still bothered by this (despite SMF's 'paragraphs' being styled by chance or design with the 'conventional' double line break) and feel that it's bound to contribute to that myth unless qualified by a statement about the semantic (as opposed to presentational) meaning of <p> tags.

I'm not so sure that two line breaks defines a paragraph...

That's why I called that assumption a myth (nurtured by that 'conventional' double line break being the default behaviour for unstyled paragraphs in most browsers), although I realise that my call for a statement about semantic vs presentational meaning could be misunderstood when <p> tags technically have no presentational value (which was actually my whole point)!

[Dannii]

A line break has a semantic meaning in some contexts, and so would two line breaks. I'm not sure two line breaks semantically equals a paragraph (forgetting about presentation completely!)

A question: if someone, like myself, was writing a mod that included code from a library, would we have to go through and change the formatting etc of the library? If we're not altering the library, it would seem to me that changing it for the sake of these guidelines when we could just leave it identical to the original library is a bad idea. If we left the library alone, then it will be easier for everyone to consult docs about the library etc.

[Daniel15]

would we have to go through and change the formatting etc of the library?

I did this with the Akismet mod, but I'd recommend against it. It's usually good to keep any external libraries with their original formatting (in the next version of the Akismet mod, as I'll be using a different Akismet library, I won't be changing its formatting)

[Joshua Dickerson]

In what terms do you mean by including a library? I think it all depends on the mod itself. Like I said, these are only guidelines - they aren't concrete. Although, they should be followed to the best of your ability, but whitespace, certain functions, and indentation should follow the SMF way as best as possible. Just to keep things normal. If you are going to be changing a lot of code, other than basic SMF functions, I think you might want to stick with your base code from the other software.

[Dannii]

It's a single file, so I'm planning on just including it as a Subs-mymod.php file. I won't be changing any of the library at all, but it has bad indents and brackets.

[JRSofty]

You think echo 'a ', $b, ' c ', $d, ' e '; looks better than echo "a $b c $d e ";?

Definately! I have never seen SMF itself use things like "a $b c $d e "; ([Unknown] hated it ), and I've never used it myself either.
Is it just me that doesn't like it?

Actually I despise the echo 'a', $b, ' c ', $d. ' e '; approach. It is too easy to get lost. If you put everything nicely inside a set of double-quotes then it is much easier to know what is going to be output and you don't end up forgetting that one little comma (that I'm sure everyone does) that screws up the template.

I'm glad to know that it is ok to use echo "this $is my $variable"; in SMF cause I'm going to be doing that now in the SMFBot mod. I'm getting really frustrated creating the administrator templates using [Unknown]'s comma method

[Rudolf]

For short strings it's ok to use the "", but for longer strings it gets heavy. PHP evaluates the double quoted strings looking for variables inside, while the simple quoted strings (') are simply sent to the output. The same goes with ,(comma) and .(dot). Whenever you concatenate strings with the dot the string as a whole is evaluated first and the resulting value used, while with comma there's no processing going on. The string is simply "streamed" to the output.

In the templates where a lot of strings are quite long and you need only a few variables inside, it's way better to use commas, extracting the variables from the strings. You avoid a lot of extra work from the parser.

PS; if you use good spacing and indenting both ways are equally easy to read. So it's only a performance issue.

[Dannii]

Personally I think %s type place holders are best

[Joshua Dickerson]

I think links should be underlined when in the body as a rule. When in navigation, you don't need to underline links. You should always use a different decoration to signify a link from headings or content. No matter where it is at. Thoughts?

[Dannii]

I don't know if they should be underlined... underlines are kinda ugly. But some sort of differentiation is essential.

[Daniel15]

I don't know if they should be underlined... underlines are kinda ugly.

IMHO, links should always be underlined on mouseover

[Joshua Dickerson]

Arguments:
http://www.uie.com/brainsparks/2006/07/05/do-links-need-underlines/
http://www.useit.com/alertbox/20040510.html
http://www.cre8asiteforums.com/forums/index.php?s=8caafce8c598748a2ef4a23ec1ae5a20&showtopic=36893&st=0&p=181570&#entry181570

There are some ideas that I have read that humans are used to adapting and changing, but the software should make them do as little as possible from the norm.

Most people expect search to be somewhere near the top (and right) from some articles that I have read. There is an article that I have read that has said that they don't need to be and shows examples. If you look around, you will see those examples. I naturally move my mouse to the top right when I am looking for search. That is while still reading for content. Or, in the cases of search engines, I just start typing. The main focus of the page is to search so it is only natural to expect the cursor to be in the search box.

When I am looking for a sitemap, about, terms of service, and privacy policy I always go to the very last thing on the page. I think my movements are standard to what I have seen other users do. I think less experienced users will go to the menu first for those things.

The thing is, people expect things to be in certain places, look and act a certain way. If we keep things standard (for the most part), people should have less trouble accessing things on a page.

Some of this should go in my usability topic (if not all). But that topic is only a feeder to this topic.
</rant>