Transcripted Summary
One of the main points here is there's a lot of power in observation. I want you to meet Andrew Gold. Andrew is just hanging out in his apartment. I want you to just look at the picture of Andrew and write down anything that looks out of the norm, or unusual, that you think doesn't fit. I want you to look, in other words, for issues in Andrew's apartment.
How many issues did you find? Did you find five issues? 10? 20? How many issues do you think are there in total? There are over 40 different issues with this picture. I'm just going to point out some of them.
For example, if you start to look at Andrew's windows, you'll see that this window is closed, but it looks like the curtain is blowing. Whereas, this window is open, and it's not. There's definitely something fishy around the windows.
Another thing is that Andrew's shoelace seems to be tied here, at the bottom, or close to his toes, as opposed to the top. Let's see if you can find a couple more.
Well, if you look closely, it looks like this chair doesn't have a leg, and it appears that this Les Paul guitar is plugged into the phone. And just by observing things like this, we can do a pretty good job in knowing whether, or not, something is correct, incorrect, if it will be a problem for other people.
The cool thing is that you don't need a lot of programming experience to look for interesting defects in other people's code.
Code reviews typically catch about 60% of defects, which is higher than most other defect detection techniques.
This information comes from Steve McConnell's book, Code Complete 2, where a number of industry studies, on different projects, were looked at and it was discovered that unit testing uncovered about 25%, on average, of defects, functional testing, 35%, integration testing, 45%, but design and code reviews were as high as 55 to 60% of defects found, on average.
This is very impressive that our power of observation, our reviews, can reveal so many things, and so many issues, that it can actually be more effective than some of the testing techniques that we have today. Let's really get into another example.
Typically, we might ask a question like, “What is wrong with this program?” But, I recommend, that a better way to look at it is, “How can I make it better?”
If you frame our question around how you can make it better, than you can start to use a lot of the different feelings, and observations, that come to make useful suggestions to the person who wrote the program.
public int foo(int a)
{
int a = 24;
int b = 25;
int c;
c = a << 2;
return c;
b = 24;
return 0;
}
Let's take a look at this particular function. First, we have that it's public. It returns an integer. The name of the function is foo, and it takes a single parameter a that's of type integer.
Right away, if we think about how we can make this better, something just seems very confusing about this function. It's not only because if you have no prior programming knowledge that this looks more like foreign language below the line here.
But, just already, in this case, where we start to look at the name of this function, foo, actually tells us nothing about the purpose of this function, and that's a problem.
Similarly, a and a lot of these variables that are declared within here, b and c, don't give us any indication as to what they're used for.
int a = 24;
int b = 25;
int c;
So, the first thing to note is, if it feels confusing, or looks confusing, then there might be something deeper than just the technical aspects of it. The code is not very self-documenting and self-explanatory. Therefore, that's a piece of feedback, initially, for the person writing this.
Another quick observation shows us that we know that we pass in these parameters to be able to use them later on in the function as part of the computation. And we gave the example earlier of the square root function, where we can say the square root of 16, so 16 comes in. But, in this case, we see that we send in something, and we call it a but on the first line, we also see some other declaration of a that seems more internal to the function, and it's being assigned 24.
The problem with this is that, when we refer to a within this body, like when we call a here...
c = a << 2;
...it's always going to refer to the most recent declaration of a. In other words, the scope of this particular variable is within this block.
Normally, if there were no other declarations of a, the parameter would be what was referred to. But, here, this declaration, actually blocks this declaration, so no matter what we pass in, as a here, a value 24, will always be used within this block. That just doesn't look right, even to a beginner.
Now, let's get into some deeper issues that might make this program a lot better.
We see that, first, we do a couple of these statements where we declare some variables, and some of them we define values for. Then, for c, we do some computation.
c = a << 2;
return c;
We don't have to get into the technicalities of this left shift operator (<<), but we'll immediately return this value, which is something new to us — that we now return the value of c. What this says is that, after we execute the function, we want to return a value and, here, we're going to return whatever value c contained. That, in essence, causes this function to cease. Because, remember, we sent some input in, and we compute a value, your ƒ(x)., and then we're done.
But there seems to be extra statements after this return, including another return.
return c;
b = 24;
return 0;
So, this section of the code is, actually, completely dead. Those are just two examples where we have unreachable code. We also have a blocking declaration and, in general, the sense that this program just feels even more confusing, because there's just no descriptive variable names, no comment, just nothing.
Now, there's several other issues with this code, so I challenge you to look at it, and see if you can find any other issues.
With that, code reviews and inspections can be driven by some simple questions that can help you to be more effective in giving feedback.
The first question is, does the routine have a clear, and singular, purpose? Every function should, really, only have one purpose and that should be clear either with the documentation of the routine, meaning that it's well-named. Your name should describe the purpose, or your variables, and so on, and so forth, should be reflective of their usage.
Is the routine correct and complete based on the requirements? As we started to look at that particular function, if we needed to test it, we needed to start asking about what was the intent of the program? Because it wasn't very clear just from the name. And, even if the name suggested something, we still can't forget that the developer wrote the name, and the name may not be indicative of the intent that the product owner, or that the user, really wants or need.
Are all the parameters and variables used, right? Is there code, for example, that might not be reachable, or code that is defined and overwritten? And, is there any extraneous or redundant statements in the program?
Resources
Quiz
The quiz for Chapter 2 can be found at the end of Section 2.3
