SSDC Document Deep Dive - Code Review Checklist

[swatts]

Hello peeps who build stuff with pictures and wires.

SSDC stands for Structured Software Design Consultants, it shouldn't therefore be a surprise that we have a pretty structured approach to the software process. We like checklists, so here's a closer look at our code review checklist.

First lets talk about reviews.

Commonly we have design reviews with our customers and these are usually mile-stoned as part of the project and will involve prototypes, drawings, presentations, sketches....anything really that can inform our customer what we think we should be delivering.

Less commonly we have internal design reviews and these should happen before any (or much) code is written.

Finally after code is written we have code reviews and these come in two forms; quality audit or collaborative.

The quality audit is done by grim-faced QA types in regulated industries (or companies that favour process over people). I'm going to ignore it as I fundamentally disagree with the approach.

Now let's talk collaborative code reviews. This is where a team gets together as equals and reviews each others code. Done right it's a learning, team-building, democratic, process building and teaching exercise.

We have a document for this. Actually that's a bit misleading, what we have is a document that should be applied prior to the code review. All SSDC documentation is in a repository and can be updated by anyone.

First off we want to do some initial checks.

Notice how we explain why it is important in the details part. My theory is that if you cannot give a good reason for doing it, it should be deleted.

Next we want to ensure the project is set up to our standard.

This helps make sure the project is repo ready. Next we need our project setting up.

The point of a checklist, and the reason we're fond of them is that they help us remember the mundane stuff. That's the project all sorted, now we move onto User Interfaces.

The latest addition to this document it with regard to making user input safe. Again it's a mental trigger. We call standard front panels that aren't user interfaces...

 Then we check out the block diagram.

 Finally we make sure the icons are up to scratch.

I'll reiterate that we could use this and go through the code in the meeting or we could tell the reviewees that we expect these to be sorted prior to the review.

The actual code review can then concentrate on the important things, like complex areas of code, areas that need more explaining (simplifying) and other useful stuff. Hopefully all the attendees will gain knowledge and share knowledge and the team will start to accrue best practices and agreed designs.

In future deep dives will poke around in some of our other documents, it will be a wild and crazy ride!

Lots of Love

Steve

[FabiolaDelaCueva]

Hi Steve,

Thanks for sharing your approach to Code Reviews with us and as you probably expected, a lot of people will have a lot to say about some of the segments in the code review checklist itself... and I could not let you down! So here I go...

As you know, we favor the use of VI Analyzer tests, but we don't favor the blind use of VI Analyzer tests. I think it is important to have custom VI Analyzer configuration files for different purposes. In your case, you could have one for the QA types with everything under the sun they would care about and a different one for your internal code reviews. 

We have worked with several teams that have a very light cfg for daily analysis of their code. Things like spell checker, for example, but not unused code. Because we assume that on a daily work, there will be lots of code that will not be "pretty" or completely in use. 

Another important thing to note is that 0 errors in VI Analyzer tests is not a reasonable goal. I like more for the developers to address as many issues as possible before the Code review. Then at the Code Review, before we do anything else, I ask for a justifications a to why certain issues remain. This is a quick exercise at the beginning of the review and helps us discuss if VI Analyzer is just being too whiny or if we really should be addressing those remaining issues. I get suspicious when somebody tells me: My code is so good, that I get 0 VI Analyzer issues reported.

As far as the project directory structure, I think starting from a project template makes it easier to comply. In the case of DQMH, even the DQMH scripting tools make sure everything gets saved in the expected folders. 

I know we have had this conversation before and I am not going to convince you otherwise, but I really favor more project libraries over auto-populating folders. This provides the name spacing you are looking for when you are "grouping subVIs based on their function". Oh! and it gives you the extra feature of ensuring every VI in the library has the same Icon header. Edit the library icon and set apply to contents and voila, two requirements for the price one 🙂

At the end of the day, the important thing is that you have a process that works for you and your team, you also have a process for everyone to be able to give feedback and you can adjust the process when it no longer meets your needs.

Thanks again for sharing!

Best regards,

Fab

[swatts]

Hi Fab,

Thanks for the time you spend on your replies, it is always appreciated.

I think we're very much behind you on the VI Analyzer stuff, currently it's a pigeonhole to get us to use it. That said I've removed pretty much all of the metrics from the tests.

Libraries are an interesting one. I watch with interest the complaints about LabVIEW projects and how VIs need constant re-saving when a project is updated from a repo and we don't have any such issues. Part of my thoughts on Lean LabVIEW is about over-use and linking of libraries within a project, I have a lot of work to do on this tho'.

I need to have a look at a project that has these problems to try and understand the issue better (sadly over the years we have eliminated these issues and it's difficult to step back)

Steve

[SercoSteveB]

I agree with the point Fab made, if it works for you and has the flexibility to continue working for you, then go for it.  Unfortunately I do not have the luxury of being able to ignore the "grim faced QA types".... or the ISO auditor but on initial inspection I suspect your process would satisfy the code review aspect for both.  BTW killer Initialism, SSDCDDDCRC.

[swatts]

All the procedures here are ISO9001 tested, it's just that some industries think they can inspect quality into things. Inspection should be a check for compliance, to actually improve your teams quality you need something more collaborative. I think a well executed code review is probably the most valuable thing you can do as a team, that value can be undermined very quickly by nonsense checks and over officious application.