

Making code better with code reviews - jgrahamc
http://blog.cloudflare.com/making-code-better-with-reviews

======
aleem
Refactoring is fun, especially on a slow day. The "Introducing an Explaining
Variable"[1] refactoring is:

    
    
      if (version < 8 && ua ~ "IE") ...
    

is better written as

    
    
      IE8 = (version < 8 && ua ~ "IE");
      if (IE8) ...
    

This allows variable IE8 to be (1) reused later on in the code, (2) is self
documenting and (3) sets a precedent for extending it easily by future devs:

    
    
      IE8 = (version < 8 && ua ~= "IE");
      mobile = (ua ~= "mobile")
      if (IE8 || mobile) ...
      
    

Another quick one is Guard Clauses[2].

    
    
      function {
        if (...) {
          ...
        } else {
          ...
        }
      }
    

where either block is long, is better written as

    
    
      function {
        if (...) {
          return ...;
        }
        ...
      }
    

This site
[http://sourcemaking.com/refactoring](http://sourcemaking.com/refactoring) is
worth a read even if you have been programming for years; especially if you
have been programming for years and haven't developed good habits.

    
    
      [1] http://sourcemaking.com/refactoring/introduce-explaining-variable
      [2] http://sourcemaking.com/refactoring/replace-nested-conditional-with-guard-clauses

~~~
taway2012
I like functions that return a value to be written in the guard-clause style.
[In C++, that's probably better for efficiency since the compiler optimizer
doesn't have to remove temporaries by converting the code to guard-clause
style.]

However, I'm not convinced that void functions are better written in the
guard-clause style. I think the intent of the code is better expressed with
nested ifs. Of course, try to factor the ifs so that they're readable.

I'm saying this is more readable if the "do something part" is less than about
15 lines. Thoughts?

    
    
        void do_something(input) {
          if (!input.already_done()) {
            // do something.
          }
        }

------
raverbashing
The problem with code reviews is when it turns into a nitpicking festival
and/or an opinion discussion

Having a big blob of comments above the code, _to me_ is bad. It might have
been good, but I'm adverse to "ASCII art"

I think the change they did went further in the direction of explaining it
then the comment blob there

Also, doing things in the X or Y way, when it's a matter of
opinion/readability (readable/easy to understand code may not be the
cleanest/most concise)

And PLEASE don't do "consts everywhere", this:

total_seconds = hours * 3600; // seconds per hour

is better than

total_seconda = hours * SECONDS_PER_HOUR;

No, the number of seconds per hour is not going to change.

~~~
prezjordan
The number of seconds per hour probably won't change, but repeating `//
seconds per hour` as a comment everywhere you use the number 3600 seems a
little silly when you can just use a constant.

The next programmer doesn't need to see those digits at all, they're nothing
but a distraction.

~~~
lostcolony
Agreed. I much prefer the latter, as it tells me not only the value, but the
intention.

newTime = currentMillis + 21600000,

I go WTF is that number and is it right,

newTime = currentMillis + SIX_HOURS_IN_MILLIS

and I not only know at a glance what you're doing, I know what the value ought
to be, so if you accidentally added/left out a zero, I can actually check it
where that const is declared.

~~~
bluefinity
Or, in a more reusable/flexible way:

    
    
      newTime = currentMillis + 6 * HOUR_IN_MILLIS
    

But an even cleaner solution would be to just use the date/time library for
this (depending on the language, of course):

    
    
      newTime = currentMillis + TimeSpan.FromHours(6).Milliseconds;

~~~
danbruc
Why six hours? This definitely needs another constant.

------
chj
I find it odd that in the header illustration bit 0 is put on the left. It is
usually put on the right so that you can construct the bit mask easily. That
is the way we write numbers: most significant digits first on the left, least
significant digits on the right. The convention has been used in all bit
twiddling literature I have ever read. Being different isn't always a smart
thing.

~~~
jgrahamc
I agree. Unfortunately, this is a DNS server and the DNS RFCs are all like
this.

See [http://www.ietf.org/rfc/rfc1035.txt](http://www.ietf.org/rfc/rfc1035.txt)
Section 2.3.2

------
k__
They rip out everything that has been proven shitty and replace it with new
stuff. But how do they do it?

I have worked in a few companies and such behaviour almost never happened.

The management always wanted to maintain the old stuff and only resorted to a
rewrite, when things were really really bad. "We don't have the time/money"
was always the reply. But I had the impression, that a rewrite wasn't that
expensive, it always went faster than the first version, without many bugs the
first version had and a better structure.

~~~
taway2012
Been in a few orgs. I now think "no rewrites" is a good policy for all but a
few orgs. Few orgs have the (1) business luxury (time and money) or, (2) the
caliber of people and, (3) team motivation (few teams people even _want_ to
improve things) to pull off rewrites.

The best orgs do a rewrite usually in the face of sea changes to the
technology stack (e.g., containers vs. VMs) or business use (number of users
is now 10X of original etc). Bringing key infrastructure in-house is also a
decent reason, but the benefit of building expertise in an existing ecosystem
vs. full rewrite should be carefully weighed.

You probably have read spolsky's article. It's quite dated by now, but it's
still more right than wrong.
[http://www.joelonsoftware.com/articles/fog0000000069.html](http://www.joelonsoftware.com/articles/fog0000000069.html)

~~~
k__
This reads like they tried to replicate 100% of the features in a big
monolithic application, which is rather time consuming. :\

When I start a rewrite I check which are the essential features and try to
iterate my way to 100% (sometimes the 100% aren't even needed) with many small
releases, but every executive fears "rewrites" like the devil....

~~~
judk
Sadly, this often leads to a 50% rewrite where key features are missing
because I he rewrite team ran out of budget or willingness to code the non-fun
parts. Which drives users to misery or competitors.

~~~
k__
True story.

But I have the feeling, that a prio-list of the needed features is all that is
needed.

People say what they need, you implement it, done.

I mean the way other companies steal your customers IS that they implement a
better version of stuff you did. So it's either, they do the rewrite (in their
case a first write) or you.

------
nawitus
Hmm, I don't quite understand the point of this blog post. I would assume
everyone knows about code reviews, and this is just a very typical code
review.

~~~
eterm
Working somewhere that scores less than 6 on the Spolsky Test I'm very glad
for articles like this, it provides useful articles I can link to when I get
asked what the point of these things I'm introducing are.

------
mac1175
Why not good ol' De Morgan's laws?

return !(raw[2]&(QR|AA|TC) == 0 && raw[3]&(Z|RCODE) == 0 )

~~~
rpsw
The code probably goes on to to do additional checks.

