Idea: Code Review Groups?

Go To Last Post
24 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

This is not my idea - ford2go suggested it in an off-forum email.

 

Would it be useful to form a "group", maybe several, to do mutual code reviews? To quote ford2go:

 

I began to wonder if it would ever be possible to form a small group of folks for the purpose of reviewing each other's stuff. By this I mean some kinf of online video review where at least the presenter could be seen and heard and the audience could make comments heard ( or possibly seen as text) by all. 

 

I do realize that only a few of us have participated in such an activity. In fact, even with all of my experience, I have never been though a design review of any sort, hardware, firmware, anything. But, non-the-less, the idea has some appeal, though tinged with dread. 

 

Could it work, here?

 

Cheers

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

Last Edited: Tue. Apr 2, 2019 - 05:55 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ka7ehk wrote:
Could it work, here?
Yes for open, for FOSS, or carefully created snippets; otherwise, no because some of what we create is private because customer's data and keys are private.

 

"Dare to be naïve." - Buckminster Fuller

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Considering how frail the site's infrastructure is to begin with I don't think we could support a video feed

ECJ

I would rather attempt something great and fail, than attempt nothing and succeed - Fortune Cookie

 

"The critical shortage here is not stuff, but time." - Johan Ekdahl

 

"If you want a career with a known path - become an undertaker. Dead people don't sue!" - Kartman

"Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?"  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB, RSLogix user

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Not sure the infrastructure would HAVE to be Freaks. Maybe some variant of Skype with screen sharing and conference calling would work?

 

Also, I'm not so much thinking of a standing group here. Instead, maybe a way to ask a couple of folks to do a code review with me, perhaps on a confidential basis. I am not so worried about confidentiality but I do know that some, here, certainly would be.

 

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ka7ehk wrote:
Maybe some variant of Skype with screen sharing and conference calling would work?
Scheduling would be a headache as AVR Freaks are worldwide.

Tele-conferences are significant events that are more likely at an iteration's kickoff and wrap (iteration duration is typically 2 to 6 weeks)

An alternate is your preferred version control system especially if it has a web interface for code review (servers do the heavy lifting, operators have a compatible web browser); repositories can be private with controlled access by you.

 

MS wants to buy GitHUb | AVR Freaks

Product | GitLab | Code Review

 

edit: an advantage of a non-web interface is a code reviewer can apply their tool box to the reviewed code; a disadvantage is loss of copy control.

 

"Dare to be naïve." - Buckminster Fuller

Last Edited: Tue. Apr 2, 2019 - 10:00 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Had not considered VCS with code review features. Will check that out.

 

I had not considered the kind of code review I am thinking of as a major event. Perhaps I am thinking of something more like an "iteration wrap" (which I have never heard of - have never worked anywhere where there were more than 2 or 3 coders at any one time). I know that to be thorough, a full code review would probably take several hours and more than 2 or 3 reviewers, at least a few of those being at a higher skill/knowledge level than I am. On the other hand, ANY code review would be better than what any of us "small shop" folks are able to pull together, now. 

 

I think that I will explore what sharing tools can be pulled together for an ad-hoc small review group and just try contacting a few of the regulars to see if something useful comes out of it.

 

Thanks

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Wonder if there's  an open Gerrit site somewhere? Actually I think just plain github lends itself to peer review.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Have not heard of Gerrit - will explore it. Will also see what github can do.

 

Thanks

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ad-hoc code reviews probably do more damage than good. You get ten different people and you'll get ten different reviews. This only serves to confuse the reviewee. You really need a standard to review against - coming up with the standard is difficult. Once you have a standard, then you know what to code against and the reviewer can check for compliance. Embedded code can more more difficult as the rules are often broken to gain performance or to meet the constraints of ram/flash etc.

 

The above  hopefully ensures compliance with the standard, but doesn't address the code architecture. Often you can get code that is coded beautifully, but the architecture is woeful. Also, checking architecture requires the reviewer to understand the application. 

 

For a standard, I'd suggest MISRA. It can be tricky to be 100% compliant (Cliff will attest) and for the most part I only try to be compliant in spirit. There are code analysers that will test against the MISRA standard and spew out reams of non-compliances! Some compilers (IAR and TI) have MISRA checking as options. The MISRA spec can be purchased for a reasonable fee - I think it cost me AUD $25. It nevertheless is a good read and makes you think about how you write your code.

 

I once had a code review on a project that I worked on. The management got an engineer who thought he 'knew it all' to review it. He concentrated on naming conventions (there were a couple of Arduino libraries for i2c and serial) and my use of C++ in a project that was resource constrained. He was telling me how to 'optimise' the code but he mustn't have even looked carefully as I had picked just about all the low hanging fruit - I had compile options for table or loop driven code for CRCs and AES encryption. Management took his comments on board and berated me. I asked to be removed from the team - I no longer work for them.  

 

Having said that, well structured code reviews with standards can be very beneficial to the overall code quality and the dissemination of knowledge in a team.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Kartman wrote:
For a standard, I'd suggest MISRA.
more

Embedded C Coding Standard | Barr Group

Proposed Rule Changes for Embedded C Coding Standard « Barr Code

...

In a sentence, MISRA C is a safer subset of the C language plus a set guidelines for using what is left of the language more safely. In about as many words, BARR-C is a style guide for the C language that reduces the number of defects introduced during the coding phase by increasing readability and portability.

...

 

"Dare to be naïve." - Buckminster Fuller

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

At this point, I am thinking less of a formal review and more of "eyes other than my own". It might not be as beneficial as a highly structured, standard based review. But lacking the capability of doing a full blown review, I am thinking that, with reasonable expectations and with some modest understanding of the purpose, and a few basic ground rules,  that it could turn out better than nothing. Potentially a bit better than that.

 

I see such a discussion providing questions (and hopefully responses) as "Why did you do that?" Or, "Did you consider ... ". Or, "You know, that code does not do what you say that it is supposed to do!" I see it more of a peer-review than a court trial with expert witnesses.

 

Those are my thoughts right now.

 

Jim

 

 

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

A lot will depend on the egos of all the participants,
ie. some reviewers might get upset if their inputs are not implemented
and/or the target might discover that they have problems coping with a barrage of (seemingly negative) comments.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Good points. And certainly concerns.

 

For me, personally, the "ground rules" would have to address those things at the start. 

 

As sort of a very rough outline, I see that it might work like this:

 

1) A Freak decides that he or she needs more eyes looking at the whole or a large part of a project. For lack of better term, I will call this a "review". This person, also for lack of a better term, is the "presenter".

 

2) Presenter prepares the source code in a share site where invited reviewers can look at and/or download before hand.

 

3) Presenter sets up the conference platform, what ever it turns out to be. (This is a part that I will do some serious searching on). This platform should allow, at the vary least, n-way voice conference, and screen sharing that can be handed off among participants. Even better if it is video.

 

4) Presenter chooses a couple of peers, probably other Freaks. Criteria may vary with presenter but might include probable availability, enough programming experience to be able to be reasonably accurate in comments and assistance, suitable personality, ability to explain, and such.

 

5) Presenter develops description, purpose, and ground rules for the review.

 

6) Presenter contacts the select list, and, for those who accept, post a Doodle poll (or similar) to select a common time. Allow for at least two hours, perhaps with a break.

 

7) Presenter is responsible for making sure that all participants can access the code to be reviewed.

 

8) The presenter may ask one of the participants to be the "master", for the purpose of tending to ground rules and general moderating. If used, there should be some preparation before the review.

 

9) At the appointed date and time, the presenter sets up the conference platform and initiates contact with the participants. The presenter will generally determine the topics to be addressed, the order in which things are considered, the hand-off of screen sharing as needed, and recording of suggestions and comments. 

 

10) Presenter summarizes the comments and suggestions near the end. Presenter might comment on what changes are likely to come out of the discussion. Presenter might also comment on what has been learned from the discussion.  Thanks are offered to the participants. 

 

11) Presenter tears down the conference platform.

 

12) THEN, finally, the presenter proceeds with figuring out how to use ideas, suggestions, comments, criticisms, and such, to improve the code that was discussed. This MIGHT include not doing anything, at all, that was directly driven by the review!

 

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Kartman wrote:
ad-hoc code reviews probably do more damage than good.
I sort of 50% agree with this. I also agree with the idea of adhering to standards and especially MISRA. But the kind of review thing I'm thinking of is simple things like:

int offset, i;

for (i = 0; i < 50; i++) {
    offset = 7;
    x = x + offset;
}

Hopefully someone will point out that offset=7 doesn't need to be (in fact should not be) in the for() loop. (yeah, a smart compiler will know this and move it anyway - but that's not the point).

 

@Jim,

 

I was googling to see if there were online sites where you could use Gerrit for reviews (it's what we use and it's most excellent) but can't really find much so I think it would be a lot of work to set up something (I found a site that explained how to add it to github but it was pretty complicated stuff and, anyway, you don't want to have to be a Gerrit server maintainer (we employ something like 50 people for that!). i did find something about simply using github itself for reviews:

 

https://help.github.com/en/articles/reviewing-proposed-changes-in-a-pull-request

 

The idea I was thinking of is that anyone with a project to review simply push it into a repository like Git - perhaps simplistically just using different sub-dirs for different projects or perhaps using feature branches? Others could pull the code to build it locally or simply read it online and then use the above facilities to annotate the code with comments.

 

While I guess you can do project reviews interactively with conference facilites and so on we find that simply posting comments to each other in Gerrit works very nicely anyway.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ka7ehk wrote:
In fact, even with all of my experience, I have never been though a design review of any sort, hardware, firmware, anything.
You don't know what you're missing wink

Dan asked Jack if code reviews are necessary and Jack said yes because code reviews

  • identify design defects
  • are one of the forces on code quality

The Embedded Muse 379 - Inspections or Static Analysis?

Dan Swiger writes:

...

So my question to you is, how do you feel about using static analysis as a replacement for peer review? 

...

 

P.S.

Dan Swiger writes:

We evaluated Code Collaborator and decided to purchase it to easily document and be able to report peer reviews.

...

Code Review Tool and Peer Review Platform | Collaborator by SmartBear

  • client-server
  • Java
  • clients : web, desktop, IDE (Visual Studio, Eclipse)

Technical Specifications | Collaborator Documentation

 


popped out of a web search engine :

Code Collaborator Alternatives and Similar Software - AlternativeTo.net

 

A design is typically in a document that is manually executed (walk-through, formally once or a few times, informally many times in code reviews) though a design can be executable (a model)

A design and the code have bi-directional links; all are complete, precise, and correct (design, code, links) (easier said than done ... best effort)

 

"Dare to be naïve." - Buckminster Fuller

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

mikech wrote:
A lot will depend on the egos of all the participants, ie. some reviewers might get upset if their inputs are not implemented and/or the target might discover that they have problems coping with a barrage of (seemingly negative) comments.
Agree. I know my limitations and do not cope with criticism all that well no matter how diplomatically delivered. That is one reason why you very, very rarely see me offering coding advice here.

Ross McKenzie ValuSoft Melbourne Australia

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

All code I write has to be reviewed by someone else - this is a mandatory step for us and is why we use Gerrit. I also spend a lot of my life reviewing other people's code too.

 

It's true it can be a bit frustrating when you think you have the "perfect solution" and some moany individual tells you "I don't like that way of doing XXX very much" !

 

I can't imagine such code review being done by a code analysis software - no machine intelligence can some up with some of the "left field" thinking that a human is capable of (yet!).

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

In the same "Embedded Muse" quoted by gchapman, Jack Gannsle also writes:

 

Code inspections, done correctly, have two goals. The first is to find design issues: does this code do what we want it to? The second is to, well, essentially scare the developer into working hard to avoid errors in the first place. 

 

...

 

"Scare the developer into working hard to avoid errors in the first place" means that any normal person will try harder to create high-quality work products if he knows he'll be accountable to his peers. More time spent in implementation, carefully checking for problems, means better code, fewer bugs, less debugging, and ultimately faster delivery. Isn't that what we want?

Jim 

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

My first experience with peer reviews was with hardware design reviews.  After spending several days working on a schematic and PCB layout, we would gather to go over the design and compare with the project requirements.  It was painful to find a mistake or have your design decisions questioned, but by the end of the ordeal (opps, review) you left with a better design/PCB then you entered with.

After a while you expected changes and the list of design guidelines grow as lessons were learned and experience grew.

Now I'm involved with software reviews as well, yes it hurts at first, but better code is the result.  You learn and grow, enjoy it.

 

Jim

 

Click Link: Get Free Stock: Retire early! PM for strategy

share.robinhood.com/jamesc3274
stack gold/silver https://www.onegold.com/join/713...

 

 

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

For code review, I would not suggest requiring any particular coding style.

That said, a couple suggestions:

Avoid the GNU coding style like the Black Death.

Except in rare cases, have no source lines longer than 80 characters.

Never have such a source line end in a comment.

 

For functionality, I'm not sure a live event (in person or otherwise)

is right for a general-purpose review.

That would seem to require some major thinking on one's feet.

Once a group reaches a disagreement about something in particular,

that might be the time for such an event.

 

For my first contribution to the new code-review system:

Put in some bloody comments,

even if they are just references to the design document.

In my experience, that hardest part of reading someone

else's code is often figuring out what it is supposed to do.

"Demons after money.
Whatever happened to the still beating heart of a virgin?
No one has any standards anymore." -- Giles

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You also need to have inquisitive discussions with a lot of "what ifs", not just checking to see if code meets the specs...unfortunately specs often have a myriad of holes, even when they seem very complete.   Probably need to have more spec reviews first.  Sometimes there is no substitute for also trying the code (albeit, this is at a different phase).

"Yep, works great, even has the key debouncing and all of our other specs we covered" Six months later..Oh the date button is stuck, and the keyroutine holds, waiting for button release.  Now the stop button can't halt the pumping of gasoline...oops.    

 

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
I can't imagine such code review being done by a code analysis software - no machine intelligence can some up with some of the "left field" thinking that a human is capable of (yet!).
Linux Foundation's Core Infrastructure Initiative (CII) has one proprietary static analyzer (Synopsys Coverity) and some of the FOSS (LGPLv2) Frama-C (formal methods implemented in C)

CII does have audits (formal reviews)

Linus Torvalds and those who aid him are well experienced and there's probably enough of them.

It's the surrounding bits that don't have enough reviewers.

 

Frequently Asked Questions - Core Infrastructure Initiative

Why didn't you think about doing this before the lack of funding for OpenSSL resulted in Heartbleed?

We’re doing what we can now collectively to identify critical projects being overlooked or underfunded so that we drastically reduce the chances of this happening again.

Tooling - Core Infrastructure Initiative

Coverity Scan - Static Analysis

Travis CI - Test and Deploy Your Code with Confidence

Continuous Integration and Delivery - CircleCI

Frama-C

 

"Dare to be naïve." - Buckminster Fuller

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

avrcandies wrote:
"Yep, works great, even has the key debouncing and all of our other specs we covered" Six months later..Oh the date button is stuck, and the keyroutine holds, waiting for button release.  Now the stop button can't halt the pumping of gasoline...oops.
I do hope the stop button next to the pump is mechanical.

I'm reminded of a series of radiation overdoses caused by a

pretty much completely computer-controlled x-ray machine.

All the safety features were were in software.

On one of the occasions, they were defeated by a fast typist.

The manufacturer's choice of radiation-detector didn't help either.

"Demons after money.
Whatever happened to the still beating heart of a virgin?
No one has any standards anymore." -- Giles

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

skeeve wrote:

I'm reminded of a series of radiation overdoses caused by a

pretty much completely computer-controlled x-ray machine.

All the safety features were were in software.


https://en.m.wikipedia.org/wiki/Therac-25

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]