code review madness
-
so I guess a typically micro managing manager that typically always adds tons of comment on reviews... I am particularly stricken (so far, only read 4 comments so far) by the contrast between 2 comments: - comment 1: add a private qualifier, normal comment. - whereas comment 2, line return after curly brace (instead of singe line"
{ a.Dispose(); }" is a mandatory task?! 🤔A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
I dunno. I'd need to see that single line curly brace in context before I get on my high horse and enter a religious war about code style... Who am I kidding! Of course I don't need context or even an excuse! Curly brackets can only be on a single line as part of a get/set pair of a property. Anything else is an affront to the Gods.
cheers Chris Maunder
-
I dunno. I'd need to see that single line curly brace in context before I get on my high horse and enter a religious war about code style... Who am I kidding! Of course I don't need context or even an excuse! Curly brackets can only be on a single line as part of a get/set pair of a property. Anything else is an affront to the Gods.
cheers Chris Maunder
I allow myself the occasional single line return:
if ( ok ) { return; }
"Before entering on an understanding, I have meditated for a long time, and have foreseen what might happen. It is not genius which reveals to me suddenly, secretly, what I have to say or to do in a circumstance unexpected by other people; it is reflection, it is meditation." - Napoleon I
-
so I guess a typically micro managing manager that typically always adds tons of comment on reviews... I am particularly stricken (so far, only read 4 comments so far) by the contrast between 2 comments: - comment 1: add a private qualifier, normal comment. - whereas comment 2, line return after curly brace (instead of singe line"
{ a.Dispose(); }" is a mandatory task?! 🤔A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
-
Replace it with
=> a.Dispose();
out of spite if it's a function. Especially if you know that function will need to be longer in the future :laugh:
it's a one short statement finally block the reviewer wanted to multiline. have to keep the curly brace for the finally. beside, dispose was an example, it's not dispose, otherwise I would use using ;P
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
-
so I guess a typically micro managing manager that typically always adds tons of comment on reviews... I am particularly stricken (so far, only read 4 comments so far) by the contrast between 2 comments: - comment 1: add a private qualifier, normal comment. - whereas comment 2, line return after curly brace (instead of singe line"
{ a.Dispose(); }" is a mandatory task?! 🤔A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
I am happy we have analyzers for our new code - now just need to get rid of all the old crap. Stuff like this never hits our code review for new code as the analyzers would flag it as a warning and help the developer fix it. And if it is checked in anyways the CI build will reject it as it is doing a release build set to treat warnings as errors. I am not saying we are doing perfect code reviews (spend too little time on it) but at least the time we do spend is on the naming, structure, and overall logic of the code - not wasting time on formatting that can be done by tools already available for free.
-
I am happy we have analyzers for our new code - now just need to get rid of all the old crap. Stuff like this never hits our code review for new code as the analyzers would flag it as a warning and help the developer fix it. And if it is checked in anyways the CI build will reject it as it is doing a release build set to treat warnings as errors. I am not saying we are doing perfect code reviews (spend too little time on it) but at least the time we do spend is on the naming, structure, and overall logic of the code - not wasting time on formatting that can be done by tools already available for free.
-
I am happy we have analyzers for our new code - now just need to get rid of all the old crap. Stuff like this never hits our code review for new code as the analyzers would flag it as a warning and help the developer fix it. And if it is checked in anyways the CI build will reject it as it is doing a release build set to treat warnings as errors. I am not saying we are doing perfect code reviews (spend too little time on it) but at least the time we do spend is on the naming, structure, and overall logic of the code - not wasting time on formatting that can be done by tools already available for free.
While I'm not convinced that level of OCD about code formatting is valuable, if done it definitely should be automated and not a human time sink. Edit: To include autoformat on save by the teams default editor.
Did you ever see history portrayed as an old man with a wise brow and pulseless heart, weighing all things in the balance of reason? Is not rather the genius of history like an eternal, imploring maiden, full of fire, with a burning heart and flaming soul, humanly warm and humanly beautiful? --Zachris Topelius
-
I allow myself the occasional single line return:
if ( ok ) { return; }
"Before entering on an understanding, I have meditated for a long time, and have foreseen what might happen. It is not genius which reveals to me suddenly, secretly, what I have to say or to do in a circumstance unexpected by other people; it is reflection, it is meditation." - Napoleon I
-
While I'm not convinced that level of OCD about code formatting is valuable, if done it definitely should be automated and not a human time sink. Edit: To include autoformat on save by the teams default editor.
Did you ever see history portrayed as an old man with a wise brow and pulseless heart, weighing all things in the balance of reason? Is not rather the genius of history like an eternal, imploring maiden, full of fire, with a burning heart and flaming soul, humanly warm and humanly beautiful? --Zachris Topelius
Maybe VS can apply corrections on save, but I would personally despise that so never gone looking for it. If needed I just bulk resolve them throughout the code as I am done with the "getting it working" part. And as the analyzers are simply nuget packages there are no requirements all team members have specific plugins or settings that needs to be coordinated - they do not even need to use the same IDE. And I must say after working on a code base with formatting kept consistent by analyzers it is nice. Code architecture is of course way more important... which is exactly why the formatting should be left to tools so reviewers are concentrating on the important things instead of just pointing out trivialities like newlines etc. Sure the tools can definitely still be improved... but for me they have passed the point where they contribute more than they get in the way.
-
If I'm feeling particularly daring, I'll even omit the curly brackets.
if (ok) return;
If you think 'goto' is evil, try writing an Assembly program without JMP.
No ... did consider it; but that leads to the else.
"Before entering on an understanding, I have meditated for a long time, and have foreseen what might happen. It is not genius which reveals to me suddenly, secretly, what I have to say or to do in a circumstance unexpected by other people; it is reflection, it is meditation." - Napoleon I
-
While I'm not convinced that level of OCD about code formatting is valuable, if done it definitely should be automated and not a human time sink. Edit: To include autoformat on save by the teams default editor.
Did you ever see history portrayed as an old man with a wise brow and pulseless heart, weighing all things in the balance of reason? Is not rather the genius of history like an eternal, imploring maiden, full of fire, with a burning heart and flaming soul, humanly warm and humanly beautiful? --Zachris Topelius
You seemed to have ignored the reader; and context ... as long as the formatting is automated, it's not OCD.
"Before entering on an understanding, I have meditated for a long time, and have foreseen what might happen. It is not genius which reveals to me suddenly, secretly, what I have to say or to do in a circumstance unexpected by other people; it is reflection, it is meditation." - Napoleon I
-
so I guess a typically micro managing manager that typically always adds tons of comment on reviews... I am particularly stricken (so far, only read 4 comments so far) by the contrast between 2 comments: - comment 1: add a private qualifier, normal comment. - whereas comment 2, line return after curly brace (instead of singe line"
{ a.Dispose(); }" is a mandatory task?! 🤔A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
Curious, for context. First, are the coding standards well-documented/understood and followed by everyone? Second, do you have "allowable exceptions"? Personally, you have coding standards for a reason. And they should be defaulted in a modern editor, and shared. I have worked on 20+ year old code. Even a bad coding standard beats a lack of any standard. Properly formatted (and consistently formatted) code is less error prone, and easier to read/spot bugs, from my experience. But exceptions should be allowed when the add to the readability/understanding of the code.
-
so I guess a typically micro managing manager that typically always adds tons of comment on reviews... I am particularly stricken (so far, only read 4 comments so far) by the contrast between 2 comments: - comment 1: add a private qualifier, normal comment. - whereas comment 2, line return after curly brace (instead of singe line"
{ a.Dispose(); }" is a mandatory task?! 🤔A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
Is your style guide that precise? Or is this just the manager's personal preference? Either way, it's a quick fix and is not at all reflective of your skill as a developer. When I get that kind of comment, I count it all good, because the reviewer had to try really hard to find things to fix before pushing my code.
-
Is your style guide that precise? Or is this just the manager's personal preference? Either way, it's a quick fix and is not at all reflective of your skill as a developer. When I get that kind of comment, I count it all good, because the reviewer had to try really hard to find things to fix before pushing my code.
Cpichols, very good point well made. IMHO, when it comes to formatting developers spend far too much time and effort (this thread is a good example) shouting at each other "I'm right and you're wrong!" If a reviewer raises a pedantic issue concerning formatting, just suck it up, fix it and move on. You probably have more important things that really are worth arguing about.
-
so I guess a typically micro managing manager that typically always adds tons of comment on reviews... I am particularly stricken (so far, only read 4 comments so far) by the contrast between 2 comments: - comment 1: add a private qualifier, normal comment. - whereas comment 2, line return after curly brace (instead of singe line"
{ a.Dispose(); }" is a mandatory task?! 🤔A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
Maybe I'm way off base on this, but when it comes to style I ask 2 questions: Does it affect the functionality? Both ways would compile exactly the same, so no. Does it make the code harder/easier to read and understand? Any dev confused by the curly braces or lack there of needs to find a new line of work.
-
so I guess a typically micro managing manager that typically always adds tons of comment on reviews... I am particularly stricken (so far, only read 4 comments so far) by the contrast between 2 comments: - comment 1: add a private qualifier, normal comment. - whereas comment 2, line return after curly brace (instead of singe line"
{ a.Dispose(); }" is a mandatory task?! 🤔A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
20 years developer here. One of the most annoying thing for me is lack of coding standard and inconsistency around the code. That just not make any sense for me, just make reading, understanding and following the code more difficult. When I'm coding I think every time "what if it's me reading this code in 2 years?". So - for me - bad formatting is very very annoying, because I want my brain to focus on the functionality and not to spend energy jumping around differents brackets style. As example I found very difficult to read a IF without brackets, or 1-liner (like your case). The worst case is when I need to read code written by the smart guy that have 12 logical steps in one line (like 200 chars long line, making calls to 2/3 functions) that I could split in 8 lines with good variable names explaining what's happening there... soooo easy to follow and understand, debug, etc. But wait... usually that code comes from the dev that have very low quality and a lot of bugs, as they never test/debug the code, because they think that "it works for sure". And it's pretty obvious they cannot debug their code, as it's a multi step singleline with a high cyclomatic complexity... that could be impossible to do! So in my experience I have associated bad coding styles to low-quality-developer. And as today I have never meet an exception to this my "personal" rule. So just stick to your company coding style, if doesn't exist, make one, otherwise you cannot complain. Many IDE have linter, editorconfig or similar. But to be honest, your manager is not "micro-managing" you, but just saying his opinion and giving you a feedback. That's the goal of a code review, is it? Usually developer have poor communication skills, this I think is a good example of it.
-
so I guess a typically micro managing manager that typically always adds tons of comment on reviews... I am particularly stricken (so far, only read 4 comments so far) by the contrast between 2 comments: - comment 1: add a private qualifier, normal comment. - whereas comment 2, line return after curly brace (instead of singe line"
{ a.Dispose(); }" is a mandatory task?! 🤔A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
Uniformity in code is simply for facilitating skimming through it. I think they call it K&R convention when you follow the function header with a newline and then a bracket '{'? Anyways I didn't before and I reformed. That and another convention about never going past 80 columns in any line so you never have to use the horizontal scroll bar (and something else, I think the ol' camel case thing) But to this day all my quick and dirty functions or variables start with my initials. I clean it up later to make it less personal (especially the really dirty names). But it almost seems religious for most C/C++ programmers that we continue to use as much short hand in our naming conventions as possible (nobody wants carpal tunnel syndrome, especially if you have to quickly debug in 'vi')
-
I am happy we have analyzers for our new code - now just need to get rid of all the old crap. Stuff like this never hits our code review for new code as the analyzers would flag it as a warning and help the developer fix it. And if it is checked in anyways the CI build will reject it as it is doing a release build set to treat warnings as errors. I am not saying we are doing perfect code reviews (spend too little time on it) but at least the time we do spend is on the naming, structure, and overall logic of the code - not wasting time on formatting that can be done by tools already available for free.