0
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ I'm not sure whether I'm reading this right, but are you asking whether or not you should use headers to indicate the different portions of your answer? \$\endgroup\$ – Mast Jul 18 '16 at 9:48
  • \$\begingroup\$ @Mast not quite. It's more about properly differentiating between "soft errors" like small convention breakers and "hard errors" like SRP violations or overly long methods \$\endgroup\$ – Vogel612 Jul 18 '16 at 9:56
  • 1
    \$\begingroup\$ Heya there I've edited your question a bit. It should read just the same words, but I tried to make it a little easier to skim and process. Feel free to roll the edits back if you disagree :) \$\endgroup\$ – Vogel612 Jul 18 '16 at 9:57
  • \$\begingroup\$ I guess I am trying to start a discussion on types of review material. as currently, like an application logger, there seems to be no standard/means of differentiation between 'severity' of review issues, all issues are provided as an answer with no indication of what really is important vs what is mostly opinion @Vogel612 exactly \$\endgroup\$ – apieceoffruit Jul 18 '16 at 9:57
  • \$\begingroup\$ When you say "design reviews" could it be that you actually mean "styling reviews"? \$\endgroup\$ – Vogel612 Jul 18 '16 at 10:01
  • \$\begingroup\$ I mean, reviewing quality of code VS code aesthetics. \$\endgroup\$ – apieceoffruit Jul 18 '16 at 10:02
  • \$\begingroup\$ What you call aesthetics are part of the quality of the code. This has been discussed on meta before. \$\endgroup\$ – Mast Jul 18 '16 at 10:07
  • 4
    \$\begingroup\$ You might be interested in Can we ask reviewers to not focus on something?. My suggestion would be: Make a note in the question that you are aware of the conventions and that you have deliberately chosen to follow your own style. \$\endgroup\$ – Simon Forsberg Jul 18 '16 at 12:58
6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I fully agree with this answer, couldn't have said it any better. \$\endgroup\$ – Mast Jul 18 '16 at 10:08
  • \$\begingroup\$ disagreements are fine :) , although I stated in the comment I expected to be challenged on the ternary, it was a rare occasion I felt was fine. but the point behind "always use brackets" is to avoid accidents when content falls out of condition, as the sole writer, of a sealed struct, writing a one like escape condition on the quality of incoming parameters, it is 100% a style choice. it causes no damage, no risk in incurred, it DOES make it clearer as it is an isolated statement which cuts down on visible cyclomatic complexity. There is no functional, structural or performance benefit. \$\endgroup\$ – apieceoffruit Jul 18 '16 at 10:25
  • \$\begingroup\$ @apieceoffruit "as the sole writer..." Will your programs die after you do? Or will your company hire more people? I think you're thinking too short term. ("no risk in incurred", unfortunately, we all make mistakes) \$\endgroup\$ – Peilonrayz Jul 18 '16 at 10:34
  • \$\begingroup\$ @JoeWallis You misunderstand me, I have handed over many projects in my life. I am not naive. I said as the sole writer of a SEALED STRUCT. there is interfaces to make additional alternates but this class is un-editable and un-extendable. It is not a changable type, it represents a value type, in no way has a need to change. it does not contain business logic. \$\endgroup\$ – apieceoffruit Jul 18 '16 at 11:58
1
\$\begingroup\$
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.

\$\endgroup\$
  • \$\begingroup\$ I'll accept the first point, resharper removed the spacing I had included. but to imply I should add character noise to my code to guard against consumer incompetence is lunacy. I write descriptive, clean, low cyclomatic complexity methods. If someone reads your code, with the intent of editing (and decided to ignore my suite of 100% code coverage unit tests) and EDITs it, without reading it, they deserve the life lesson, premature security is akin to a violation of KISS. can you offer a single benefit to those brackets aside from the risk of someone screwing up? is it any clearer? performant? \$\endgroup\$ – apieceoffruit Jul 18 '16 at 17:20
1
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ I 100% agree, hence my point. If every time Picasso painted; a review contained the sentiment "angles are too sharp, not realistic". it would be laborious and irritating, especially when the intent truly is to improve from critique. a craft is like a knife, useless if not sharpened, I have no intention of silencing criticism but effectively review 'noise' means the focus is not on things a questioner will listen to. no offense but I will ignore any comment questioning "style" not out of hubris, but out of personal comfort in that decision, one I didn't take lightly. \$\endgroup\$ – apieceoffruit Jul 18 '16 at 17:09
  • \$\begingroup\$ style guides are transient and opinion based, and something like "if you don't include braces in a one line statement AND someone edits your code AND they forget to add brackets...." is silly to me. I am altering my code not because of clarity reasons, not because of performance but to guard against a colleges incompetence. I follow 99.9% of standards and truly believe in the wisdom of crowds, I do not take such a decision without deep consideration. But I don't want to shy away from posting reviews for the fear or repetitious, unproductive re-iterations of an un-harmful design choice. \$\endgroup\$ – apieceoffruit Jul 18 '16 at 17:11
  • 1
    \$\begingroup\$ You can't put all of the blame on your colleague, if you didn't make reasonable efforts to make the code maintainable. From your colleague's point of view, you also contributed to the coding accident. We're trying to help promote best practices. You can ignore the advice, at your own peril. \$\endgroup\$ – 200_success Jul 18 '16 at 17:16
  • \$\begingroup\$ There in lies the problem (from my perspective) promoting best practices...for who? If the conceit is to design for maximum understanding for those with less experience and personal standards,...then we shouldn't ever use a large portion of advanced concepts like reflection and generics. tbh If writing well designed, well named code, with adherence to 90% of design principles,with a suite of complete unit tests is not reasonable enough, then i would like to know where you learned such standards and apply for a job. it is analogous to adding a "this is an int" comment above declarations \$\endgroup\$ – apieceoffruit Jul 18 '16 at 17:37

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .