Making code better with code reviews | Hacker News Reader

Making code better with code reviews

blog.cloudflare.com

59 pointsjgrahamc46 comments

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 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

Those guard clauses is something that I've got more used to doing since using ReSharper as it is something it is fairly agressive in suggesting (and will do the refactoring for you).

It's not something I'd previously considered doing as it seems a bit unnatural at first, but reducing nesting does improve readability over dogmatically sticking to "single return point".

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.
      }
    }

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.

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.

I think you're making the point of the parent commenter, which is that getting a bunch of devs to agree on the 'best' way to write code ends up in a nitpicking session.

That's been my experience. I've yet to work on or with a team that has been able to resolve the concept of a consistent coding standard.

Personally, I think it's the "repeating" part that is important - if you use it once then have 3600 explained with a comment, if for some reason you use the same 3600 a lot in a chunk of code make it a sensibly named constant.

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.

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;

Why six hours? This definitely needs another constant.

This is exactly where the BS starts. So what are you doing next, FIVE_HOURS_IN_MILLIS? Should we do the whole range?

More importantly: Why 6 hours?

The comment by bluefinity gets it right. HOURS_IN_MILLISECONDS is ok, but don't "constant" everything.

Certainly. Though it may be that you only have a single offset due to whatever business rules, and you never use HOURS_IN_MILLISECONDS, it's always either 6*HOURS_IN_MILLISECONDS or some other term. I didn't want to have to think of an actual domain case, but just show an example of when having a clear, named constant rather than a, yes, unchanging, but ultimately not intuitive and easy to get wrong value, is a better choice; I readily admit you can run that down further once you have a real domain. :P

Smalltalk:

newTime := currentMillis + 6 hours asMilliseconds.

> 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

Yes, so if it's only one case you can have the comment

Now, if you're doing it a lot of times I bet your problem is bigger than just adding a constant, it may be even better to turn this into a function so you do to_seconds(time_in_hours) or something

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

And by using a constant you're making sure that it's not going to change accidentally (typo in one of 17 locations).

Which reminds me of this: GitHub search for 84600 seconds. https://news.ycombinator.com/item?id=7717414

Half an hour for lunch, mate!

Nitpicking should not be the point of a review, but would you consider it nitpicking to point out minor things that do not fall exactly in line with the current project standards? Naming is a big thing that comes to mind. Formatting is another.

Personally, when I review code I never look at it from an X or Y way, but from a current project way. I do not agree with how my current project does everything, but consistency is more important in a large code base until I can schedule time to change things I do not agree with across the entire code base.

To your specific case, someone who uses 3600 instead of making a constant probably has other magic numbers scattered about.

Also, remember the code that exists needs to serve as a guide for future programmers. The next programmer who comes in and sees 3600 now may think magic numbers are okay. So the minor thing you called a nitpick is picked up by someone else and made more of an issue, and so on.

I like opinion discussions when the opinion's are backed by reason. You may learn something. I have no problem with "ASCII Art" in this case.

Not when the discussion of a variable naming or something equally stupid is taking 3 days, and in 3 days you could get some features working and think about the name in the background and fix it later.

At the risk of nitpicking, seconds per hour do change, don't they? Leap seconds are a thing, though maybe that's not all that important :)

In that case, the constant name is incorrect, the literal number is correct.

The problem here is that most programmers confuse readability with familiarity and/or aesthetics. Readability has to be an objective measure, because it would be totally irrelevant if it was just a subjective impression, yet we still have no idea what readability really means and how to measure it. Not to mention it seems to be very context sensitive.

One nice write-up on readability I saw is this: http://www.perlmonks.org/?node_id=592616 along with sources cited therein. It sums up the problems with defining readability nicely.

The bottom line is that we can't say for sure if this:

    total_seconds = hours * 3600; // seconds per hour

is any better than this:

    total_seconds = hours * SECONDS_PER_HOUR;

or this:

    total_seconds = hours * 60 * 60;

or this:

    total_time = hours * 60 * 60; // in seconds

or this:

    total_time = TO_SECONDS( hours );

or this:

    total_time = SECONDS_FROM_HOURS( hours );

or... you get the idea.

And we not only don't know which is better in general, but even which would be better in most specific conditions. We just don't know. Saying otherwise is just an expression of opinion or personal preference. I think acknowledging this would be a decent first step towards improving the readability level of code in general.

True, amongst the examples given they all score more or less the same to me (and it's a short example)

A lot on readability stands on the expectations of the reader (and the idioms of the language) as well. For a lisp programmer (* (hours 3600)) may be easier to read for example.

Now I can take that example and turn into

ttscds = x * (36 * 10 * 10);

And we know this is less readable, the intentions are hidden and nobody unfamiliar with the code knows what's going on.

And all of them are better than

    total_seconds = hours * atoi(getenv("SECONDS_PER_HOUR"));

Because you know, we might want to change that value some day.

I've got a blog post coming up along these lines in the next few days.

Yes, please

And someone is going to criticize you saying SECONDS_PER_HOUR may be a float, so you should use atof

I disagree with your opinion on consts. You're right that SECONDS_PER_HOUR is not going to change. The advantage to using a const even for values that will never be updated is that you allow the compiler to find your typos. If I write

total_seconds = hours * 3500; // seconds per hour

the compiler will happily let me do this and I may or may not notice the error. If I write

total_seconds = hours * SECOND_PER_HOUR;

the compiler will complain and I'll fix the typo.

ASCII art? The engineer is describing a low level header format. The RFCs describing many, many low level protocols that deal with bitwise operations on file formats (gzip, http, tcp, etc) all contain plenty of 'ascii art'. Maybe they could have included a link to the RFC for DNS in the comment, but this seems very useful IMO.

// DNS Header

// raw[0:1] ID of the query

// raw[2]

// bit 0 - QR

// bit 1:4 - OPCODE

etc

Easier to write and possibly to read.

Yea definitely, this engineer should deviate from very standard looking easy to visualize format./s

Take a look at this RFC defining websockets written in 2011

http://tools.ietf.org/html/rfc6455#section-5.2

and the RFC for TCP from 1981 (check out section 3.1 describing the header)

http://www.ietf.org/rfc/rfc793.txt

See the similarity? We have over 30 years of consistency here.

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.

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

See http://www.ietf.org/rfc/rfc1035.txt Section 2.3.2

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.

It's all about maintenance cost. It's pretty easy to justify rewriting stuff because as we've grown the older code has been added to, wrapped in things, turned into APIs, all sorts of stuff.

Eventually, it's just better to start again.

For example, right now I'm rewriting something that is currently a PHP program that executes a Python program that runs one of three binaries or calls a REST API and posts results back to another API into a single program.

Just makes sense: easier for developers to maintain, better for SRE to manage when deployed.

So you think devs need to get better at convincing the management?

John doesn't have any trouble convincing me. But, generally, CloudFlare is still a place where the engineering team drives most the decisions. That comes with benefits as well as its own challenges.

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

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....

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.

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.

That's why having a full suite of tests is important so that we can fearlessly refactor, so that we can replace a part whose needs we have outgrown without breaking other parts of the system, allowing a "rewrite" to happen organically.

Far too often, code grows stale because nobody wants to work on it anymore. Every change is stressful because it could break something else, which means more time wading through unfamiliar and unpleasant code. So, the maintenance programmer patches it with silly putty and duct tape as to not bother any other portion of the system.

A few years of this and it becomes brittle.

If we have a decent suite of tests, however, we can have an application that doesn't need to be rewritten because the maintenance programmer can attack a problem with the confidence that they haven't broken anything else, and so can make the right solution rather than the least invasive solution.

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.

Me neither. The example also seems very contrived and trivial. And it's a bad example! It's fairly obvious that it's a good idea to remove magic numbers. But the improvement still left the magic numbers 0 and 1 to index the raw-array. These are documented in the huge comment block, which are rarely good idea. Indeed, Why not write code that does not require comments? Here they could access the byte array's fields with descriptive accessor functions.

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.

Why not good ol' De Morgan's laws?

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

The code probably goes on to to do additional checks.

Reply on news.ycombinator.com