|
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.
|
|
|
|
|
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.
|
|
|
|
|
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
|
|
|
|
|
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.
|
|
|
|
|
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 don't know about documented coding standards, but i frequently use one liner if, for, if-else and even for-if.
for me, when working in a lower level language, they represent some alternative to higher level constructs.
one liner if and for it 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 C if-else is not well suited for a one liner, because if and else are separated by a semicolon.
but in Pascal, they go swell.
if (condition) then expression1 else expression2;
on the other hand, whenever i use if as 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.
|
|
|
|
|
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:
if (condition) doThing();
becomes
if (condition)
doThing();
and then someone adds one line, forgetting about the lack of braces
if (condition)
doThing();
doTheOtherThing();
and then doTheOtherThing(); is always invoked, which is precisely why most linters will recommend that you rather did:
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:
- 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
- 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
- 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.
|
|
|
|
|
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 that else some time later, make a guess what was intended! 
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 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
|
|
|
|
|
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
|
|
|
|
|
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
|
|
|
|
|
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!
|
|
|
|
|
This week I've had the pleasure to work on an application I wrote three years ago.
Today I'm working on an application I started three months ago.
The recent application has a few less layers of, well, overhead.
I've gone from business and data layers to validations and (Entity Framework Core) database code straight in my controllers.
There's little to no reuse between the code anyway.
Then the database.
I've gone from hardcore "normalize ALL the things!" to "I need to save this redundantly anyway."
The redundant storage is mostly for reporting and historical accuracy (for example, save a name in a record as it is now, not relying on the name in a related record as that might change over time).
It feels like I've been studying all these best practices for years, but in the end only one practice really reigns supreme: YAGNI[^].
Don't get me wrong, I think you need to know the best practices before you have the right to ignore them, it should be a choice, not ignorance.
Kind of like not knowing the law is not an excuse to break it.
Is this what they call "experience"?
It certainly makes coding a lot easier 
This message was brought to you by Not-Working-In-A-Team, LLC.
|
|
|
|
|
Sander Rossel wrote: I think you need to know the best practices before you have the right to ignore them, it should be a choice, not ignorance.
Nice way to put it
Though, believe it would depend on mindset and with whom you are discussing/reviewing it. Flexible ones will take it and rigid ones would stick to 'best' known practice.
Sander Rossel wrote: Is this what they call "experience"?
|
|
|
|
|
Before I got to the end I was going to say, I think that's called "applying your experience", so yeah
I often feel the majority of the time, "best practices" are there to make sure all the noobs are doing things the same way. Less stress for the seniors, because they can just fob them off with "go and learn practice X".
And yeah, "no team" makes things a lot simpler.. the more people you have working on something, the longer it takes to get it done!
|
|
|
|
|
Sander Rossel wrote: The redundant storage is mostly for reporting and historical accuracy (for example, save a name in a record as it is now, not relying on the name in a related record as that might change over time).
That has always been my objection to over-normalization.
We almost always wound up with databases we lovingly called BigAss Tables. That comes of working mainly for the military where vendors and "customers" change frequently.
|
|
|
|
|
Yeah - what we've done vs. what we do - the improvements aren't limited to code, either.
However, agreeing with one of your points: fully normalizing SQL tables isn't always the best practice. Sometimes it just makes mores sense to have for each record instead of a reference to it (as a most likely scenario).
That term "best practices" - that's really someone's opinion. How you name your symbols? Hungarian notation was all the rage. I used camelCase. Now camelCase is much appreciated. I still use camelCase. And when next the latest and greatest comes out to be the next Best Practice . . . well, actually I can retire whenever I want so who gives a sh*t what they think? For, you see,
I figured out the real meaning of best practices - something that works, is maintainable, understandable by others and even yourself after a few years. Something that is enduring.
| Ravings en masse^ |
|---|
| "The difference between genius and stupidity is that genius has its limits." - Albert Einstein | | "If you are searching for perfection in others, then you seek disappointment. If you seek perfection in yourself, then you will find failure." - Balboos HaGadol Mar 2010 |
|
|
|
|
|
W∴ Balboos, GHB wrote: I figured out the real meaning of best practices - something that works, is maintainable, understandable by others and even yourself after a few years. Something that is enduring.
M.D.V. 
If something has a solution... Why do we have to worry about?. If it has no solution... For what reason do we have to worry about?
Help me to understand what I'm saying, and I'll explain it better to you
Rating helpful answers is nice, but saying thanks can be even nicer.
|
|
|
|
|
W∴ Balboos, GHB wrote: I figured out the real meaning of best practices - something that works, is maintainable, understandable by others and even yourself after a few years. Something that is enduring.
Hear! Hear! Well said!
"Go forth into the source" - Neal Morse
|
|
|
|
|
Just wait until you have forgotten more than some people will ever know.
I have lived with several Zen masters - all of them were cats.
His last invention was an evil Lasagna. It didn't kill anyone, and it actually tasted pretty good.
|
|
|
|
|
Experience is when you know which shortcuts you can take without them coming back biting your ass a few years down the line.
So yes.
Wrong is evil and must be defeated. - Jeff Ello
Never stop dreaming - Freddie Kruger
|
|
|
|
|
Jörgen Andersson wrote: Experience is when you know which shortcuts you can take without them coming back biting your ass a few years down the line.
And yet, they still will 
|
|
|
|
|
I think theoldfool answered that one below already. 
Wrong is evil and must be defeated. - Jeff Ello
Never stop dreaming - Freddie Kruger
|
|
|
|
|
Experience is something you think you have, until you get more of it.
If you can keep your head while those about you are losing theirs, perhaps you don't understand the situation.
|
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
