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);
}
} |
