Alex Gwartney is new to programming in JavaScript and has started learning it by watching Team Tree House videos.

He has asked me to review his first project, a quiz. The code is available here:

Quiz Application

I will be regularly updating this post to show the process I go through while doing code reviews.

It is my belief that all developers in a team should produce code of equal quality. That is even the most junior developers should not be allowed to put code into production until it meets the same standards as those of the most senior developer.

This is tough for junior developers at first because there are a lot of things to learn. However, learning lots of things is how we improve and the whole team benefits immensely if all of the code meets the same professional standard.

When I was a junior developer I made a lot of mistakes and even now I still sometimes make mistakes. So any criticisms are meant constructively, and I agree with Alex that code reviews are an important way to learn better techniques and practices.

I have found that just a few key rules can have a massive impact on code quality

Rule Number 1: Always use strict mode

JavaScript was originally created to be a very forgiving language. But this willingness to forgive signals a green light for sloppy code. This is the reason why the the normal JavaScript mode has been given the nickname “sloppy mode”.

For details of how strict mode works see Mozilla Developer Network

After we turn on strict mode, the program stops working. Pressing F12 in chrome brings up the developer console and we see some problems:

In first of the for loops, we just need to add var before the i. We are using the same variable for the other for loops, so we need to consider whether reusing the variable for other counts will cause a problem.

In this situation, once we have used i for one for loop, it is safe to reset it to 0 again. So we in this case we don’t need to re-declare i for the subsequent for loops, but if we refactor later this may need to change.

Now let’s look at the statement the undefined a and b problem:

var done = [
a = new askquestion("question one","questiononeawnser","questionawnsertwo","correct","incorect"),
b = new askquestion("question two","questiononeawnser three","questionawnser four","incorect","correct"),
]


This is creating an array of askquestion objects. As part of this statement we are attempting to implicitly create two variables: a and b.

We are no longer allowed to do this. Now why do we need to create these variables? It turns out that we don’t.

We can remove
a =
and
b =

Rule Number 2: Meaningful variable names

We should also think about whether done is a good variable name. A good variable name describes what the object does. What does done mean?

There is another problem with this name. We are using jQuery, and we typically associate done with the deferred object. In a more complex project, this could potentially cause a lot of confusion for other code readers.

We can improve this a bit by changing it to:

var askQuestionArray = [
new askquestion("question one","questiononeawnser","questionawnsertwo","correct","incorect"),
new askquestion("question two","questiononeawnser three","questionawnser four","incorect","correct"),
]

We must update the rest of the function changing done to askQuestionArray.

Next we see another implicitly created variable:

count =0;

We can just change this to

var count = 0;

However, is count the best name? It suggests that the variable is a number, which is correct, but it doesn’t describe what we are counting.

To find this out we look at the for loop.

for(i=0; i<askQuestionArray.length; i++){
askQuestionArray[count].addquestion()
$("#guess0").click(function(){
count+=1;
askQuestionArray[count].addquestion()
});
$("#guess1").click(function(){
count+=1;
askQuestionArray[count].addquestion()
});
}

We see that it is counting the array elements inside askQuestionArray. But wait, we are already using another variable to do the same.

Rule Number 3: Remove any unnecessary variables

We can do the same thing with less code like so:

for(var i=0; i<askQuestionArray.length; i++){
askQuestionArray[i].addquestion()
$("#guess0").click(function(){
askQuestionArray[i].addquestion()
});
$("#guess1").click(function(){
askQuestionArray[i].addquestion()
});
}

Rule Number 4: Long variables should be in camel case, and spell checked

We can see variable names such as this: userawnserscorrect

The lack of capitalization makes it harder to read. Let’s change it to camel case:

userAwnsersCorrect

It’s now easier to see that we have a spelling mistake here. So let’s correct that:

userAnswersCorrect

Rule Number 5: Lint your code

Now that we have gotten rid of all the JavaScript, it’s time to look at the next level of quality, by using a linting tool.

Go to http://jshint.com/ and copy the code in here.

We get the following results:

Metrics

There are 7 functions in this file.
Function with the largest signature take 5 arguments, while the median is 0.
Largest function has 21 statements in it, while the median is 5.
The most complex function has a cyclomatic complexity value of 4 while the median is 2.
28 warnings
1 Use the function form of “use strict”.
6 Possible strict violation.
7 Possible strict violation.
8 Possible strict violation.
8 Missing semicolon.
9 Possible strict violation.
10 Possible strict violation.
18 Missing semicolon.
57 Missing semicolon.
70 Missing semicolon.
75 Missing semicolon.
80 Missing semicolon.
83 Missing semicolon.
85 Missing semicolon.
86 Don’t make functions within a loop.
88 Missing semicolon.
89 Don’t make functions within a loop.
35 ‘$’ is not defined.
39 ‘$’ is not defined.
44 ‘$’ is not defined.
48 ‘$’ is not defined.
57 ‘$’ is not defined.
62 ‘$’ is not defined.
70 ‘$’ is not defined.
84 ‘$’ is not defined.
87 ‘$’ is not defined.
One undefined variable
35 $
39 $
44 $
48 $
57 $
62 $
70 $
84 $
87 $
One unused variable
40 choice

The results of Lint tools should be assessed and considered carefully. You don’t always necessarily need to get rid of every warning that such a tool, comes back with.

What you do want to do however, is understand what it is telling you and at a minimum address any major issues.

The many warnings about $ are due to us not declaring the jquery object in this file. We won’t worry about those.

The easy things we can address are the unused variable, and the missing semicolons.

An interesting warning that shows above is don’t make functions within a loop

Rule Number 6: Always check your logic

The following code is logically incorrect:

$(“#progress”).html(” You got questions” + “” + point + “of” + wrong + “right”)

We want to count the number correct answers and the number of answered questions. Instead this displays the number of wrong answers next to the word “right”.

This suggests the developer has been hacking away rather than thinking about the design enough up front. We might also find other bugs as we continue our review.

At this point, I tend to step well back from “refactoring mode” and instead of looking at implementation details, I focus on reviewing the design of the application.

It sometimes helps to turn this code into pseudo-code so that the program design can be assessed instead of the implementation details. In Chapter 9 of the book Code Complete, Steve McConnell explains the benefits of writing pseudo code before writing the actual application code, describing the Pseudocode Programming Process as a useful tool for detailed design that makes coding easy.

The riskiest situation when doing a code review is when there is no specification available and no acceptance criteria to use either. In this case, the program is so simple and that we can course correct just by using common sense. However for a more complex the best action to take might be to fail the work on the grounds of an implementation being written without a prior design.

Rule 0: What is our program actually meant to do?

I call this rule zero because this is the question that needs to be asked first of all. What are we trying to achieve with the quiz? Let’s brainstorm:

Some features of a quiz might be:

• Interesting questions
• Functionality to make different answers
• A score counter
• A final screen at the end of the quiz

I have found that several other authors are also sharing their solutions. So let’s pick some good ones already available to us.

Nathaniel Nasarow has used these:

Question: Who is the largest producer of bicycles in Taiwan?
Options: Giant, Merida
Correct Answer: Giant

Question: Of the following people, who is a street photographer?
Options: Paul Gerone, John Free
Correct Answer: John Free

Ran Yehushua has used these:

Question: When did Pablo Escobar die?
Options: 1983, 1993
Correct Answer: 1993

Question: When did the US claim independence?
Options: 1492, 1776
Correct Answer: 1776

Question: When did Michael Jackson release “Thriller”?
Options: 1983, 1976
Correct Answer: 1983

Coming Soon

We will continue our review and refactor as we find further problems.

• Unit Testing
• Global Scope
• And More…