Difference between revisions of "Talk:Coding style"

Jump to: navigation, search
m (Proposed change to inline comments style (CONTRIB-3564))
Line 443: Line 443:
  
 
:This has been applied to the rules, ciao :-) --[[User:Eloy Lafuente (stronk7)|Eloy Lafuente (stronk7)]] 18:54, 2 April 2012 (WST)
 
:This has been applied to the rules, ciao :-) --[[User:Eloy Lafuente (stronk7)|Eloy Lafuente (stronk7)]] 18:54, 2 April 2012 (WST)
 +
 +
 +
== Filenames standardisation ==
 +
https://docs.moodle.org/dev/Coding_style#Filenames does not say how should the files be named (file_names_like_this.php vs filenameslikethis.php). At the moment we have a mix but we could decide that we use one naming convention for the new files at least.
 +
 +
+1 for file_names_like_this.php --[[User:Tomasz Muras|Tomasz Muras]] 00:59, 4 April 2012 (WST)

Revision as of 16:59, 3 April 2012

PHP5 constructor

Should we enforce the PHP5 constructor __construct() instead of $classname() Nicolas Connault 19:55, 19 May 2009 (UTC)

+1. I think we agreed about that some time ago (when igniting some 2.0 developments) --Eloy Lafuente (stronk7) 00:03, 20 May 2009 (UTC)

Inline comments

I have been using since ages ago "///" with outer alignment (I think that it was caused by some agreement long time ago, not my decision), for example:

function check_moodle_environment(..., ..., ..., ...) {
 
    $status = true;
 
/// This are cached per request
    static $result = true;
    static $env_results;
    static $cache_exists = false;
 
/// if we have results cached, use them
    if ($cache_exists) {
        $environment_results = $env_results;
/// No cache exists, calculate everything
    } else {
    /// Get the more recent version before the requested
        if (!$version = get_latest_version_available($version, $env_select)) {
            $status = false;
        }
        ....
        ....

Is that allowed, or only the "//" with inner alignment as commented at inline comments --Eloy Lafuente (stronk7) 00:03, 20 May 2009 (UTC)


I did start doing all mine this way, but if you look at the code it seems you and I are the only ones.  :P It seems sensible to make the standard fit what most people are used to (and how most of the code looks). -- Martin Dougiamas 06:02, 9 June 2009 (UTC)


Eloy and Martin, this the only thing which consistently offends me about moodle coding style! Please go inline like all the cool kids ;-) --Dan Poltawski 00:15, 12 June 2009 (UTC)

LOL. +1 for inline in even lines + out in odd one  :-P (seriously np with inline at all, it's only one habit easily modifiable) --Eloy Lafuente (stronk7) 01:03, 12 June 2009 (UTC)

Trailing spaces

"Lines should not contain trailing spaces. In order to facilitate this convention, most editors can be configured to strip trailing spaces, such as upon a save operation. However, if you are editing an existing Moodle file and are planning to submit your changes to CVS, please switch off that feature so that whitespace changes do not pollute CVS history (other developers will have trouble viewing what you've done if every other line has been edited for whitespace)." Wouldn't the CVS history only get 'polluted' when someone fetched a file that DIDN'T follow this principle and returned it following the principle? Isn't that the price that has to be paid to clean things up? Otherwise this is pretty scary for someone who feels: "Great I can turn on my editor to autostrip those when saving. BUT I have to remember to turn that off if submitting it! (Or become a 'polluter'!)" And what happens if I have already saved it locally while working on (and striped those trailing spaces) and later try to commit it. Maybe my ignorance about CVS is making me pose a "silly" question. Jeff Forssell 07:17, 27 May 2009 (UTC)

Jeff, the point with coding guidelines is that all new code must follow all the guidelines. With old bad code, mostly it is better to make the smallest change necessary when, for example, you fix a bug. That makes it clearest what the purpose of your change was. However, any line you do change while fixing the bug, you can then improve the coding style on that line.
Really good editors actually have an option "Strip trailing whitespace when saving a file - but only from lines that I have edited", which is what you really want.
The real way to avoid being a CVS polluter is to follow the best practice and always do a cvs diff and review all your changes (and if necessary amend them) before you do a CVS commit.--Tim Hunt 03:52, 1 June 2009 (UTC)

Breaking up lines (e.g. huge arrays)

The coding style says to align the start of the following lines with the element in the first.. e.g.

$myarray = array( 'fred' => 'blue',
                  'tom'  => 'green' );

I have to say I don't like this. While it looks very pretty it is a code maintenance nightmare. If you change (say) the array name you have to change every other line just to keep it looking nice. Surely if you must have it looking like this, do this...

$myarray = array(
    'fred' => 'blue',
    'tom'  => 'green' );

It's the same issue with long lists of assignments - making all the RHSs line up. Completely unnecessary and just asking for errors. Don't encourage people to change code that they don't have to. If they don't touch it then they can't break it. --Howard Miller 13:52, 1 June 2009 (UTC)

Agree 100% with Howard, I prefer the 2nd alternative for sure, if not always, at least in places where indentation is already considerable. BTW, same for harcoded SQLs and other strings. --Eloy Lafuente (stronk7) 13:58, 1 June 2009 (UTC)
+1 from me too for a fixed indent for follow-on lines. Actually I have always used 8-spaces (double the normal indent) ever since I started programming in Java and read their docing guidelines. I think it is helpful to have a different level of indent for a follow-on line and a block.
Sure, makes sense. I've fixed the docs. The only exception IMO would be wrapping function parameters, which just look too wierd to me if you wrap them the same as arrays. See the example. -- Martin Dougiamas 06:08, 9 June 2009 (UTC)
At the expense of some white space...
public function graded_users_iterator(
    $course, 
    $grade_items = null, 
    $groupid = 0,
    $sortfield1 = 'lastname', 
    $sortorder1 = 'ASC',
    $sortfield2 = 'firstname', 
    $sortorder2 = 'ASC') {
There is an argument that a function is badly thought out if it needs line after line of parameters. Won't go there though :-) --Howard Miller 09:51, 23 June 2009 (UTC)

Function and method declaration

Coding_style#Function_and_method_declaration says "Don't leave spaces between the function name and the opening parenthesis for the arguments." and yet the example below it completely disregards it. Please fix it. --Kumaraguru G 21:32, 4 July 2009 (UTC)

Hi, Kumaraguru. This is a wiki so you can just do it yourself ;-) --Frank Ralf 21:45, 4 July 2009 (UTC)
Hi Frank, the page is protected and I don't have edit access :) --Kumaraguru G 00:30, 5 July 2009 (UTC)
Oops, sorry, totally forgot about that. --Frank Ralf 06:11, 6 July 2009 (UTC)
I just fixed that, and a few other things I noticed.--Tim Hunt 03:13, 5 July 2009 (UTC)

When you have shoved too much into one function

I'll just quote the Linus Torvalds in the Linux Kernel coding style guide because he says it perfectly...

Chapter 6: Functions

Functions should be short and sweet, and do just one thing.  They should
fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24,
as we all know), and do one thing and do that well. 

The maximum length of a function is inversely proportional to the
complexity and indentation level of that function.  So, if you have a
conceptually simple function that is just one long (but simple)
case-statement, where you have to do lots of small things for a lot of
different cases, it's OK to have a longer function. 

However, if you have a complex function, and you suspect that a
less-than-gifted first-year high-school student might not even
understand what the function is all about, you should adhere to the
maximum limits all the more closely.  Use helper functions with
descriptive names (you can ask the compiler to in-line them if you think
it's performance-critical, and it will probably do a better job of it
than you would have done). 

Another measure of the function is the number of local variables.  They
shouldn't exceed 5-10, or you're doing something wrong.  Re-think the
function, and split it into smaller pieces.  A human brain can
generally easily keep track of about 7 different things, anything more
and it gets confused.  You know you're brilliant, but maybe you'd like
to understand what you did 2 weeks from now.

and...

The answer to that is that if you need more than 3 levels of indentation, you're screwed anyway, and should fix your program.

+1 for adopting this into the coding style guide --Gerard Caulfield 17:28, 28 February 2012 (WST)

Running empty lines

I propose to allow only single empty line as a separator. We can get rid of running empty lines at once using

for file in `find . -name "*.php"`; do cat -s $file > /tmp/cat.php && cat /tmp/cat.php > $file  ; done;

If agreed, the fix of the template for the beginning of a file is needed. --David Mudrak 15:25, 18 July 2009 (UTC)

I use a single line everywhere, except that I use two blank lines between classes. I don't think we need to make a big deal about this.--Tim Hunt 09:42, 19 July 2009 (UTC)

File comments

1. About packages. I think what is there is OK. Separate packages for moodlecore and plugs. However, I don't think it is sufficient. There are some parts of the code (I am thinking of question) that are relatively self-contained. I think it is silly to put them all in moodlecore. I would rather use packages questionbank and questionengine. Is that OK. If so, can someone (I am happy to) add this to the docs. message, notes and comments may be other good candidates for separate packages. Also course and user.--Tim Hunt 10:10, 2 October 2009 (UTC)

Martin Dougiamas 06:31, 5 October 2009 (UTC): The problem is that we don't want to have hundreds of packages in the list at the top of http://phpdocs.moodle.org/HEAD/index.html so this has to be controlled. In fact we already have all the ones you mentioned there ... there seems to be some confusion with different guesses/versions at what the name should be (group/groups is one example). I'm against courses and users because functions for these are in many different mixed files. We have the new Moodle API externallib.php files in 2.0 which will really list all the main functions people would need. Frankly it's a mess and I think trying to add lots more distinction makes it worse... The main thing we were trying to do was separate core from modules.
I disagree. It is stupid to worry about the list of packages being so long that it is difficult to understand if, once you get into a particular package, the list of stuff in there is so long it is completely incomprehensible. The whole point of packages is to break the codebase up into manageable and relatively independent chunks to help people understand it.
Really, the problem here is that the PHPdocumentor template we are using is not up to the job of displaying a project as big as Moodle. We should use a template more like the JavaDoc on (http://java.sun.com/javase/6/docs/api/). That scales much better. Job for Jordan?
And it would make sense to ensure that all the core packages were listed together. Perhpas we should adopt a convention like core-question-bank and core-question-engine for core sub-packages. I agree that too many packages would still be a mistake, but so is too few.--Tim Hunt 08:37, 5 October 2009 (UTC)
Martin Dougiamas 12:58, 5 October 2009 (UTC): Isn't that what we have subpackages for? We're using them that way all over the place already (eg see blog and deprecated etc under moodlecore). That makes more sense to me than going for long names. At least for now. A lot of core functions are not as easy to separate as questions might be, and if we do it for some and not others then we have to explain that decision-making process too ...
Tim Hunt 20:32, 5 October 2009 (UTC) OK. I am happy to use subpackages. I'll follow those two examples.

2. The OU likes me to put

* @copyright © 2006 The Open University

which is fair enough. In this case, I think it would make sense to add an extra

* @author ...

tag, even though in other cases we don't require them.--Tim Hunt 10:10, 2 October 2009 (UTC)

Martin Dougiamas 06:31, 5 October 2009 (UTC): Sorry but I totally disagree this should be in a @author tag at all. You are probably not the sole author in the first place, and others will edit them in future and think they have to start adding their own name to any file they touch. It gets contentious. That's why we pulled them all out in the first place. CVS has full author information for every line, no need to put it in the file. You can put your name in the comments at the top of you like (something like "Initially written by Tim Hunt").
Ah, fair enough. I was forgetting that the purpose of that like was just to make the copyright owner clear. I completely agree with you about not duplicating information in CVS.--Tim Hunt 08:37, 5 October 2009 (UTC)

I just updated the bit about @package names, following discussion and a clear consensus with Petr: http://moodle.org/mod/cvsadmin/view.php?conversationid=4294.--Tim Hunt 10:47, 3 March 2010 (UTC)

Wrapping Control Structures

The example given suggests that:

if (f() and g()) {
 
}

is equivalent to:

$tmp1 = f();
$tmp2 = g();
if ($tmp1 and $tmp2){
 
}

But in the former g() is evaluated only if f() returns a value that is true (in a boolean context).

I suggest something like

BETTER GOOD:

$coursecategory = $element['object']->is_course_item() or $element['object']->is_category_item();
if ($coursecategory) {
    $scalevalue = in_array($element['object']->gradetype, array(GRADE_TYPE_SCALE, GRADE_TYPE_VALUE));
    if ($scalevalue){

I also suggest that the code fragments in GOOD and BAD be made to agree in the rest of the logic. (GOOD has form ((a or b) and c) while BAD has form ((a or b) and (c or d)).

User:jfine 7.35, 1 Aug 2010 (UTC)

Sensible temporaries

Here's some code thought to be BAD, improved with temporaries (and a probably trivial change of meaning).

$eo = element['object'];
$eog = $eo->gradetype;
 
if (($eo->is_course_item() or $eo->is_category_item())
    and ($eog == GRADE_TYPE_SCALE or $eog == GRADE_TYPE_VALUE)) {

There. The BAD code is now not so bad after all. User:jfine 00:76, 1 Aug 2010 (UTC)

Database code

I would suggest the following database coding guidelines

Linked to Developer_FAQ#How_do_I_insert.2Fretrieve_records_in_the_database.2C_without_creating_my_own_database_connections.3F --Frank Ralf 10:54, 14 January 2011 (UTC)


Database code using SQL

  • Where possible, use the plain get_records, get_recordset, etc functions instead of the _sql variants.
Correct
$DB->get_record('course', array('id' => $id));
Wrong
$DB->get_record_sql('SELECT * FROM {course} WHERE id=?', array($id));
  • When using SQL code, do not add variable parameters directly into the string. Instead, use the new methods for including parameters.
Correct
$DB->get_record_sql('SELECT * FROM {course} WHERE id <> ?', array($exceptid));
Wrong
$DB->get_record_sql('SELECT * FROM {course} WHERE id <> ' . $exceptid);

(Using parameters properly avoids possible serious security holes.)

  • When using SQL code, do not use the $CFG->prefix variable. Instead use the new {tablename} syntax.
Correct
$DB->get_record_sql('SELECT * FROM {course} WHERE id <> ?', array($exceptid));
Wrong
$DB->get_record_sql("SELECT * FROM {$CFG->prefix}course WHERE id <> ?', array($exceptid));

Formatting arrays

As reported by Tomasz Muras in MDLSITE-1187:

--Helen Foster 14:06, 28 January 2011 (UTC)

Acutally, Martin chose to do arrays like that. You can argue that it is inconsistent, but actually it looks nice, so I like it the way it currently is in Coding style.--Tim Hunt 16:30, 28 January 2011 (UTC)

String Concatenation

Multiline strings should use a sensible approach. While most people put the concatenator at the end of the line, it's much clearer if they're at the start of the line. That way you don't have to look up a line and over to see if the line is actually the same string or a new one. Additionally, it moves a character from the end of the line to unused space at the start of line which is just a little bit more efficient use of horizontal space.

Example:

$string = 'This is a very long and stupid string because '.$editorname.
          " couldn't think of a better example at the time.";

should be

$string = 'This is a very long and stupid string because '. $editorname
        . " couldn't think of a better example at the time.";

This doesn't have to be required, but it should be allowed, or else you're enforcing poor style. --Tyler Bannister 01:27, 16 July 2011 (WST)

More detailed suggestion for doc comments

This is from local stuff. I wrote more detailed guidelines about doc comments. Note these are actually a bit different from the one in the guidelines (I don't think it was there then?) (by 'different' I mean 'correct') but the main point is there is more detail about some of it.

Which functions need comments?

Every function should have a comment except:

  • Comments are welcome, but optional, for private functions (any function in a class that has the 'private' keyword). Generally you might omit comments for a simple private function, but include them for a complicated one.
  • Comments are optional and generally not recommended for a function that is defined in a base class. In that case it is usually more appropriate to use the base class definition.

Example

Here is an example PHPdoc function comment. An explanation follows.

/**
 * Obtains the key from underneath the mat or flowerpot. Tries the
 * mat first.
 *
 * Due to variations in the total perspective vortex, it may sometimes
 * be necessary to borrow the key for other functions. In that case,
 * this function will throw an exception.
 *
 * @param door_accessory $mat Welcome mat
 * @param string $flowerpot Flowerpot - e.g. 'bill and ben'
 * @return object Key with fields ->id, ->name, ->secretcode
 * @throws moodle_exception If key not present
 */
function get_key(door_accessory $mat, $flowerpot) {

Description

The first part of the comment, before any @commands, is the description.

  • The description should be one or more sentences ending in a full stop. It can run across multiple lines of text.
  • If you need a really long description you can leave blank lines in the middle of the description to indicate a paragraph break.
  • There should be a blank line after the description.
  • The first sentence of the description summarises what the function does. It should be of the form 'Gets...', 'Sets...', 'Displays...', 'Loads...', etc. (This might not technically be a full sentence; you can imagine inserting the words 'This function' before the start in order to turn it into one.)
  • You can leave out the description altogether if the function is adequately described by @param and/or @return values (this is usually the case for simple get_whatever and set_whatever functions).

@param, @return, @throws

These should all be formatted similarly.

  • Immediately after the @-keyword you need to define a type.
    • If the variable is an object, use the class name of the object (e.g. door_accessory). For stdClass objects, use the keyword object.
    • If it is a primitive type, use a keyword from this list: string, int, bool, number, array.
    • If the variable might have multiple possible types, list them with | symbol and no space e.g. int|string|carbon_based_lifeform.
  • An explanation follows (after the parameter name, in the case of @param).
    • The explanation should be short, frequently two or three words.
    • It should begin with a capital letter.
    • It must be a single line. If it needs to be longer, put it up top in the description part instead.
    • It does not need to be a full sentence and should not end in a full stop. Use comma, semicolon or dash if you need more than one clause.
    • It's OK to duplicate the variable name when there is nothing else to say.
    • For string variables it is a good idea to include an example of suitable parameter.
    • For int variables where the int should be a constant, indicate which kind of constant, e.g. @param int $format Moodle FORMAT_xx constant
    • For stdClass object variables, it is often useful to indicate which fields are required or returned, e.g. @param object $course Course object with at least ->id and ->shortname fields
  • For @throws, normally begin with 'if' so that the line can be read 'Throws moodle_exception if something goes wrong' (@throws moodle_exception If something goes wrong)
  • If you want to indicate that a function doesn't return a value, don't include an @return line.

@global, @uses

Do not use @global or @uses; the syntax is horrible and it makes a mess of the doc block when reading it as part of the code.

--sam marshall 00:26, 26 November 2011 (WST)

@var syntax

I think there is one miss-interpretation of the examples @ phpdocs @var. The correct format to document such tags is, like params, like everything else: @var + type + description, aka, the description AFTER the tag.

And I've seen a lot of bad uses assuming that the line BEFORE the tag is the description. 100% wrong IMO. And now there is one wrong example in this page. Please amend it. Descriptions go, always, after the tag. The comment before the tag are, simply, comments, no descriptions!

--Eloy Lafuente (stronk7) 17:04, 15 February 2012 (WST)

require() vs require_once()

I've just noticed that numerous examples refer to require rather than require_once. This seems to be something Petr has added, yet there are 3992 instances of require_once() and 250 instances of require() in the Moodle codebase. Is there rationale for require? --Dan Poltawski 17:33, 4 January 2012 (WST)


Please can we not do @return void

The change made here is horrible: https://docs.moodle.org/dev/index.php?title=Coding_style&action=historysubmit&diff=31619&oldid=31616

The purpose of PHPdoc is to document the API of a function/method - so that people calling the function know how to use it. Whether the code inside the method just gets to the end and returns, or there is an earlier explicit return; is an implementation detail. Therefore, that detail should not affect what is written in the PHPdocs.

See also http://stackoverflow.com/questions/2061550/phpdoc-return-void-necessary (found by Eloy) - which says this is incorrect.--Tim Hunt 18:16, 20 January 2012 (WST)

+1 to avoid "@return void" completely - adds no value and is 75% incorrect (25% correct, if we assume imagination is accepted) --Eloy Lafuente (stronk7) 18:21, 20 January 2012 (WST)
+1 to avoid "@return void", -1 to "found by Eloy" chat log says no =P --Gerard Caulfield 17:55, 15 February 2012 (WST)
LoL, Gerard, I really found it (myself). But ok, np, credit goes to you, LoL --Eloy Lafuente (stronk7) 18:35, 16 February 2012 (WST)
+-0 - void tells us that the function wasn't written to return any data. not even null. so even though php returns a technical null when a function runs to the end and returns, the developer can know that it is void of any meaning and steer clear of any misconceptions and misuse. That is the slight added value - seeing that slight bit more about our dearly written function. --09:32, 22 February 2012 (WST)Aparup Banerjee

Wrapping Control Structures

The "GOOD" example given is as follows:

$coursecategory = $element['object']->is_course_item() or $element['object']->is_category_item();
$scalevalue = in_array($element['object']->gradetype, array(GRADE_TYPE_SCALE, GRADE_TYPE_VALUE));
 
if ($coursecategory and $scalevalue) {

This appears like it could make good use of short circuit evaluation, however we appear to be discouraging it. This could lead to performance problems if this rule were to be followed everywhere. Perhaps a better example instead of in_array() could be used? Putting the in_array() statement into a funciton and then calling the function from inside the conditional would keep the readability advantages while also documenting what the statement is actually trying to achieve, while not sacraficing performance. It is perhaps not ideal, but probably better than what we have in the example. There are other options, but as it stands I'm not sure what we currently have is a great example of "GOOD". Discuss... --Gerard Caulfield 17:48, 15 February 2012 (WST)

Request for clarification, file/classes phpdocs fallback

If one file has @category CAT, @package PACK, @license LIC and, @copyright COP.

And all classes and methods within it have exactly the same... do we need to repeat them in all classes? Or automatic fallback (inheritance) of all them is assumed? it's not clear in the Docs page, right now. It only talks about files with ONE class.

Edited: phpDocumentor says that they must be specified everywhere because no fallback from file to class happen. The only exception is that inheritance between classes happen at least for @package. They strongly recommend to specify the @package tag everywhere, but don't say mush about the rest of tags. So, surely we need to decide and later make System X to work accordingly.

--Eloy Lafuente (stronk7) 20:31, 21 February 2012 (WST)

Well, 1. heading towards a coding style where each file contains one PHP class might be a good thing anyway. 2. Martin has always said to put @copyright and @license onto each class, because they are a unit of code that people are quite likely to copy-and-paste into their own projects. 3. duplication is a pain in the bum, but if necessary it is not the end of the world.--Tim Hunt 22:02, 21 February 2012 (WST)

Proposed change to inline comments style (CONTRIB-3564)

Some days ago it was asked @ HQ chat if anybody had anything against to "enforce" the "// " (2 slash + space) rule more clearly. Nobody issued any -1.

So, with the work performed @ CONTRIB-3564 this is a request to change the "Inline_comments" section in the coding style from:

 Inline comments should use the // style, laid out neatly so that it fits among the code and lines up with it.

to:

 Inline comments must use the "// " (2 slashes + whitespace) style, laid out neatly so that it fits among the code
 and lines up with it. Also, beginning with uppercase and using correct punctuation marks at the end is recommended.

(and, of course, change all the examples along the page to fulfill those rules and recommendations.

Ciao :-)

This has been applied to the rules, ciao :-) --Eloy Lafuente (stronk7) 18:54, 2 April 2012 (WST)


Filenames standardisation

https://docs.moodle.org/dev/Coding_style#Filenames does not say how should the files be named (file_names_like_this.php vs filenameslikethis.php). At the moment we have a mix but we could decide that we use one naming convention for the new files at least.

+1 for file_names_like_this.php --Tomasz Muras 00:59, 4 April 2012 (WST)