#147 enhancement
Nick Stakenburg

Consistent coding style. Source and unittest cleanup.

Reported by Nick Stakenburg | June 5th, 2008 @ 05:56 PM | in 1.7

There is inconsistency in coding style throughout the source and unittests. I decided to take care of this with a big cleanup, available in my fork:

http://github.com/staaky/prototype/commits/master

I've attempted to pick the most elegant solutions while keeping good readability. Everything is based on what was there, only making things more consistent.

Styleguide

1) Do not rely on automatic semicolon insertion:

// OK:
function foo() { bar++; }

// NOT OK:
function foo() { bar++ }

2) Use comma seperated and 4 spaced variable statements:

var one   = foo.first(),
    two   = foo.last(),
    three = element.tagName || 'blah';

3) Curly braces can be omitted within if...else statements only when all statements are oneliners:

// OK:
if (foo.bar) foo.bar();
else noBar();

// NOT OK:
if (foo.veryLongMethodNameHere)
  foo.veryLongMethodNameHere(); // <--- NO LONGER A ONELINER
else noBar();

// Should be rewritten as:

if (foo.veryLongMethodNameHere) {
  foo.veryLongMethodNameHere();
} else {
  noBar();
}

// NOT OK:
if (foo.bar) foo.bar();
else {
  noBar();
  log('Missing bar');
}

// Should be rewritten as:

if (foo.bar) {
  foo.bar();
} else {
  noBar();
  log('Missing bar');
}

4) Curly braces in if...else, do...while, etc. belong on the keyword's line, not on the line before:

// OK:
if (foo.bar) {
  foo.bar();
} else {
  noBar();
}

// NOT OK:
if (foo.bar) {
  foo.bar();
}
else {
  noBar();
}

5) Avoid ternary operators at the beginning of a line:

// NOT OK
var foo = bar.startsWith('foo')
          ? 'foo' : baz.endsWith('bar')
          ? 'bar' : 'baz';

// Should be rewritten as:

var foo = bar.startsWith('foo') ? 'foo' :
          baz.endsWith('bar') ? 'bar' : 'baz';

Updated based on the discussion in this ticket - 11.09.'9

Comments and changes to this ticket

  • Nick Stakenburg

    Nick Stakenburg June 5th, 2008 @ 05:54 PM

    One correction/addition to that guideline.

    // newlines don't start with operators.
    var one = foo.first(),
     two = foo.last(),
     three = element.tagName ? element.tagName.toUpperCase() :
      'blahblahblah';
    
  • John-David Dalton

    John-David Dalton June 5th, 2008 @ 06:05 PM

    • State changed from “new” to “enhancement”
    • Assigned user set to “Andrew Dupont”

    Thanks for all the cleanup and things.

    You may want to wait for the core to become stable before doing all of this though.

    A lot of the core code is in flux. So many of your changes wont have any bearing.

  • Nick Stakenburg

    Nick Stakenburg June 5th, 2008 @ 07:07 PM

    No problem. When 1.6.1 is there I'll have another look.

  • John-David Dalton

    John-David Dalton June 5th, 2008 @ 09:56 PM

    • Milestone set to 1.6.0.3
  • Juriy Zaytsev

    Juriy Zaytsev June 6th, 2008 @ 04:55 AM

    • Milestone changed from 1.6.0.3 to 1.7

    I would vote for:

    var foo = element.tagName 
      ? element.tagName.toUpperCase()
      : 'blahblahblah';
    

    Imo, by bringing operators upfront, it's much easier to navigate through the code. This syntax makes ternaries stand out from other expressions.

    Also, I find 1 space in front of comma-separated variable declarations less readable than when using 2 spaces.

    var foo = 'bar',
        bar = 5,
        baz = true;
    

    The left alignment in such case is very helpful.

  • Nick Stakenburg

    Nick Stakenburg June 6th, 2008 @ 01:58 PM

    The 4 spaces on extra vars could be nice, it would make things a bit easier to read.

    For readability I'd prefer the operators at the end, so you know what to expect on the next line before you read it.

    Operators are not often used up front in the code. When adding strings together for example, the + is always added at the end, it's something most people do naturaly. I'd leave it like that.

  • John-David Dalton

    John-David Dalton June 6th, 2008 @ 02:29 PM

    Ya now that there is discussion over the formating a 1.6.1 milestone is more appropriate.

    I was thinking that the patches just made things more consistent in the coding using established coding styles in the code. If this is about changing them or adding new styles I think everything would need to be run by Sam as I believe most of the style is dictated by his preference.

    For example I believe Sam doesn't like this

     element.observe('click', function foo() { bar++; });
     //but instead likes this
     element.observe('click', function foo() { bar++ }); //without the inner semi-colon
    

    When dealing with a list of variables I have seen Andrew prefer the "1 space" style:

     var foo = 'happyCowsComeFromCalifornia',
      bar = true,
      baz = 10;
    

    I think its good but, I think somtimes the "4 space" style, as Juriy points out, is helpful. I think it depends on the context of the surrounding code and I don't think one style is appropriate in all situations. For example, I wouldn't add those extra spaces if its a list of two or something.

  • Juriy Zaytsev

    Juriy Zaytsev June 6th, 2008 @ 03:31 PM

    The number of declarations doesn't seem to matter.

    I still find left alignment more readable (both, first character and "=" sign alignment definitely bring a balance).

    var foo = 'bar',
        baz = 5;
    
  • Nick Stakenburg

    Nick Stakenburg June 6th, 2008 @ 04:02 PM

    Good idea JDD, I never liked those inner semi-colons.

    I think '4 spaces' is more readable. '1 space' is good and might combine a bit better with the '2 space' style. Unless you want to go for 6 spaces in a situation like this:

    var foo = 'happyCowsComeFromCalifornia',
        bar = (moo == 1234567890) ? 'blablah' :
          (baz == moo) ? 'blahblabarbapapa';
    
    var foo = 'happyCowsComeFromCalifornia',
     bar = (moo == 1234567890) ? 'blablah' :
      (baz == moo) ? 'blahblab' : 'barbapapa';
    

    I'm undecided. What would Sam do?

  • Juriy Zaytsev

    Juriy Zaytsev June 6th, 2008 @ 04:20 PM

    The reason for bringing ternaries upfront is that it's easy to confuse ":" with ";" at the end of the line:

    var foo = 'happyCowsComeFromCalifornia',
        bar = (moo == 123) ?
          'blahblah' :
          'harharhar';
    

    vs.

    var foo = 'happyCowsComeFromCalifornia',
        bar = (moo == 123)
          ? 'blahblah'
          : 'harharhar';
    
  • Nick Stakenburg

    Nick Stakenburg June 6th, 2008 @ 04:41 PM

    For me operators feel more natural when they stay at the end. In front they might improve readability when skimming through code. But it doesn't make things easier when reading from top to bottom, most of the time.

    I'd expect something at the end of each line that tells me how to interpret the next, that adds to readability. I'd rather have ':' and ';' there for that reason, how easily do we mix those up?

  • ronin-24025 (at lighthouseapp)

    ronin-24025 (at lighthouseapp) September 9th, 2009 @ 04:41 PM

    If you're consistently spreading the ternary operator across more than one line, you're using it to do too much. Same for using a single 'var' to define multiple variables or worse, multiple variables across more than one line. Using one 'var' for each variable helps to limit the number of variables defined in the first place.

  • Andrew Dupont

    Andrew Dupont September 10th, 2009 @ 06:32 AM

    A few thoughts:

    1. Juriy, Nick: I much prefer ternary operators on the end of the line. A line break after an operator signals (both to the user and to the interpreter) that the next line continues the statement.

    2. Joran: we don't use the ternary operator enough to be spreading it across multiple lines "consistently." And, while I don't subscribe to Crockford's bizarre behavior (declaring at the top of a function all the variables you're going to use in the entire function), I think using a single var to define multiple variables (usually no more than 3–4) enhances readability and avoids excessive line breaks.

    3. Nick: The last example in the ticket summary is incorrect. A function expression (i.e., anonymous function) would need a semicolon after if (if it ends the statement); its braces act as part of the function literal syntax. A function declaration should not have a semicolon after it; its braces act as a control structure, just like the braces in an if statement.

    4. For quite awhile I've preferred one space of indentation when continuing a statement on a second line. I'm willing to switch to four spaces, though, because the one-space approach drives TextMate mad and is often too subtle.

  • Tobie Langel

    Tobie Langel September 10th, 2009 @ 05:04 PM

    @andrew: agreed. Thanks for taking the time to clarify this.

    I also agree with kangax regarding alignment in var statements.

    I prefer

    var div         = document.createElement('div'),
        form        = document.createElement('form'),
        isSupported = false;
    

    over:

    var div = document.createElement('div');
    var form = document.createElement('form');
    var isSupported = false;
    

    Finally, I think that ternary operators spread across more than one line are at best difficult to read and a worst a sign of code-smell. I think we should strive to avoid them.

  • Nick Stakenburg

    Nick Stakenburg September 10th, 2009 @ 08:42 PM

    Andrew: You're right, I've fixed the summary.

    I like the extra spaces Tobie adds for alignment, that one seems like the most readable (and compressor friendly).

    I could patch 1.6.1 keeping all of this in mind but I don't know what's planned as far as changes go for 1.7, guess it's better to wait.

  • Tobie Langel

    Tobie Langel September 10th, 2009 @ 10:55 PM

    You're right about waiting.

    In the meantime, turning the above into a style guide (which we could host on prototypejs.org) and which would include the above, plus stuff like:

    • when to use // and / / comments (Sprockets strips the former out, not the latter),
    • when to use whitespace (for example, after keywords and inside object literals),
    • etc.

    would be extremely useful.

  • Tobie Langel

    Tobie Langel September 10th, 2009 @ 11:18 PM

    For the record I disagree with parts of your above list:

    1) We should not rely on automatic semicolon insertion:

    // OK:
    function foo() { bar++; }
    
    // NOT OK:
    function foo() { bar++ }
    

    2) curly braces can be omitted within if...else statements only when all statements are oneliners.

    // OK:
    if (foo.bar) foo.bar();
    else noBar();
    
    // NOT OK:
    if (foo.veryLongMethodNameHere)
      foo.veryLongMethodNameHere(); // <--- NO LONGER A ONELINER
    else noBar();
    
    // Should be rewritten as:
    
    if (foo.veryLongMethodNameHere) {
      foo.veryLongMethodNameHere();
    } else {
      noBar();
    }
    
    // NOT OK:
    if (foo.bar) foo.bar();
    else {
      noBar();
      log('Missing bar');
    }
    
    // Should be rewritten as:
    
    if (foo.bar) {
      foo.bar();
    } else {
      noBar();
      log('Missing bar');
    }
    

    3) Curly braces in if...else, do...while, etc. belong on the keyword's line, not on the line before:

    // OK:
    if (foo.bar) {
      foo.bar();
    } else {
      noBar();
    }
    
    // NOT OK:
    if (foo.bar) {
      foo.bar();
    }
    else {
      noBar();
    }
    
  • ronin-24025 (at lighthouseapp)

    ronin-24025 (at lighthouseapp) September 11th, 2009 @ 09:08 AM

    Thanks Tobie. I second you for points 1,2,3 above.

    Re: 2) curly braces can be omitted within if...else statements only when all statements are oneliners.

    Suggestion: 2) Curly braces must be omitted for if statements when the statement is written on one line. If...else statements must always use curly brackets.

  • ronin-24025 (at lighthouseapp)

    ronin-24025 (at lighthouseapp) September 11th, 2009 @ 09:31 AM

    Perhaps we're zooming in on readability and neglecting codeability. It's more organic to type (and edit):

    var div = document.createElement('div');
    var form = document.createElement('form');
    var isSupported = false;
    

    Than:

    var div         = document.createElement('div'),
        form        = document.createElement('form'),
        isSupported = false;
    

    Also compare the number of columns used as well.

    As with the ternary operator, var should not be spread over one line:

    // OK:
    var array = [], count = 0;
    
    // NOT OK:
    var array = [],
        count = 0;
    
  • Nick Stakenburg

    Nick Stakenburg September 11th, 2009 @ 01:42 PM

    @Tobie: I agree, defining it like that sets some clear boundries. A newline after a curly brace would probably only improve readability on some if...else if statements.

    I'll keep updating the above so we can work on that styleguide.

    Should the new (1.6) pattern be covered? I remember there was some discussion about how that had to look. If so, here are some more things that might be worth discussing.

    1) When to prefix function and variable names with '_'? I like this where anything kept private is prefixed except for shortcut variables:

    var Foo = (function() {
      var _cache = [],
          _slice = Array.prototype.slice,
          docEl = document.documentElement;
    
      function _private(x, i) { ... }  
    
      function exposed(x) {
        _cache.push(x);
        return _private(x, 1);
      }
    
      return {
        exposed: exposed
      };
    })();
    

    2) Define variables at the top followed by feature tests having their own closures. When creating a function based on browser features test for native functions first, if those aren't available use fallback functions ordered by speed:

    var Baz = (function() {
      var docEl = document.documentElement;  
    
      var FEATURE_IS_AVAILABLE = (function() {
        ...
      })();
    
      function barbaz() { ... }
     
      function fooUsingFeature() { ... }
      
      function fooFallback() { .. }
      
      // native first, fallback last
      var foo = document.nativeFunction ||
                FEATURE_IS_AVAILABLE ? fooUsingFeature : fooFallback;
    
      return {
        barbaz: bar,
        foo:    foo
      };
    })();
    

    3) toUpperCase or toLowerCase?

    @Joran: Readability seems of a higher priority then codeability, there's a lot of time, might as well make things readable. I don't see the added value of adding var where it's not required. Tobie's first snippet shows that you don't have to force curly braces on every if...else statement.

    How about this for var statements on one line:

    4) If a number of short descriptive var statements can make a oneliner you can keep them on one line.

    // OK:
    var array = [], count = 0;
    
    // NOT OK:
    var div = new Element('div'), long = very.long.variableName; <-- NO LONGER A ONELINER
    
    // Should be rewritten as:
    
    var div  = new Element('div'),
        long = very.long.variableName;
    
  • ronin-24025 (at lighthouseapp)

    ronin-24025 (at lighthouseapp) September 11th, 2009 @ 01:58 PM

    Thanks Nick, agreed:

    // OK:
    var array = [], count = 0;
    
    // NOT OK:
    var div = new Element('div'), long = very.long.variableName; <-- NO LONGER A ONELINER
    

    Further:

    // OK:
    var div = new Element('div');
    var long = very.long.variableName;
    
    // NOT OK:
    var div  = new Element('div'),
        long = very.long.variableName;
    

    Re: these last two examples: the former is faster to type and easier to edit than the latter. Moreover, just as readable. And in keeping with other similar style guidelines, e.g. not splitting the ternary operator over more than one line.

  • Nick Stakenburg

    Nick Stakenburg September 11th, 2009 @ 02:29 PM

    Joran: I don't agree with the var example. The only thing that's easier to type is you leave out one space, and that doesn't improve readability. The source seems to move towards comma seperated vars, old parts of the source have unnecessary var's, those should be avoided. A ternary operator is not placed at the beginning of a line for readability, I don't see how that relates to typing var where you don't have to.

    As far as the alignment goes I think that's better compared to how we make objects more readable with a few extra spaces. Makes sense to also do that on var statements for consistency.

    var div      = new Element('div'),
        verylong = very.long.variableName;
    
    // same alignment is used in situations like this
    return {
      div:      div,
      verylong: very.long.variableName
    };
    
  • ronin-24025 (at lighthouseapp)

    ronin-24025 (at lighthouseapp) September 11th, 2009 @ 03:09 PM

    Nick: Okay. Let's run with that:

    If we start indenting our 'var' assignments, then let's start indenting all assignments. If we start indenting our object definitions then let's start indenting our class method definitions (as these are also object definitions).

    Javascript is not the same language as Ruby. We can enjoy using curly brackets for if...else statements, and put semicolons within anonymous function blocks. Javascript is not the same language as Python. We don't need to feel guilty if we don't indent if...else statements or 'var' assignments. At the same time, we can extract the best of other languages and we're certainly doing this.

  • T.J. Crowder

    T.J. Crowder November 16th, 2009 @ 04:50 PM

    • Assigned user cleared.

    [responsible:none bulk edit command]

  • Radoslav Stankov

    Radoslav Stankov March 1st, 2010 @ 08:51 PM

    • Tag set to general, needs:discussion

    I'm also wondering if this is for the Documentation project ?

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป

The Prototype JavaScript library.

Shared Ticket Bins

Pages