OSDev.org

The Place to Start for Operating System Developers
It is currently Thu Dec 12, 2019 11:44 am

All times are UTC - 6 hours




Post new topic This topic is locked, you cannot edit posts or make further replies.  [ 16 posts ]  Go to page 1, 2  Next
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: 256
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: 9287
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: 1066
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: 256
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: 256
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  
 
 Post subject: Re: Effective Code Reviews
PostPosted: Fri Aug 02, 2019 7:07 am 
Offline
Member
Member
User avatar

Joined: Sat Mar 31, 2012 3:07 am
Posts: 3593
Location: Chichester, UK
It's very long.


Top
 Profile  
 
 Post subject: Re: Effective Code Reviews
PostPosted: Fri Aug 02, 2019 8:11 am 
Offline
Member
Member
User avatar

Joined: Fri Oct 27, 2006 9:42 am
Posts: 1521
Location: Athens, GA, USA
Pardon me, I need to go wash this blood off my face again...

_________________
Rev. First Speaker Schol-R-LEA;2 LCF ELF JAM POEE KoR KCO PPWMTF
μή εἶναι βασιλικήν ἀτραπόν ἐπί γεωμετρίαν
Lisp programmers tend to seem very odd to outsiders, just like anyone else who has had a religious experience they can't quite explain to others.


Top
 Profile  
 
 Post subject: Re: Review this!!
PostPosted: Sun Aug 04, 2019 4:30 am 
Offline
Member
Member

Joined: Mon Jul 25, 2016 6:54 pm
Posts: 187
Location: Adelaide, Australia
Code:
line_30710:
    II = ttt1 + 1
For JJ = II to III

Oh my


Top
 Profile  
 
 Post subject: The only person that can effectively code review this ---- I
PostPosted: Sun Aug 04, 2019 6:43 am 
Offline

Joined: Mon Jul 09, 2018 9:32 am
Posts: 12
The only person that can effectively code review this ---- Is Me.

You'd be a fool to think otherwise.
I know the shortcomings and the opportunities that this Fantastic app has.
AND

My review has nothing to do with loops or goto or anything else that doesn't matter.

I give this code a 97%


Top
 Profile  
 
 Post subject: Re: Effective Code Reviews
PostPosted: Sun Aug 04, 2019 7:08 am 
Offline
Member
Member
User avatar

Joined: Sat Mar 31, 2012 3:07 am
Posts: 3593
Location: Chichester, UK
Quite right.

Who else is going to waste their time reading thousands of lines of crap?


Top
 Profile  
 
 Post subject: Re: Effective Code Reviews
PostPosted: Sun Aug 04, 2019 9:51 am 
Offline
Member
Member

Joined: Mon Jul 25, 2016 6:54 pm
Posts: 187
Location: Adelaide, Australia
If anything that code makes a great case for review, because a reviewer wouldn't let you submit code that would be so difficult to maintain.

I don't want to discourage you SpectateSwamp, but if that is really your work, you should focus on learning some good programming practises. It really is quite poorly written.


Top
 Profile  
 
 Post subject: Re: Effective Code Reviews
PostPosted: Sun Aug 04, 2019 2:00 pm 
Offline
Member
Member
User avatar

Joined: Fri Oct 27, 2006 9:42 am
Posts: 1521
Location: Athens, GA, USA
StudlyCaps wrote:
I don't want to discourage you SpectateSwamp, but if that is really your work, you should focus on learning some good programming practises. It really is quite poorly written.


That's the problem. If you look at his posts on what.thedailywtf.com (NSFW, to say the least - not Swampy's posts, but almost everyone else's, including my own), you'll find that he's advocating - vociferously - for this approach to programming, and insists that 'noodling' is the only proper way to write code.

He feels that spaghetti code is easier to write, read, and understand than structured code, and insists that structured code is 'unreadable'. He also claims that all competent programmers really 'jam' their code the same way he does, and that advocates of other approaches are nothing more than a handful of 'perfect perfects' shouting down the ones who point this out.

A few relevant quotes from that thread and related ones:
SpectateSwamp wrote:
Probably because I LOVE Line Numbers too. They tried to Steal line numbers from You but SSDS has brought them BACK. Every possible error must come from within this ONE module (how simple). I can just jam it and jam it till things fail.

SpectateSwamp wrote:
Nested statements look like an eagle flying across the page and aren't that readable.
To be fair you'd need a copy of VB5 to jam it like I do. So you are somewhat handicapped.

SpectateSwamp wrote:
Yup the "programmatically corrects" got rid of the line numbers but we can use pseudo line numbers.
When I want to make a change. I don't have to consider what other routines need to be considered. There aren't any.

SpecatateSwamp wrote:
Spaghetti code spawns serendipity not so with structured code. I can jam my code through trial and error and get results. Structured is not so flexible
90% of the greatest programmers coded noodle code.
I'm against anything done in the name of efficiency... unless it makes the code easier to understand.


SpectateSwamp wrote:
It's not the code that counts it's the results.
The topic remains unchallenged...
There are a few examples in the code that the "perfect perfects" wouldn't like or may even drive them crazy
What about keep it simple don't you understand?
I have no problem diving back into code I wrote nearly 20 years ago.

SpectateSwamp wrote:
Computers are thousands of times faster now...
Anybody that worries about performance is STUPID.

Here:
SpectateSwamp wrote:
Maybe try Cobol or Fortran versions. There are probably more Cobol apps running businesses than Basic. They at least have GOTO's. How can you do any jamming it, without a simple GOTO to skip around some untested logic or past a display that you want to disable or view only when running in "test mode" Those overly nested systems have code looking like a bird flying across the screen. GoTo's clean that up. Local error trapping makes the code even more useless. I do most error checking using an "on_error routine" to simplify things and for testing. Good Example: Mid 80's. I had FlowCharted the billing, workorder and monthend ... logic for the CableSystem I was working at. Doing a 2 or 3 page chart for each. Not displaying the unimportant checks. Just those that were significant. After returning from a 2 month off-site conversion. I found that most of my charts were thrown out and the rest were in the process of being replaced with 10 to 12 pages for each function. I should have made copies. I should have made copies. It was easier to go directly to the program than look at those next to useless line for line flowcharts. That's the main reason for having all or most of the error checking moved down to an exception handling area for this Search. Far more understandable Far more easy to jam and test. This Search wasn't created by a Genius. So It doesn't need a bigger Genius to maintain or upgrade it. Much easier to test out 1 simple program that's Centrally trapped, Than a 100 or more adjunct parts that make it next to impossible to extend or upgrade. Better quit yapping and put in The jerky jerk backwards option. At the same time allow for a fast forward video playback. Watch out for those Systems that were written by a Genius. What are you going to do when your programmer gets old and sick. Or dies in a fiery crash. What are you going to do.
Yup if that language can't support this style of programming it ain't worth squat
We need a society to protect Cobol Fortran and Basic. No perfect perfects allowed.

SpectateSwamp wrote:
Nobody shares computer knowledge better than this
Years ago a friend and mentor (Grant Cook) said that on the next system he developed all the data files would be TEXT only. Forget the integer, real and other numeric formats.
I'm a firm believer in that. When you need to do a data fix, how easy it is to use a simple text edit program like notepad.
I really love line numbers those that are against, don't have any idea how simple they make things. A magical noodle here and there and serendipitous events sometimes happen.
If they take away "goto's" I'm be sunk. Few of the purist programmers would have hired me for my coding methods. I wouldn't hire any of them either.

SpectateSwamp wrote:
[Pre-emptive multitasking is a]nother stupid idea by Microsoft. Right up there after taking away line numbers.
Probably all to speed things up. You have to have slow code to make use of the computer power we have today. SSDS is poised to make good use of quantum computing.. Bring it on.

_________________
Rev. First Speaker Schol-R-LEA;2 LCF ELF JAM POEE KoR KCO PPWMTF
μή εἶναι βασιλικήν ἀτραπόν ἐπί γεωμετρίαν
Lisp programmers tend to seem very odd to outsiders, just like anyone else who has had a religious experience they can't quite explain to others.


Last edited by Schol-R-LEA on Sun Aug 04, 2019 2:26 pm, edited 3 times in total.

Top
 Profile  
 
 Post subject: Re: Effective Code Reviews
PostPosted: Sun Aug 04, 2019 2:14 pm 
Offline
Member
Member
User avatar

Joined: Sat Mar 31, 2012 3:07 am
Posts: 3593
Location: Chichester, UK
Why are we even bothering with a guy who thinks that VB5 is the height of programming science?

Can't we all just ignore this guy until he gets fed up?


Top
 Profile  
 
 Post subject: Re: Effective Code Reviews
PostPosted: Sun Aug 04, 2019 6:51 pm 
Offline
Member
Member

Joined: Mon Jul 25, 2016 6:54 pm
Posts: 187
Location: Adelaide, Australia
Oh wow, that's some powerful Dunning-Kruger energy.

e: Also agree with iansjack, he seems pretty happy with his "style" so probably not much point arguing the point.


Top
 Profile  
 
 Post subject: Re: Effective Code Reviews
PostPosted: Mon Aug 05, 2019 4:17 am 
Offline
Member
Member
User avatar

Joined: Thu Nov 16, 2006 12:01 pm
Posts: 7431
Location: Germany
You realize that literally all this user has posted so far was two trollish necro-thread-hijacks?

Don't feed the trolls. 8)

_________________
Every good solution is obvious once you've found it.


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic This topic is locked, you cannot edit posts or make further replies.  [ 16 posts ]  Go to page 1, 2  Next

All times are UTC - 6 hours


Who is online

Users browsing this forum: No registered users and 1 guest


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