Code Reviews
-
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!
You do it all wrong! The correct syntax is:
if (parameter == null)
{
return;
}
else
{
}Reason: always add an else to an if, even if it's empty, because otherwise a future nested if could accidentally add an else at the wrong nesting level! ;P
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)
-
You do it all wrong! The correct syntax is:
if (parameter == null)
{
return;
}
else
{
}Reason: always add an else to an if, even if it's empty, because otherwise a future nested if could accidentally add an else at the wrong nesting level! ;P
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)
Take thy code to The Weird and The Wonderful[^] where it belongs! :laugh:
"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!
-
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 you may have won the battle, but lost the larger point of it all. What you accomplished was to make yourself difficult to deal with. I don't think that's what you intended.
The difficult we do right away... ...the impossible takes slightly longer.
-
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?
It's if (...), not if {...} I'll consider a single line if, if it is simple; and at the top of a method. Otherwise no. e.g. if ( parm1 == null ) { return; } And brackets around code: always. Even one-liners.
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
-
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.
-
Richard MacCutchan wrote:
Cobol?
:) It's slightly easier than that! It's a (fairly significant) variation on Basic, which comes as part of the Universe DBMS. It's derived from Pick DataBasic (another one you won't have heard of!) and is actually very easy to develop with - albeit limited in scope. i.e. It's for dumb terminal applications. I like it though.
-
Absent a team style document, definitely stand your ground. And certainly ignore weenies who try to quote from other teams' style documents. "But Microsoft says.." "We don't work for Microsoft, you weenie!" I'm a whitespace supremicist as most of you know, so I nearly always use as much vertical space as I deem reasonable. There are times, though, when I would use a series of one-liners. Usually if the tests and actions are very similar -- to show how they are similar.
if ( TestX ) { DoX() } ;
if ( TestY ) { DoY() } ;
if ( TestZ ) { DoX() } ;
...I feel that in this situation, this style leads to more readable/understandable code and that copy/paste errors like above may be more easily spotted. Does the language you're using not allow:
{do something} if {condition}
VAX BASIC V3.9-000
Ready
10 LET X = 42
20 PRINT "yes" IF X = 42
runnhyes
Ready:laugh:
-
I think you may have won the battle, but lost the larger point of it all. What you accomplished was to make yourself difficult to deal with. I don't think that's what you intended.
The difficult we do right away... ...the impossible takes slightly longer.
I didn't just make myself difficult to deal with. I've been like that for about 40 years! :) Hopefully, anyone who knows me, knows that as well as being challenging and pedantic, I will always accept when I'm wrong; never take myself too seriously; and never get personal. Also, I generally give those above me a hard time, rather than my peers. Yep, my chances of promotion are zero - but that matches my ambition. I am way too old to be worrying about career and more than happy to come in and do the day-to-day grunt work.
-
Richard MacCutchan wrote:
Cobol?
:) It's slightly easier than that! It's a (fairly significant) variation on Basic, which comes as part of the Universe DBMS. It's derived from Pick DataBasic (another one you won't have heard of!) and is actually very easy to develop with - albeit limited in scope. i.e. It's for dumb terminal applications. I like it though.
-
I didn't just make myself difficult to deal with. I've been like that for about 40 years! :) Hopefully, anyone who knows me, knows that as well as being challenging and pedantic, I will always accept when I'm wrong; never take myself too seriously; and never get personal. Also, I generally give those above me a hard time, rather than my peers. Yep, my chances of promotion are zero - but that matches my ambition. I am way too old to be worrying about career and more than happy to come in and do the day-to-day grunt work.
Oh, well in that case, carry on :-D
The difficult we do right away... ...the impossible takes slightly longer.
-
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 the point of the code review is to encourage developers to talk and agree on standards in their work environment. Don't discuss it with us, discuss it with the other developer, add more reviewers. Think of the consequences of your decision for future problems and future if statements
-
You do it all wrong! The correct syntax is:
if (parameter == null)
{
return;
}
else
{
}Reason: always add an else to an if, even if it's empty, because otherwise a future nested if could accidentally add an else at the wrong nesting level! ;P
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)
Nah! The correct syntax to use is:
if (parameter =
= NULL)
{
return;
}
else
{
} -
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?
One liner is fine, but I always add the brackets since getting stung by a badly formed #define or macro (can't recall which). It had a ; in it, so only the first statement in the do something was executed
-
I think the point of the code review is to encourage developers to talk and agree on standards in their work environment. Don't discuss it with us, discuss it with the other developer, add more reviewers. Think of the consequences of your decision for future problems and future if statements
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.
-
Take thy code to The Weird and The Wonderful[^] where it belongs! :laugh:
"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!
Well, it definitely doesn't belong on an agile forum. But it would be fun ;P
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 agree. I always do brackets even for one-liners because, in the words of Forrest Gump, "That's one less thing to worry about".
- I would love to change the world, but they won’t give me the source code.
-
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?
Not everything has to be documented. An oral agreement is legally valid, so you could've taken the opportunity to once and for all solve this matter (until someone forgets it or a new member joins the team) without documenting anything. Of course it would be nice to write it down for future reference, and if you already have such a document you should certainly add it, but my experience is that no one ever reads it anyway (unless they want to prove someone else wrong). Style-wise I agree with your coworker.
Best, Sander sanderrossel.com Migrating Applications to the Cloud with Azure arrgh.js - Bringing LINQ to JavaScript Object-Oriented Programming in C# Succinctly
-
It's if (...), not if {...} I'll consider a single line if, if it is simple; and at the top of a method. Otherwise no. e.g. if ( parm1 == null ) { return; } And brackets around code: always. Even one-liners.
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
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)
-
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?
Assuming this is at work. Have a discussion and vote on the syntax you all want to use and document it as a standard in your workplace only. These debates happen all the time and the only way to move forward is to all agree on a single way. The code is easy to understand when it's all int he same format, There's less time thinking about what option to take, and no one will reformat files to their own preferred way. Just make it clear that there is no "right" or "wrong" way of doing this. All accepted syntax by the IDE is correct, but being aligned is more important. and when voting make sure to include an option for "no preference". Make sure you document these decisions in a "[Company name] Best practices". Eventually the debates will stop and you will have some awesome best practices for new starters to just pick up
-
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 don't know about documented coding standards, but i frequently use one liner
if,for,if-elseand evenfor-if. for me, when working in a lower level language, they represent some alternative to higher level constructs. one linerifandforit that scenario, at least for me, are not equivalent to branching and looping. the are equivalent to something that in JavaScript would represent Array.map(), Array.every(), Array.some()... for (condition) if (condition) expression; in Cif-elseis not well suited for a one liner, becauseifandelseare separated by a semicolon. but in Pascal, they go swell.if(condition)thenexpression1elseexpression2; on the other hand, whenever i useifas a logical condition for branching (in old books represented as rhombus with arrows going left or right), i always create a new block at a new line.
