v0.8 Release – Improving the Coding Style for the OnScroll Patch

This post is concerning the patch for bug 459727. For this release I decided that I wanted to make some adjustments to my coding style to make it more consistent with the rest of the code. What do I mean by coding style? Well, things like variable names, indentation, new lines, commenting etc. The goal here is to improve code readability.

Braces

OK, first lets examine one of the small functions I’ve implemented in the hg_templates/pushlog.tmpl file:

function fillPage() {
  start = ($("div").attr("id")) - 10;
  var vHeight = 0;
  if (document.documentElement) {
      vHeight = document.documentElement.clientHeight;
  } else {
      vHeight = document.body.clientHeight
    }
 
  if (document.body.offsetHeight < vHeight) {
    for(var count = 0, offSetHeight = document.body.offsetHeight;
        offSetHeight < vHeight;
        count++, offSetHeight += 180);
 
    if(count > 0)
      loadEntries(count);
  }
}

Now, lets look at one of the functions that was already present beforehand:

function localizeDates()
{
  var dates = document.getElementsByClassName("date");
  for (var i=0; i<dates.length; i++) {
    dates[i].textContent = new Date(dates[i].textContent).toLocaleString();
  }
  document.getElementById("localize").style.display = 'none';
  return false;
}

As you can see there’s a bit of a difference in the way the braces are handled. In my function I always keep the braces on the same line but in the other function the brace for the function name is on a new line while the rest of the braces are on the same line. A very peculiar coding style, if you ask me. Then again this may be some sort of coding convention but I haven’t really seen it implemented anywhere else before. I didn’t even notice this subtle difference until recently, which is why I didn’t make this change earlier. So, I’m going to be consistent and use the already implemented way of handling braces.

Variable Names

There doesn’t seem to be any type of convention being used, as far as I can tell, regarding variable names. The key is to just use short but logical variable names to increase readability. I’ve decided to change some of my variable names being used:

Old Name New Name Reasoning
trScroll row This is the actual row that gets added to the end of the table so it’s clearer to give it the name row. Nice and simple
pushCheckins req Since it is an xmlHttpRequest object I’ll give it the name req, pretty conventional
tdScrollUser userCol Since the variable is a column that will hold user information it will be appropriately called userCol
tdScrollChangeset revCol Since the variable is a column that will hold changeset information it will be appropriately called revCol
tdScrollAuthorDesc authDescCol This column will contain both the author name and the description and thus an appropriate name is chosen
trScrollMerge mergeRow Since it is a row that contains a merge push the new name seems more logical
tdScroll_MergeUser mergeUserCol Same reasoning as the userCol change above
tdScroll_MergeC mergeRevCol Same reasoning as the revCol change above
tdScroll_MergeAuthorDesc mergeAuthDescCol Same reasoning as the authDescCol change above
createRevLink(str) createRevTd(rev) This change was recommended by Justin Wood (Callek). The reasoning is that the attribute should indicate what to pass in, and the function name should indicate what it returns
mergeC mergeRev mergeC doesn’t make much sense. What does C stand for? Using mergeRev is definitely more logical


Spacing

One thing that I’ve always found a bit confusing is why or how to space out your code within a function or a method. Some people prefer to put in no spaces while others fall in love with them. Is there some sort of convention for this? Maybe there is but I’m not aware of any. Currently, with this patch’s code I’ve gone space-happy. There’s too much spacing in some places and it doesn’t look good. I’m going to be reducing the spacing wherever possible.

Variable Declarations

One thing I noticed was that I wasn’t putting all the variable declarations together, one after another. That made my code look very untidy. I’ve rectified that situation and now all the declarations at the very start of a code block.

Most of the changes have occurred with loadEntries(). The following is what my code now looks like:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
function loadEntries(iterations) 
{
  for(var t = 0; t < iterations; t++) {
    var loader = document.getElementById("loader");
    var end = start;
    var req = new XMLHttpRequest();
 
    loader.innerHTML = '<img src="{url}static/ajax-loader.gif" align="right" />';
    start = start - 20;
    req.open('GET', '/json-pushes?startID=' + start + '&endID=' + end, true);
    req.onreadystatechange = function() {
      if(req.readyState == 4) {
        if(req.status != 404) {
          var pushData = JSON.parse(req.responseText), counter = 0;
          //var counter = 0;
          for(var i = end; i > start; i--) {
            var row = document.createElement("tr");
            if(counter == 0) {
              row.className = "parity0"; 
              counter = 1;
            } else {
                row.className = "parity1";
                counter = 0;
            }  
            var userCol = document.createElement("td");
            var revCol = createRevTd(pushData[i].individualChangeset);
            var bugLink = createBuglink(pushData[i].desc);
            var authDescCol = document.createElement("td");
 
            userCol.width = "184px";
            userCol.innerHTML += pushData[i].user + '<br />' + pushData[i].formattedDate;
            authDescCol.innerHTML += '<strong>' + pushData[i].author + ' &mdash ' + bugLink + '</strong>';
            row.appendChild(userCol);
            row.appendChild(revCol);
            row.appendChild(authDescCol);
            loader.innerHTML = "";
            document.getElementById("titlePush").appendChild(row);
 
            //Check whether it is a merge changeset or not
            if(pushData[i].MergeData != []) {
              for(var j = 0; j < pushData[i].mergeData.length; j++) {
                if(pushData[i].mergeData[j] != "") {
                  var mergeStr = pushData[i].mergeData[j];
                  for(var k = 0; k < pushData[i].mergeData[j].length; k++) {
                    if(pushData[i].mergeData[j] instanceof Array) {
                      var actualMergeStr = mergeStr[k].split('|-|');
                      var mergeRev = actualMergeStr[0];
                      var mergeUser = actualMergeStr[1];
                      var mergeDesc = actualMergeStr[2];
                    }
                    else {
                      var actualMergeStr = mergeStr.split('|-|');
                      var mergeRev = actualMergeStr[0];
                      var mergeUser = actualMergeStr[1];
                      var mergeDesc = actualMergeStr[2];
                      k = pushData[i].mergeData[j].length;
                    }
                    if(mergeDesc != pushData[i].desc) {
                      var mergeRow = document.createElement("tr");
                      var mergeUserCol = document.createElement("td");
                      var mergeRevCol = createRevTd(mergeRev);
                      var merge_bugLink = createBuglink(mergeDesc);
                      var mergeAuthDescCol = document.createElement("td");
 
                      mergeRow.className = row.className;
                      mergeUserCol.width = "184px";
                      mergeAuthDescCol.innerHTML += '<strong>' + mergeUser + ' &mdash ' + merge_bugLink + '</strong>';
                      mergeRow.appendChild(mergeUserCol);
                      mergeRow.appendChild(mergeRevCol);
                      mergeRow.appendChild(mergeAuthDescCol);
                      document.getElementById("titlePush").appendChild(mergeRow);
                    }
                  }
                }
              }
            }
          }    
        }  
      }
    }
    req.send(null);
  }
}
This entry was posted in DPS911, Mercurial Project, Open Source and tagged , . Bookmark the permalink.

4 Responses to v0.8 Release – Improving the Coding Style for the OnScroll Patch

  1. When I’m coding in PHP, I use curly braces on their own line, because it makes it easy to see where blocks begin and end. When I hack on Bespin nowadays (in JavaScript), I always have trouble figuring out where the blocks begin, because they keep the curly braces in the same line as other code. This also causes frustration when having to copy, move, or delete a certain block of code, because you can’t just select certain lines—you have to select certain parts of the beginning and/or end lines.

    As for spaces, I prefer no space between function names and opening parentheses, but spaces between parentheses and arguments. The same goes for other parentheses—spaces on the inside side of them. As for line breaks, I’m a lot freer with them than the Bespin code seems to be. I like to separate things logically so it’s not all bunched together.

    (And, yes, I’m aware you’re not talking about Bespin. It’s just the only difference in coding styles that I can relate to recently.)

  2. Sid says:

    Hi Gordon,

    Until recently I was putting the curly braces on a new line whenever I was writing new code. However, I’ve noticed that not many projects follow this style. They prefer to put the curly braces on the same line. I’ve always found it easier to read the code if the braces are on a new line so I don’t understand why most people prefer to the other convention (braces on the same line).

    As for line breaks, I’m with you. I don’t like to bunch everything up together. I prefer gaps, which increases readability, at least for me.

  3. Justin Wood (Callek) says:

    for the |var| inside the if’s etc. I’d actually recommend putting var at the start of the function and leave the values undefined until needed.

    The point is that var is scoped to the whole function, and as such that once you define it, the entire function (not just that block) can use it.

    Alternatively use |let| at the highest scope you need to (in this case, probably the for loop)

  4. The coding convention here is “what Ted does”. ;-) I’m not sure why I do things that way, it’s just the style I’ve fallen into. I like having function braces on their own lines, as I think named top-level functions are important. In general I’m not terribly picky, as long as we’re consistent within a file.

Leave a Reply to Sid Cancel reply

Your email address will not be published. Required fields are marked *


*

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>