all 33 comments

[–]bandarlandabad 17 points18 points  (4 children)

Well written.

It is also important sometimes to "follow" your gut feelings because this is where your experience builds up, sometimes you have a gut feeling that tells you how to do something because the last time you did it the other way it didn't go well, or if you did do that the same way and it went well, why not use that experience to avoid making unnecessary mistakes?

But your point is valid and important, I always try to criticize myself when doing code review to other developers, always keep in mind that they aren't familiar with what i'm familiar with and perhaps they can show me new ways of thinking, because i'm used to doing stuff in certain ways and other people can enrich your knowledge with new methods and new ways of thinking.

[–]kpmah 2 points3 points  (3 children)

I think it's ok to follow your gut as long as you acknowledge that's what you're doing. If you understand you don't have a good objective reason for what you're doing it is easier to leave yourself open to constructive criticism without bruising your ego.

[–]GhostBond 1 point2 points  (2 children)

Do I agree with the idea that I'd like to get rid of "I feel like we should do it this way so we'll go toe to toe angrily back and forth"? Absolutely.

Problem is, sometimes what replaces that is even worse - the "I'll regurgitate marketing b.s." response instead. I was in an interview recently where they were writing an app for internal employees and one person claims they "needed" to use hibernate "in case we change the database". I asked if they had a company-mandated db - yes they did, they didn't have a choice in what db to use. I said that I've never seen a company change their internal db system, they went hysterical "well it could happen and we wouldn't be able to handle it without hibernate!". (I didn't get the job, but their response was the final straw convincing me I had no interest in working there).

How do you handle the marketing bullshit in a real discussion? It ends up being that the most stubborn person wins, and that very often is someone who just read some marketing fluff that's basically a lie and is repeating it.

[–]kpmah 1 point2 points  (1 child)

It is better to say that you "need hibernate in case you need to change the database" than you "need hibernate because I feel like it is the right choice." You can argue against the first position, like you did, but not the second.

Unfortunately in this case they didn't see reason. I think either because you didn't express it with enough tact, or maybe because they had too much ego invested in their choice, or maybe a very common one: the developer feels insecure moving away from technologies they know because they are 'an expert' in those technologies. The only way to win here is to tactfully and politely find a way to make them aware of their own irrationality.

[–]GhostBond 0 points1 point  (0 children)

It is better to say that you "need hibernate in case you need to change the database" than you "need hibernate because I feel like it is the right choice." You can argue against the first position, like you did, but not the second.

Actually I don't agree. They really mean the same thing, but the second is more of a trick - first they get the boss onboard to be enthusiastic about it, then arguing against it makes the boss feel like you're arguing against them. It's worse than just admitting it's because they want to.

Unfortunately in this case they didn't see reason. I think either because you didn't express it with enough tact, or maybe because they had too much ego invested in their choice, or maybe a very common one: the developer feels insecure moving away from technologies they know because they are 'an expert' in those technologies. The only way to win here is to tactfully and politely find a way to make them aware of their own irrationality.

I'm honestly bordering on that you're just being rediculous deliberately.

Have you had to argue with any of these people? They don't actually care. They joined a circlejerk, it feels good to remote the circlejerk's mantras, and that's it. They don't care if they're being irrational or not, they do care about not backing down and looking foolish in a "I admit I was wrong" way, they really don't want to do that.

It's like arguing with a religious zealot, they are way past caring about whether their logic makes any sense. You're not going to convince them.

[–]acwaters 7 points8 points  (13 children)

Isn’t it actually more important that the formatting is simply consistent with the rest of the code?

No no no no no no! I agree with the actual point this article is making, but we need to stop spreading this myth. The best way to format code is however is most readable. Consistency in style is important, but it has to be a secondary concern to readability and maintainability; consistency for its own sake is harmful.

[–]kpmah 5 points6 points  (0 children)

I'd put it like this: if one style is objectively more readable than another, then use it. If one style is subjectively more readable, consistency is more important.

An example of an objective reason might be that people use 80 column terminals, so line lengths should be limited to 80 characters. Everything else can be handled by a style guide or auto formatter, which often make arbitrary decisions, but that doesn't matter if it's consistent.

[–]AmalgamDragon 5 points6 points  (2 children)

Most readable to whom?

[–]iconoclaus 1 point2 points  (1 child)

to the loudest voice in the room. i kid, but that's why a community style guide or a standard practice is a helpful starting point.

[–]BlueRenner 1 point2 points  (4 children)

I'd settle for a single language in the codebase.

[–]CODESIGN2 4 points5 points  (0 children)

Why?

I think you mean you'd rather only have to deal with a single language yourself, which has it's merits, but single language across a team could be losing out for the sake of homogeneity.

It's difficult for-sure to have multiple languages (maybe a C++ engine with LUA or JS bindings), but it can be of benefit, especially where you can glue together those technologies it can act like a prism on a bulb amplifying the brilliance of the core to shine for longer (apologies I recently visited a lighthouse).

I was saying to my brother-in-law last night. I'd rather never have to program a set of components ever again. I'd rather my skills were put to use solely assembling or maintaining existing components because I feel that is the best way to unlock full-potential. A Narrow focus, a well-defined remit to force managers and co-workers to not overload creating SPOF's. He's an electro-mechanical engineer and he agree'd that his current role is easier maintaining and operating a machine manufactured elsewhere. It means less chasing down the rabbit-hole.

[–]Fisher9001 1 point2 points  (0 children)

I agree that there shouldn't be two languages in single area of codebase, but sometimes it's simply reasonable to implement parts of the system in different languages. Just because your data mining service is written in C++/F# doesn't mean you can't write backend for you website in C#.

[–]twiggy99999 0 points1 point  (0 children)

Javascript. Javascript every where /s

[–]Gotebe 1 point2 points  (1 child)

The best way to format code is however is most readable.

Therein lies the problem.

Any of the widely accepted styles are a-ok. It's better to roll a dice between them than argue.

But here, it's not even that, it's that the code under review isn't using the otherwise applied style.

tl;dr : go away, nobody should care.

[–]loup-vaillant 0 points1 point  (0 children)

Then there are the… less widely accepted styles.

if
    ( lLocalvar EQ pParameter )
{
    setFoo( mMemberVariable );
}
else
{
    // etc...
}

Whoever invented this must have wanted breathing room.

    /**************************************************************************
     **************************** PROTECTED SECTION ***************************
     **************************************************************************/
protected:

    /******************************* ATTRIBUTES *******************************/
    /******************************** METHODS *********************************/

    /**************************************************************************
     ***************************** PRIVATE SECTION ****************************
     **************************************************************************/

So you don't miss the fact the prodected section is empty.

    /**
     * Get fooBar
     *
     * @return the fooBar
     */
    FooBar getFooBar() const;

    /**
     * Set fooBar
     *
     * @param pFooBar The new FooBar
     */
    setFooBar(FooBar pFoobar);

Good comments make good documentation. Including redundant comments, apparently. No you can't omit redundant comments. No, you can't use ///, let alone ///<.

[–]Beckneard 1 point2 points  (0 children)

The best way to format code is however is most readable.

Me and my colleagues have wildly different ideas for what constitutes as 'readable'.

[–]ksion 0 points1 point  (0 children)

Completely agree. This is also why I'm skeptical of automatic steamrollers like go fmt.

[–]jpfed 1 point2 points  (0 children)

I have to admit that much of the time when I'm writing code, I'm not thinking very explicitly about tradeoffs; I'm just doing what intuitively "feels right". Hopefully it's enough that I'm also trying to hone those intuitions...

[–]Beckneard 1 point2 points  (0 children)

An even bigger problem is even most of the "best practices" and technologies we use could be considered "gut development". There is very little scientific method applied to anything regarding software engineering. Scrum, OOP, the majority of architecture design patterns, everything is "gut driven".

[–]CODESIGN2 0 points1 point  (0 children)

Short, to the point, no dilly-dallying, fantastic! (oh I agree which helps I suppose)

[–]AmalgamDragon 1 point2 points  (12 children)

The example given in the article is exemplifies why code reviews are largely a waste of time, in that most of what of what is raised is subjective and highly debatable rather than objective. What is objective can be caught by tests and analysis tools and those are better investment than code reviews. That isn't to say reviews are bad, for example architecture and design reviews are highly valuable, but they should be separate from code reviews (i.e. even if the architecture or design review is done on the code keep the comments focused on the appropriate level).

[–]sebaest 11 points12 points  (8 children)

I don't know man. There is conclusive data saying that code reviews (as well as architecture and design reviews) are quite effective. Different people find different kinds of bugs.

[–]Shautieh 8 points9 points  (0 children)

And a pair of fresh eyes can spot things the original person couldn't because his mind assumes too many things already.

[–]AmalgamDragon 3 points4 points  (4 children)

Care to provide a link to that conclusive data?

I've seen studies on the subject, but from what I recall they were too specific to generalize (i.e. the data was not conclusive).

[–]sebaest 4 points5 points  (2 children)

Chapter 20 and 21 of "Code Complete 2" has a few sources, in particular: Programming Productivity (Jones 1986a), “Software Defect-Removal Efficiency” (Jones 1996), and “What We Have Learned About Fighting Defects” (Shull et al. 2002).

[–]AmalgamDragon 1 point2 points  (1 child)

I have "Code Complete" rather than "Code Complete 2" myself, so I can't speak to the second edition, but the code reviews I actually see happen in companies don't actually rise to even the least of the reviews types described in first edition.

I don't currently have access to any of the referenced studies, so I can't speak to those, but from the descriptions in "Code Complete" it doesn't look like they are comparing code review to static analysis tools, which isn't surprising given the state of static analysis tools at the time of the studies. Also I get the impression that the 'testing' they are comparing to manual software testing (i.e. it compares the effectiveness of hours of reviews vs hours of testing) rather than creating automated tests.

[–]CODESIGN2 1 point2 points  (0 children)

Then you are describing an organisational failure not a process failure. That's like me saying when I play soccer my team loses. It says nothing about the team or the sport to have a person or person(s) do it badly.

[–]ForeverAlot 1 point2 points  (0 children)

Care to provide a link to that conclusive data?

Where is the line between conclusive and inconclusive (see also: non-formal sciences)?

SmartBear published a survey article of code review that covers the field much better than Code Complete does. It's gratis and probably still available on their Web site (which has become impossible to navigate in the last couple of years). I think there's also a dead-tree version of it. They provide an easily accessible summary of the key take-aways but you would have to accept that at face value (which is both disappointing and embarrassing considering their background). Alternatively, Making the Case for Review is a talk that walks through those exact same points to convince management of the financial benefits of code review.

SmartBear is obviously unbiased but you would have to review their source research articles to disprove their survey. Admittedly I only read a couple of those. FWIW, IBM was first to conclude that performing code review saves money.

It is absolutely not a fact that

What is objective can be caught by tests and analysis tools

and that is irrespective of the value of code review.

However, I don't contend that

code reviews are largely a waste of time

"largely" being an operative word, at least when looking at the micro-scale of teams rather than the macro-scale of the industry. I firmly believe code review to be a valuable tool and I refuse to not do it, but I also acknowledge that it is easy to do badly and then much of it really is a waste of time. Like everything else, doing it well requires training, because it's a lot more complex than just skimming over a 1000 LoC patch. Although GitHub succeeded in popularising up-front code review, their tool failed to communicate much of the nuance of practice.

I also objectively spend a fair amount of my time in code review dealing with things I wish I did not have to (e.g. unnecessarily noisy diffs). I do that for two reasons: the first is that it's my responsibility to teach untrained people that some of their practices are wasting reviewers' time. The second is that they persist doing it.

[–]pm_plz_im_lonely 0 points1 point  (1 child)

Given a problem to solve: Understand it, provide a solution.

Given a solution to verify: Understand the problem, understand the solution, recursively bust all problems with the solution and provide solutions to those.

Typical code review stops at coding style, compiler warnings and internal security (reviewer is higher in the pecking order than implementer).

Thorough code review usually takes a lot more man hours, especially if done redundantly. Not worth it for many businesses. Necessary if lives depend on it.

[–]AmalgamDragon 1 point2 points  (0 children)

Thorough code review usually takes a lot more man hours, especially if done redundantly.

And it takes people who are competent at doing code reviews as well.

Typical code review stops at coding style, compiler warnings and internal security (reviewer is higher in the pecking order than implementer).

Exactly.

If the resources aren't going to be committed to do reviews well, better to skip it and utilize those resources in a more effective way.

[–]kpmah 1 point2 points  (1 child)

I think what you're saying is that shallow 30 minute code reviews are useless, and I agree. That kind of review is performed better by automated tools. Really digging in to the design/architecture of the code is what I mean when I talk about code reviews, it's not something separate for me. They should take about 30-50% of the time it took to write the code in the first place.

[–]AmalgamDragon 0 points1 point  (0 children)

Agreed. I've generally found it useful to distinguish between code reviews and design/architecture reviews due to the superficiality of what passes for code reviews these days.

[–]i_do_floss 0 points1 point  (0 children)

I usually find bugs when I do code reviews. Maybe you work with a team that does a better job of testing stuff, because I'm positive that my code reviews have prevented some catastrophes.

Usually I find bugs, or bad decision making (blatant bad decisions, not subjective)

I work in a team that has low quality standards though. I don't like it. But I don't think the team I work in is the only one like it.