Reviewing Inexperience vs Reviewing Style

Question

I just recently Posted a Review Question and got some fantastic feedback. Exactly the point of this site.

Although IMO I processed 3 separate 'types' of review points.

1. You Missed X, (In my case an accidental public set on an immutable type)
2. a better approach might be (in my case, pre-computing some return results. )
3. Style choices, In my case omitting single-line braces/screaming cap consts

Now, I am not saying design reviews are bad. it is a discussion point and all sides should be heard, leads to a better overall understanding of design for everyone involved,but....

I like to consider myself somewhat experienced. I have quite a lot of years of practical development under my belt and for one reason or another I have settled on a design choice that does not always 100% match the document standards, as does everyone.

And even though no hard is intended it starts to become frustrating, bordering on condescending when you are repeatedly corrected on something you choose to do. When your decision is 'wrong'.

I think reviewing usability issues, ways to better adhere to design principles, code clarity issues in general great.

Example:

I chose a nested ternary. In fact the only one In my entire programming career. Given the implicit correlation between Start and End conceptually I believe

public static int AsClamped(int index, int start, int end)
{
    return index < start ? start 
         : index > end ? end 
         : index;
}

does not need to be split into a separate if statement, as I would do in 90% of similar cases.
Now, that is an unpopular choice and I expected to be challenged on it…

but lets say, when I have a Fail-Early condition, invalid arguments, incompatible configuration, e.g a one line if(cantGoAnyFurther()) scenario I always choose to forgo the brackets. it is unneccessary weight to what is, by design a quick, initial contract exit condition e.g

 if (percent <= 0 || percent > 100)
    throw new ArgumentOutOfRangeException("P...");

or

  if (!items.HasRange())
     return Enumerable.Empty<T>();

Clearly, those are design choices. Not mistakes, not even some major slap in the face of convention. I simply disagree, again same with screaming caps.

My Point

We should differentiate review points between

• "you seem to be lacking an understanding in X"
• "you might have missed Y"
• "There is an X out there that simplifies...."

And the case of

• "The style choices you display do not meet convention".

This is not a personal rant, I honestly think elements of reviews of something clearly not egregious or of potentially a chosen style or opinion should be pointed out with a proviso, something along the lines of.

"regarding a few style choice. the prevailing standard is:"

• ...

• ...

A marked differentiation for newcomers between what is actually wrong vs what is different.

Thoughts?

EDIT

It seems the innocuous example I chose is surprisingly divisive. (not using single line brackets)

So while we hash that point out in the comments I do want to point out the question (I hope) is still a valid one.

Should there be a precedent set for separating stylistic choices that do not improve efficiency, adherence to solid/other design principles; and those that belong in a style guide?

Answer 1

I don't agree with you.

The ternary in a ternary is hard to read, and your option to miss brackets can and does lead to bugs. Both probably are in style guides, but both are definite problems with your program. I don't know much C#, I definitely don't know its style guide, but I'd definitely agree with Heslacher.

Things such as use TitleCase rather than mixedCase or snake_case are style only concerns, and are there to adhere to a standard. You may be using a different style guide, and so it's understandable if you don't follow these. And so putting these on their own, 'you may want to do x, y, z' could be good, but not mandatory.

But dismissing actual problems with your code as 'just style concerns' as they may be in a style guide is bad.

---

_{Edit in response to OPs edit.}

Should there be a precedent set for separating stylistic choices that do not improve efficiency, adherence to solid/other design principles; and those that belong in a style guide?

First off, style guides are there to improve efficiency. Not code efficiency, but how efficient people can read or edit your code. Take:

• Indentation,
• Tabs or spaces,
• Maximum line length,
• Blank lines,
• String quotes,
• Whitespace in expressions,
• Naming conventions, etc.

These can be ignored, and could be good in a 'you may want to do this, but you may have a style guide that goes against most of the other people using your language', comment/section. If you were to put in your question that you're using your own style guide, one for say work, then it'd make sense for answerers to not say to change your style guide. But again it's ultimately up to the reviewer.
So no, there should not be a precedent set.

The reason your examples have sparked debate:

It seems the innocuous example I chose is surprisingly divisive. (not using single line brackets)

This is as they aren't purely stylistic. If I were to get into an internet flame war my argument for TitleCase over snake_case is only going to be 'follow the style guide'. Just like all the other examples I gave above.

If we were to get into an argument about the examples you gave, the argument would be little to no 'follow the style guide', but more what you're doing has problems.

Answer 2

public static int AsClamped(int index, int start, int end)
{
    return index < start ? start 
         : index > end ? end 
         : index;
}

This isn't what appeared in your question. There you had

public static int AsClamped(int index, int start, int end)
{
    return index < start ? start : index > end ? end : index;
}

Which is questionable not just as a nested ternary but as being overly dense and difficult to parse.

but lets say, when I have a Fail-Early condition, invalid arguments, incompatible configuration, e.g a one line if(cantGoAnyFurther()) scenario I always choose to forgo the brackets. it is unneccessary weight to what is, by design a quick, initial contract exit condition e.g

  if (!items.HasRange())
     return Enumerable.Empty<T>();

OK. What happens if I then decide that this situation has to be logged?

  if (!items.HasRange())
     mylog(MY_LOG_DEBUG, "No range on items.");
     return Enumerable.Empty<T>();

Why did my occasionally problematic code suddenly stop returning results ever? OOPS! I forgot I wasn't writing python code. I hope that I had good unit tests and/or quality assurance (QA) and caught this before deployment.

Another issue that arises is that you say you know when you can and cannot use this pattern. I'm not sure that I agree (see my counter example above), but what if I concede the point. You've now published your code on the internet for anyone to see. Shouldn't someone point out that the pattern has weaknesses? Because not everyone is going to have the same knowledge of the weaknesses of this pattern.

I suppose that you could explicitly comment this kind of thing

 // only using single statement form here 
 // because this is a fatal condition 
 // and we should never have more than a single statement response
 if (percent <= 0 || percent > 100)
    throw new ArgumentOutOfRangeException("P...");

But at that point, wouldn't it be easier to just write

 if (percent <= 0 || percent > 100) {
    throw new ArgumentOutOfRangeException("P...");
 }

Which is what people are recommending.

This pattern will never not work. It doesn't require additional understanding of the problem to select when not to use it. It adds a trivial amount of overhead to producing the statement but saves a lot of careful thinking. It's the kind of thing that a complete novice can learn easily. A good solid bright line rule.

It is possible to justify alternatives. But if you're going to do that in posted code, then you need to justify using that alternative each and every time. Not just by thinking about it, but by posting a notice in either the code or the accompanying explanation. That way innocent bystanders won't see this and start using this pattern in places where an alternative is not justified.

Or just comply with the simple rule.

Answer 3

You post your code on this site, and you get feedback. Just like anything else you read on the Internet, some of the feedback will be valuable, and some of it, frankly, will be garbage. For that matter, the same caveat would apply if you asked a friend or colleague to review your code.

To some extent, each of us has a personal style. You couldn't possibly convince Mozart to write music like Chopin, even though they are each good in their own way. In the end, you maintain the code, so you make the final decision as to what advice to accept and what advice to ignore. I say "ignore" instead of "reject", because it's probably not appropriate to argue with your reviewer over non-technical issues.

Still, it's important to keep an open mind. One advantage of Code Review is that lots of people can voice their opinion through answers and votes. If an answer appears to be supported by public opinion, it's probably a good idea to consider the advice more seriously. You wouldn't want people to be secretly hating your code without telling you, right?