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

[KGIII]

Except, well, the links you posted aren't underlined until mouseover... I agree that they MUST be easily spotted.

[Joshua Dickerson]

Maybe I should change that to "decoration". So long as they are clearly defined. For instance if you were to click the following link or if you were to click this link

[KGIII]

I'd go with something like "clear decoration" or "easily understood navigational links" or something perhaps. Just my two cents.

[B Patterson]

Files

When you are displaying an index of information to be displayed to a user such as BoardIndex.php and MessageIndex.php, the file should end with 'Index'.

Probably needs to be reworded to something like:

When an index of information is to be displayed to a user, such as BoardIndex.php and MessageIndex.php, the file should end with 'Index'.

Queries

   You may not use UNION, SET PASSWORD, BENCHMARK, subselects, or comments in a query. As usual, there is an exception that you should almost never do as it allows for security vulnerabilities:
$modSettings['disableQueryCheck'] = 1;

What's the exception?  I should never what... enable the "disableQueryCheck" setting?  Why? (I don't mean, tell me, but explain it, make a sub-point about the security vulnerability so users can read it.)

   When selecting columns, only select those which you are going to use. Do not use SELECT *.

EXCEPT when doing "SELECT COUNT(*) AS mycount FROM ..." as typically that form is faster than "SELECT COUNT(ID_SOMETHING) AS mycount FROM ...".

[Joshua Dickerson]

Everyone is coming up with excellent points. When I get some time I am going to update this. Might be a couple of weeks though.

[Daniel15]

EXCEPT when doing "SELECT COUNT(*) AS mycount FROM ..." as typically that form is faster than "SELECT COUNT(ID_SOMETHING) AS mycount FROM ...".

Technically, that's not a SELECT *, as you're not selecting all columns. You're simply passing * to the COUNT function. A SELECT * would be something like:
SELECT *, COUNT(*) AS mycount FROM ...

The COUNT(column_name) syntax is useful when you have a GROUP BY clause...

[Dannii]

I think maybe we should add a few more (problems I've come across from mods that conflict with mine):
Don't directly access $_REQUEST['board'] or $_REQUEST['topic'], use $board and $topic instead.
Don't overload the display action (ie, no action) to do stuff other than displaying. Make a new action instead. You can overload existing actions, but only by adding a subaction.

[Joshua Dickerson]

Can you give me an example of overloading? Not totally sure of what you mean. Good point about $board and $topic.

[Dannii]

The topic ratings mod puts its code directly into Display.php using a new variable rather than a new action. It's caused problems because my pretty URLs mod doesn't know that the user is actively trying to do something. It's not just my mod though, I think a general principle should be that messageindex and display are just for reading, and not for any user actions. If PHP had better support, display and messageindex would be accessed only by GET, while the other actions only by POST or PUT.

[karlbenson]

I think a general principle should be that messageindex and display are just for reading

Actually these days with web2.0, I'd expect alot MORE user interactive stuff than there has been before.

I agree though, it may be best to get a handle on it now with suggested 'correct/incorrect' ways of doing it.

[Dannii]

More user interaction is great, but not with GET
It's a shame PHP isn't suited for RESTful web apps.

[karlbenson]

gotcha!

[koji]

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

Where do we find out what these legendary variables are?

[Natoka]

very nice coding guidelines, really helpful, hopefully everybody follows them

what I am missing to some extent is some notes about languages / internationalization, since smf  has quite a nice support to do that, but I had to read the source code of several mods in order to get an understanding how to do that. Plus I am still a bit puzzled how I am going to do it, because there are several ways to do it. Additionally it would be nice to have some standards in that area too, so that translators will have an easier job to include mod xyz in their translation.

[jackey]

hey folks!
just started looking round here for some info about modding smf and saw this thread here.. so after i have spent half an hour searching the starting post for any links, doing a thorough forum search looking for those mysterious coding guidelines.. just yielded http://docs.simplemachines.org/index.php?topic=218, where it says it isnt up to date and a link to this topic..
dunno if i am missing something here, but from what i read in this topic, the guidelines could be really useful, so i would appreciate if someone would reveal to me the whereabouts of this magical document everybody praises so highly...

[B Patterson]

Attachments in the first post of this thread by groundup  link

[jackey]

well now i am REALLY confused.. dunno what happened there, maybe i failed to reload the page after i logged in or something, but the links definitely where not there when i looked there three times.. really sorry for acting like some newb here 

anyway, great work there, really covers everything i was unsure about.. are there any changes regarding smf 2.0beta (except those new database query functions) i should consider when coding a mod?

[B Patterson]

Keep in mind things may change   It's still a beta.  I'm willing to bet not much is going to change; however, things may change to fix bugs or fix security holes, so just keep up on the changelog during future beta releases.

[BizB]

You must be logged in to see the attachments' links.

[PJLawrence]

In the coding guide lines (smf coding guidelines.pdf) it states the following..

Do not indent statements in the global scope (not inside of a class, function, statement)
(see the example in Appendix B)

However, Appendix B does not seem to contain this information.
Or is it just me and I've missed it?

[Joshua Dickerson]

In the coding guide lines (smf coding guidelines.pdf) it states the following..

Do not indent statements in the global scope (not inside of a class, function, statement)
(see the example in Appendix B)

However, Appendix B does not seem to contain this information.
Or is it just me and I've missed it?

It's not you, I haven't had the time to do any of this. I was hoping someone could pick it up for me. Anyway, to give you an example of what that means:

Code Select Expand

<?php
    // Indented code. Not supposed to be indented here.
// This is where it should start. Being that it is in the global scope.
function show_you()
{
    $a = 'Notice how this is indented here. That is because it is inside a function';
}

// Notice the below is not indented because it is in the global scope.
show_you();
?>

I hope that helps you out some. If you want to edit the original file, I will fix the original post.

[Joshua Dickerson]

If you want to collaborate on this, I've uploaded it to Google Docs. Just give me your info and I will add you to the doc.

[Joshua Dickerson]

This will need tons of updating for SMF 2

[N3RVE]

This will need tons of updating for SMF 2

Indeed.

-[n3rve]

[ivan91tran]

Well I'm still working on my coding, so I don't want to correct you there. I did however do a thorough run-through of all the grammar and punctuation, and there seems to be nothing wrong. Hope that helps