It's still growing, but I'd appreciate any feedback on the clarity of what's in there as well as suggestions for additions before the translators start on it.
http://moodle.org/doc/?frame=coding.html
Its very useful.
The only slight point I think I would like to make is about variable names. I have spent years trying to get people to user longer variable names. Silly long names could be described as a good fault.
I personally have always subscribed to the Java style:
$thisIsAVeryLongVariable
but
$this_is_a_very_long_variable
is also good if harder to type; however
$thisisaverylongvariable
is no good, and I can see where you are coming from if you take the all lower case approach.
I strongly believe that good descriptive, readable variable names go a long way towards generating easy to understand code.
Don't hate me
In practice Moodle code is mostly dealing with arrays of objects, and the context is obvious, so one or two words has almost always been enough.
For example, if dealing with assignment submissions, there is no need to name the array $assignmentsubmissions, since it's ALL assignment stuff .. so just $submissions works fine, and "reads as English" very well, eg:
foreach ($submissions as $submission) {
$submission->marker = $user->id;
}
That said, if you really NEED to make a longer name by all means do so, as long as it reads well.
Or something...
I may be wrong but one important rule/style I keep to is the use of objects in function arguments. I'm working to the unspoken rule/style "If there's a choice between sending a function a dirt great object instead of a nice neat integer send in the object!" Now that's rather an extreme way of expressing it
The positive side is much much neater code, room for expansion in the function as it's got all the gubbings, and less hits on the database.
My impression is that in the Moodle development things started by passing integers, for example isstudent($course->id... and evolved to passing objects, for example print_table($table... But then I may be reading too much into it.
However, if it IS a rule/style it's a jolly good one, worth documenting and worth following. If it's NOT a rule/style I've got several thousands of lines of code to rehash Oh I'm not sure whether its a rule or a style
- In rule 12, let's hear it for one of my favourite functions - print_heading_with_help
In the example of Style 4, doesn't the statement
} else if (empty(...
break style rule 5? and should be
} elseif (empty(...
Nobody can say we don't read the Documentation
2. Next will be a complete API reference using phpdoc (Russell has already done some work for datalib.php).
3. Wow, that's a sharp eye but "elseif" and "else if" are synonyms aren't they? I always use "else if" anywhow.
This fine editor has a tool that I fell in love right a way, The Help Generator, which makes quite fine documentation.
I just made some example docs in http://edu.jaiko.fi/~janne/moodledocs
Username: moodle
Password: moodledev
Check it out
Janne,
That is nice...thanks.
I wonder if there's any way to get it locally or put it up on a server, maybe sourceforge.net?, where it could be accessed as needed?
I'd use it here locally, but I'd hate to use up your server resources with miscellaneous hits unnecessarily.
Thanks, Janne.
Les.
(Phpdoc comments were only done for datalib.php so far because I suggested to Russell there might be many cleanups in the other libraries for Moodle 2 and so not to bother yet ... comments in phpdoc form are still necessary to make these phpedit docs useful)
Still, even the function name and arguments is useful - I'll put these up somewhere in moodle.org.
I'd love to see something like section 6 of OpenClinic's guidelines:
http://openclinic.sourceforge.net/openclinic/coding_guidelines.html
For some info on what options can be used in phpdoc comments see:
http://www.phpdoc.org/docs/HTMLSmartyConverter/default/phpDocumentor/tutorial_phpDocumentor.howto.pkg.html#documenting.elements
if reading int value arguments ensure that the data is valid, especially before inserting into the database
$day = intval($_GET['cal_d']);
vs.
$day = $_GET['cal_d'];
Pretty basic things like this and perhaps the proper times to use p() and s() from weblib would be good guidelines in my opinion.
These guidelines are very helpful. I would like some similar guidelines regarding the database. I think I have gathered the following by looking at current practice:
- Every table shall have an id (INT(10)) field as primary index and have auto_increment turned on.
- The table containing instances of a module shall have the same name as the module, e.g., newmodule.
- Other tables associated with a module and holding information about things shall be called newmodule_things (notice the plural in things).
- Table fields that hold the id number of entries in another table, let us call it object shall either be called object or objectid (Martin, do you have a preference here? Both seem to be used equally at the moment.)
- Boolean table fields shall be implemented as TINYINT(2) rather than as BOOLEAN, with 0 corresponding to false and 1 to true.
- name fields shall be of type VARCHAR(255).
- Tables shall have a timemodified (INT(10)) field and code updating the database is required to set this with a PHP timestamp obtained with time(). (Martin: which tables arer required to have a timemodified field and which ones don't?)
- All table fields shall have attribute NOT NULL. Zero or the empty string are to be used instead of NULL (Martin: is this really a rule and if so, why?)
I've added database guidelines to the coding guidelines page.
The difference between object and objectid is that the latter is a newer standard to help self-documentation in the code. It also avoids some problems with reserved words (eg when PostgreSQL support was added I had to go through all the code and change "user" fields to "userid" just so it could work). All new tables should refer to id fields in other tables using objectid and eventually I suppose we might fix the old tables someday.
timemodified should go in almost all new tables, unless it really is never going to be useful there.
The NOT NULL thing is not a standard .. it's just what phpMYAdmin adds by default and it stuck. It shouldn't matter either way to PHP/Moodle, which treats all empty things as equal.
And if you create table names in your prototype, do not forget to use the prefix:
$CFG->prefix = 'mdl'; //user chooses his local prefix (in config.php)
I'm using your guidelines while trying to modify a Wiki application to fit into Moodle as a module.
One thing I've come against; I'm guessing that some of the Moodle code requires a field called 'name' as well? If this isn't true, how can I redefine my module to use a field called something other than 'name' as the display name?
mike
It also looks like you need to have a field called 'course', which contains the 'id' for the course that the module belongs to.
mike
That is a good point. The way the NEWMODULE template is at the moment it indeed assumes in mod.html that the field that contains the course id is called 'course'. This conflicts with the coding guidelines.
cvs:/moodle/mod/newmodule_template.zip deserves an overhaul anyway. For example it should contain a config.html.
As Gustav said the use of "course" should really be "courseid" but since this convention came later the older core tables don't always use it (and fixing them now would be a HUGE effort). I've updated the docs to make this clearer too.
If an element shows up in the Main Menu, it uses the name field to display it. I'm not sure what part of Moodle displays the Main Menu. There may be other occurrences as well.
mike