Code Reviews
-
MarkTJohnson wrote:
But then, I'm old.
Me too! Which is probably why I get landed with working with archaic languages. :(
MarkTJohnson wrote:
Since both formats are used within the code base the person who bounced it was just wrong. Code reviews should be about functionality not form.
Yep. If this had been about something not working, (or if it was important enough to be documented as a standard), I would have changed it in a jif!
5teveH wrote:
I would have changed it in a jif!
I do like peanut butter[^], but I'm a little unsure what to think of this. :confused:
GOTOs are a bit like wire coat hangers: they tend to breed in the darkness, such that where there once were few, eventually there are many, and the program's architecture collapses beneath them. (Fran Poretto)
-
I just had a line of my code bounced for 'incorrect' syntax. It was a simple if/then - e.g.
if {condition} then {do something}
Which I had coded, as above, on a single line. The Code Reviewer said it should be on 3 lines. Both syntaxes are allowed in the language I am coding in, so I checked the Coding Standards document - and there's no mention of a preferred if/then syntax. Also, looking at the code-base, both syntaxes are used throughout. My view is: if they can't be bothered to document something as a standard, I, as a developer, am free to choose the syntax I prefer. I stood my ground, (and won out), not because I didn't want to spend 2 minutes changing the code, but because I value properly documented standards. What do you think guys?
I think that the three-line version is easier to grok as a reader of the code, especially if you weren't the original author. In a lot of languages, the single-line version is a good starter for a bug: ```javascript if (condition) doThing(); ``` becomes ```javascript if (condition) doThing(); ``` and then someone adds one line, forgetting about the lack of braces ```javascript if (condition) doThing(); doTheOtherThing(); ``` and then `doTheOtherThing();` is _always_ invoked, which is precisely why most linters will recommend that you rather did: ```javascript if (condition) { doThing(); } ``` the first time. I understand your frustration, but remember that code reviews shouldn't be a place for "standing ground" or duking stuff out. They should be collaborative. So I'd recommend the following: 1. Ask for the "why" of the comment. If the "why" has value, even if it's not immediately apparent to you, but it's important enough to a team-member, and it doesn't break stuff, then try to accommodate 2. Update the coding standards doc (whichever way the team agrees is the accepted way) -- it sounds like it's still in the same place, so this sounds like a typical lawyer's argument where one has to refer to precedents rather than simply the letter of the law. It makes it harder for your next new team-member to collaborate 3. Prefer uniform rules, but even if you all decide that the three-line version has merit, you don't have to go back and fix the entire code-base. As people move through files to update for whatever reason, _then_ they can fix style issues. This is how we've addressed a number of style issues in our code-bases: discuss, agree, document, fix-up when you're in the area. For example, `this` in C# has been deemed as "noise", so whenever we work in a file which uses `this` unnecessarily (it may be required for an extension method, for example), then we clean up that file. Rider / R# makes it easy (`alt-enter, enter` is often enough). Similarly we can fix-up JS stuff via WebStorm intentions. /2c, ymmv
------------------------------------------------ If you say that getting the money is the most important thing You will spend your life completely wasting your time You will be doing things you don't like doing In order to go on living That is, to go on doing things you don't like doing Which is stupid. - Alan Watts https://www.youtube.com/watch?v=-gXTZM\_uPMY
-
braces and comments reduce performance :-\
Real programmers use butterflies
-
I just had a line of my code bounced for 'incorrect' syntax. It was a simple if/then - e.g.
if {condition} then {do something}
Which I had coded, as above, on a single line. The Code Reviewer said it should be on 3 lines. Both syntaxes are allowed in the language I am coding in, so I checked the Coding Standards document - and there's no mention of a preferred if/then syntax. Also, looking at the code-base, both syntaxes are used throughout. My view is: if they can't be bothered to document something as a standard, I, as a developer, am free to choose the syntax I prefer. I stood my ground, (and won out), not because I didn't want to spend 2 minutes changing the code, but because I value properly documented standards. What do you think guys?
If they have a standards document, then they should use it. If they don't like the standards, instead of enforcing arbitrary new standards, then they should change the standards document. Either that or just ignore the documentation completely like all of the users do with help files. Sounds like you stood up for your principles and won. Yeah! Bond Keep all things a simple as possible, but no simpler. -said someone, somewhere
-
I just had a line of my code bounced for 'incorrect' syntax. It was a simple if/then - e.g.
if {condition} then {do something}
Which I had coded, as above, on a single line. The Code Reviewer said it should be on 3 lines. Both syntaxes are allowed in the language I am coding in, so I checked the Coding Standards document - and there's no mention of a preferred if/then syntax. Also, looking at the code-base, both syntaxes are used throughout. My view is: if they can't be bothered to document something as a standard, I, as a developer, am free to choose the syntax I prefer. I stood my ground, (and won out), not because I didn't want to spend 2 minutes changing the code, but because I value properly documented standards. What do you think guys?
If there's nothing in the standards, then (technically) you're fine. Having said that, I get from your description of your reviewer that they're going to find *something* to ding you with, no matter what. Because that's just who they are... Reminds me of the time I was pulled up for my incorrect use of apostrophes in code comments... Said reviewer was just *that* sort of person - "nothing wrong with the code... ***I'll find something to complain about!!!***". I found it quicker and easier to just change my apostrophes and then refuse that reviewer in the future, as they were obviously a pedantic dickhead of the wrong sort. And I say that as someone who can have an elite level of pedantry when I choose...
Java, Basic, who cares - it's all a bunch of tree-hugging hippy cr*p
-
braces and comments reduce performance :-\
Real programmers use butterflies
-
braces and comments reduce performance :-\
Real programmers use butterflies
-
-
I just had a line of my code bounced for 'incorrect' syntax. It was a simple if/then - e.g.
if {condition} then {do something}
Which I had coded, as above, on a single line. The Code Reviewer said it should be on 3 lines. Both syntaxes are allowed in the language I am coding in, so I checked the Coding Standards document - and there's no mention of a preferred if/then syntax. Also, looking at the code-base, both syntaxes are used throughout. My view is: if they can't be bothered to document something as a standard, I, as a developer, am free to choose the syntax I prefer. I stood my ground, (and won out), not because I didn't want to spend 2 minutes changing the code, but because I value properly documented standards. What do you think guys?
I think that this is something automatic tooling should take care of and it's a waste of time to discuss in a code review. If there's no good evidence to suggest why it should be three lines (literature that shows a good point, convention in the code base, etc) and it's not documented then it has no place in the review. To me it sounds like the purpose of the code review is what needs to be talked about.
Elephant elephant elephant, sunshine sunshine sunshine
-
I just had a line of my code bounced for 'incorrect' syntax. It was a simple if/then - e.g.
if {condition} then {do something}
Which I had coded, as above, on a single line. The Code Reviewer said it should be on 3 lines. Both syntaxes are allowed in the language I am coding in, so I checked the Coding Standards document - and there's no mention of a preferred if/then syntax. Also, looking at the code-base, both syntaxes are used throughout. My view is: if they can't be bothered to document something as a standard, I, as a developer, am free to choose the syntax I prefer. I stood my ground, (and won out), not because I didn't want to spend 2 minutes changing the code, but because I value properly documented standards. What do you think guys?
Hear, hear!
-
Only of the build.
- I would love to change the world, but they won’t give me the source code.
:-\ ;P
Real programmers use butterflies
-
I'm happy with
if (parameter == null) return;
and so on, but anything more complex than "return" or "throw" I put in brackets over three lines:
if (parameter == null)
{
...
}The only time I will omit the brackets is for a very short instruction on the same line as it's test.
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!
-
braces and comments reduce performance :-\
Real programmers use butterflies
-
I do brackets because, sooner or later, I'll be adding something else in there.
We won't sit down. We won't shut up. We won't go quietly away. YouTube, and My Mu[sic], Films and Windows Programs, etc. and FB
-
I just had a line of my code bounced for 'incorrect' syntax. It was a simple if/then - e.g.
if {condition} then {do something}
Which I had coded, as above, on a single line. The Code Reviewer said it should be on 3 lines. Both syntaxes are allowed in the language I am coding in, so I checked the Coding Standards document - and there's no mention of a preferred if/then syntax. Also, looking at the code-base, both syntaxes are used throughout. My view is: if they can't be bothered to document something as a standard, I, as a developer, am free to choose the syntax I prefer. I stood my ground, (and won out), not because I didn't want to spend 2 minutes changing the code, but because I value properly documented standards. What do you think guys?
Absolutely I agree - if its not in the coding standard and it's a matter of style (some practices are objectively bad, and should be avoided, regardless of whether it is captured in a coding standard) - then the coder should be free to use the style they prefer. If it were C++, then your style is OK by me. I've been doing C++ for 30 years, and precisely one occasion have I had a bad merge that placing the extra braces would have averted. And that merge error was picked up in regression tests. I currently work in an environment that has a large, complex coding standard (mildly adapted from the Google one). Apart from the small number of things I disagree with (eg total ban on using exceptions), it has a vast number of idiosyncratic "style stuff" that is impossible to remember, particular when working across multiple code bases that each have their own styles. My view is: mandate a coding style that can be checked automatically (eg by clang-tidy), and don't include anything else that is not checkable. If it passes your linter then it is OK - take a chill pill, it won't hurt you if different parts of the code base have slightly different accents. The charm of those code bases is after a while you can instantly tell who wrote a piece of code without reaching for git blame. Let's spend our time in code review on logic errors, not style stuff!
-
Firstly, I'm coding in a very old language. Here's the exact line of code. Hopefully you can translate to a 'proper' language.
IF ERROR.MSG = "" THEN ERROR.MSG = "Error ":ERROR.CODE
It was a catch-all I inserted, (without being asked), to ensure that an error message would always be returned, even when it was an unknown error code. Yes, it's a single "=" for both conditions and assignments. And I was made to change it to upper-case. FFS! What they wanted, was:
IF ERROR.MSG = "" THEN
ERROR.MSG = "Error ":ERROR.CODE
ENDBut, the point that I was trying to make was: not that my way is right, but: if they want it doing a particular way, it should be documented - particular as the code-base contains a, pretty much, 50:50 split of both syntaxes.
-
Firstly, I'm coding in a very old language. Here's the exact line of code. Hopefully you can translate to a 'proper' language.
IF ERROR.MSG = "" THEN ERROR.MSG = "Error ":ERROR.CODE
It was a catch-all I inserted, (without being asked), to ensure that an error message would always be returned, even when it was an unknown error code. Yes, it's a single "=" for both conditions and assignments. And I was made to change it to upper-case. FFS! What they wanted, was:
IF ERROR.MSG = "" THEN
ERROR.MSG = "Error ":ERROR.CODE
ENDBut, the point that I was trying to make was: not that my way is right, but: if they want it doing a particular way, it should be documented - particular as the code-base contains a, pretty much, 50:50 split of both syntaxes.
-
Yup, anything that effectively comes down to an error exit could be a one-liner: typically a simple return or a throw. Anything else should be surrounded by {}, no matter the format. (that's what format tools are for). It's just not worth risking an incorrect semantic only because someone was too lazy to foresee that someone might change your one-liner into a two-liner. It's not like we're printing that code and need to save paper! :omg:
GOTOs are a bit like wire coat hangers: they tend to breed in the darkness, such that where there once were few, eventually there are many, and the program's architecture collapses beneath them. (Fran Poretto)
(If you got another message ignore it ... CP is posting my responses to the wrong msg).
It was only in wine that he laid down no limit for himself, but he did not allow himself to be confused by it. ― Confucian Analects: Rules of Confucius about his food
-
iskSYS wrote:
I think the point of the code review is to encourage developers to talk and agree on standards in their work environment.
Surely the main reason for a Code Review is to help prevent developers from breaking the production system. It's one of the key steps, (along with SIT and UAT), in the SDLC which are intended to reduce the risk of change. i.e. check for coding errors/bugs. Second would be, where possible, (and this isn't always the case), to check that the changes deliver the functionality that was requested. And don't break existing functionality. Next, (in my opinion), would be ensuring adherence to formal coding standards. Here, I'm talking more about structural, than syntactical stuff. You may have a standard program structure that should be followed; or standard functions that should be used - e.g. we have an email validation function - so we don't want 20 developers doing their own ('better') version. Finally, check 'adherence' to informal standards. This would be where coding style and readability sit. And this is a subjective thing - so much harder to manage.
I beg to differ, the unit tests, the integration tests, and system tests (automated and manual) are responsible to check whether or not the production systems are broken. Visually inspecting the code for bugs i would argue is not the best solution for that. In addition, some companies use static inspection tools such as code sonar. That said, of course inspecting the code doesn't hurt, but I think the main reason is to check whether code meets the code standards, architectural standards, design patterns etc. In some companies, the informal standards are usually solved with a standardized formatter rules. To reach this point however, developers need to define these rules, and thus my original argument, use code Review to create a set of rules for the formatter.
-
I think that the three-line version is easier to grok as a reader of the code, especially if you weren't the original author. In a lot of languages, the single-line version is a good starter for a bug: ```javascript if (condition) doThing(); ``` becomes ```javascript if (condition) doThing(); ``` and then someone adds one line, forgetting about the lack of braces ```javascript if (condition) doThing(); doTheOtherThing(); ``` and then `doTheOtherThing();` is _always_ invoked, which is precisely why most linters will recommend that you rather did: ```javascript if (condition) { doThing(); } ``` the first time. I understand your frustration, but remember that code reviews shouldn't be a place for "standing ground" or duking stuff out. They should be collaborative. So I'd recommend the following: 1. Ask for the "why" of the comment. If the "why" has value, even if it's not immediately apparent to you, but it's important enough to a team-member, and it doesn't break stuff, then try to accommodate 2. Update the coding standards doc (whichever way the team agrees is the accepted way) -- it sounds like it's still in the same place, so this sounds like a typical lawyer's argument where one has to refer to precedents rather than simply the letter of the law. It makes it harder for your next new team-member to collaborate 3. Prefer uniform rules, but even if you all decide that the three-line version has merit, you don't have to go back and fix the entire code-base. As people move through files to update for whatever reason, _then_ they can fix style issues. This is how we've addressed a number of style issues in our code-bases: discuss, agree, document, fix-up when you're in the area. For example, `this` in C# has been deemed as "noise", so whenever we work in a file which uses `this` unnecessarily (it may be required for an extension method, for example), then we clean up that file. Rider / R# makes it easy (`alt-enter, enter` is often enough). Similarly we can fix-up JS stuff via WebStorm intentions. /2c, ymmv
------------------------------------------------ If you say that getting the money is the most important thing You will spend your life completely wasting your time You will be doing things you don't like doing In order to go on living That is, to go on doing things you don't like doing Which is stupid. - Alan Watts https://www.youtube.com/watch?v=-gXTZM\_uPMY
Davyd McColl wrote:
and then someone adds one line, forgetting about the lack of braces
if (condition)
doThing();
doTheOtherThing();Indeed. Or, even more evil:
if (condition)
if (other_condition)
doThing();
else
doOtherThing();If you're not the one who wrote the original
if, nor the one adding thatelsesome time later, make a guess what was intended! :wtf:GOTOs are a bit like wire coat hangers: they tend to breed in the darkness, such that where there once were few, eventually there are many, and the program's architecture collapses beneath them. (Fran Poretto)
