OSDev.org

The Place to Start for Operating System Developers
It is currently Sun Jul 21, 2019 5:38 pm

All times are UTC - 6 hours




Post new topic Reply to topic  [ 5 posts ] 
Author Message
 Post subject: Effective Code Reviews
PostPosted: Tue Mar 12, 2019 11:13 am 
Offline
Member
Member
User avatar

Joined: Thu Jun 04, 2009 11:12 pm
Posts: 253
What steps do you do get good review comments from reviewers ? I am just curious to see how other engineers go about it ? When i submit code for review, i want other engineers to engage creatively about how the code can break not about "how their way of doing it is better than others" or a misplaced indentation in comments which has absolutely no effect on product. I eventually end up finding the possible flaws myself and review just seems to have no good effect at times.


--Thomas


Top
 Profile  
 
 Post subject: Re: Effective Code Reviews
PostPosted: Tue Mar 12, 2019 11:47 am 
Offline
Member
Member
User avatar

Joined: Wed Oct 18, 2006 3:45 am
Posts: 9284
Location: On the balcony, watching the Swedish Chef
Companies keep standards. Not because the standard is the best, but because it makes things predictable and to allow anybody on the team to deal with your code as efficiently as anybody's. Don't confuse it with egocentrism.
Things like that are also much easier on people compared to mentally doing a full data flow analysis on your submitted code, which effectively means anybody can review your indentation, but not everybody has the mindset to effectively notice all the corner cases and antipatterns that might be present. And of course, there's quite often pressure on people to finish their thing, typically at the cost of your thing.

The one remaining legitimate issue here seems to be that bugs get missed in review. For that one, I hope you are skilled in the workplace art of discussing problems without becoming confrontational.

_________________
"Certainly avoid yourself. He is a newbie and might not realize it. You'll hate his code deeply a few years down the road." - Sortie
[ My OS ] [ VDisk/SFS ]


Top
 Profile  
 
 Post subject: Re: Effective Code Reviews
PostPosted: Tue Mar 12, 2019 12:33 pm 
Offline
Member
Member
User avatar

Joined: Sun Sep 19, 2010 10:05 pm
Posts: 1065
If you are reviewing someone's code, just be clear about the nature of any issues that you discover. Minor issues, or personal preferences where you would have taken a different approach are fine, as long as everyone is clear that a) the code does not necessarily need to be changed, and b) you explain, briefly, why you would have done it a different way. (i.e. Aesthetics, performance, simplicity, readability, etc.)

On the flip side, if someone is reviewing your code, be sure you understand where the reviewer is coming from in regards to the items above. If necessary, ask specifically whether their issue is more of a personal preference, or if it is something that needs to be addressed.

Everyone involved just needs to realize that no two developers will ever agree on a particular approach, so unless you have made some sort of team agreement, or the company or client has mandated some sort of guidelines, all solutions should be considered acceptable as long as the end result meets the expected behavior.

It's hard to look at a block of code that is new or that has been changed by someone else, and spot logic errors in a 5 minute code review. Which is why we've moved to pair programming, or mob programming with up to 5 people in a conference room working on a single task. It is very unlikely that you will have a typo (= instead of ==), or a logic error (!= instead of ==), or forget a critical step when other people are watching. As soon as you deviate from their expected path, they will immediately start making noise. Another advantage is that once you are done, you will have 2-5 people who understand how the code works, rather than 1-2. However, this approach can be overkill for simple modifications, like label changes, or hiding/removing UI elements. But it does virtually eliminate the need for code reviews...

_________________
Project: OZone
Source: GitHub
Current Task: DOSBox Compatibility
"The more they overthink the plumbing, the easier it is to stop up the drain." - Montgomery Scott


Top
 Profile  
 
 Post subject: Re: Effective Code Reviews
PostPosted: Tue Mar 12, 2019 3:22 pm 
Offline
Member
Member
User avatar

Joined: Thu Jun 04, 2009 11:12 pm
Posts: 253
Code:
Companies keep standards. Not because the standard is the best, but because it makes things predictable and to allow anybody on the team to deal with your code as efficiently as anybody's. Don't confuse it with egocentrism.

Ideally this would be true. There are global coding standards. But an owner of a particular module might do it differently going against the existing standards and i fit my style to mimic it. My gripe is that comments should not be only about coding standards alone which are appreciated.

Code:
there's quite often pressure on people to finish their thing, typically at the cost of your thing.

This is a real problem and to be fair it is not possible to catch everything in a review. However, I get the feeling that just looking at the coding standards is an easy way out!.

Most people are motivated and intelligent. I was thinking how we can tap into that without making it a lot of work.

--Thomas


Last edited by Thomas on Tue Mar 12, 2019 3:35 pm, edited 2 times in total.

Top
 Profile  
 
 Post subject: Re: Effective Code Reviews
PostPosted: Tue Mar 12, 2019 3:26 pm 
Offline
Member
Member
User avatar

Joined: Thu Jun 04, 2009 11:12 pm
Posts: 253
Quote:
Which is why we've moved to pair programming, or mob programming with up to 5 people in a conference room working on a single task. It is very unlikely that you will have a typo (= instead of ==), or a logic error (!= instead of ==), or forget a critical step when other people are watching. As soon as you deviate from their expected path, they will immediately start making noise. Another advantage is that once you are done, you will have 2-5 people who understand how the code works, rather than 1-2. However, this approach can be overkill for simple modifications, like label changes, or hiding/removing UI elements. But it does virtually eliminate the need for code reviews...


Interesting reply. Thank you!.


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 5 posts ] 

All times are UTC - 6 hours


Who is online

Users browsing this forum: No registered users and 2 guests


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Search for:
Jump to:  
Powered by phpBB © 2000, 2002, 2005, 2007 phpBB Group