Hacker News Reader

Comment by raverbashing | Hacker News Reader

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.

View on HN

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

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.

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.

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.

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

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

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

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.

Reply on news.ycombinator.com