Rescue Job (Code Review) - Process Description

[swatts]

Hello My Lovelies,

2 or 3 times a year we get asked to rescue a project that's gone awry and I thought it might be interesting to discuss our process and highlight some common situations that arise.

I usually starts with a phone call in which the project manager will say....."it's nearly finished, can you come and review the code as the developer(s) have left and we have a deadline in 2 weeks"

And this is a red flag.

Good developers don't leave just as the project gets interesting!

Next we need to know what we are reviewing the code against.

For me, being a practical sort, I want to see the entire system working first. 

Here's a list of questions you can use to establish what you are reviewing the code for. 

Question	Weighting (1 least, 5 Most)
How operational is the system?
How much software maintenance will need doing on a regular basis?
How many software changes will be expected on delivery?
How many software changes will be expected during its lifetime?
Is being added to important?
How risky is the project? (legal or safety)
How close to or past the deadline are we? (Timescale)
How important is ongoing system support?
Cost (how much of the budget is still available?)
How much spare capacity in the system? (Performance)
How important is robustness?
How close to meeting customer requirements is it?
How many deployed systems will there be?
How difficult will support be?

Next we need to look at project management, generally there are 2 scenarios here (and many places in-between)

Incumbent Team

Here you have the additional difficulty of criticising the work and management of people to their faces. This is never easy and rarely successful, because there is an investment in the status quo. If the status quo was working we generally are not called in.

New Team

There will usually be very little domain knowledge, but they will be more willing to make the hard choices.

A final consideration is that as soon as you (or your company) does any work on the code-base you will own and be responsible for 100% of the code-base. With this in mind you have to think about the impact this will have on your business, because maintaining someone else's code is unpredictable, expensive and usually very frustrating.

Brutal Honesty

It's easy to blame the unscrupulous developer for lying on their CV, or just being a bit useless, but in our experience the main culpability lies with poor project management and lack of oversight. Generally we sit the customer down and explain to them these failings and describe how we don't want to work in that fashion.

We haven't even looked at the code yet?

If the customer is still serious about sorting the project out, we're ready to go...

The next stage is to apply some simple engineering process....

Project Clean-up

Before we look at the design we need to ensure the project is in a state to be engineered.

This means that we need to know what version of the source code we're looking at. 

LabVIEW Life Lessons #6 - Versions

Next we need it portable (or under Source Code Control it depends on the work involved)

LabVIEW Life Lessons #1 - Project Portability

Now put in Source Code Control

The we tidy the project

A Tidy Project is a Happy Project

Finally we can look at the design.... Generally our bias is either a light-touch (if the code is redeemable) or a rip out the technical bits approach. The amount of work we do depends on the results of the survey questions earlier in this article.

With the architecture of the system we follow some design rules or as Fabiola puts it here is where we find the meatballs in the spaghetti.

We always start with Coupling, Cohesion and Information Hiding. Coupling is usually the main cause of project designs going badly wrong. The common factor here is overuse of Globals, Locals (on massive block diagrams), FGVs, Classes used to hold data and references, of clever tricks to hold global data and references (DVRs, Files, dBs, Queues, Shared Variables).

Next we look at external dependencies - to libraries, files, MAX etc etc

Cohesion and Info Hiding is mainly bagging up of like functionality (and shared data), encapsulation is what the cool kids are calling it.

Information on this is scattered all over my blog, go look it up.

All of this heart-ache can be solved at a fraction of the cost by following these simple practices...

1.  Employ good engineers (not good software engineers...GOOD ENGINEERS!)
2. Ask them regularly how they are doing and get them to show practical demonstrations that they are progressing as promised.
3. Sack them if they lie or are evasive.

Oh and don't expect us to sort out your crappy projects on a fixed price.

Lots of Love

Steve

[joerg.hampel]

+1 for the brutal honesty part, to your customers just as much as to yourself. 

[PiDi]

Don't forget the option to trash all the code created and start from scratch might also be valid in some cases. That doesn't mean falling into "not invented here" syndrome and just always start from scratch. But sometimes it's the best course. One of the project I've been rescuing was left by the only LV developer after 2 months of work and 2 weeks before the deadline. It was some kind of production line tester using external instruments, some PLC communication, data logging... No rocket science. But for this relatively simple application, he left behind some incomprehensible actor-framework bloat. After the requirements analysis it definitely looked liked there is no point in continuing with existing code. So, start from zero, after few days we've got functional version, after some adjustments in the next few days we've got working application.

The point is: start with the requirements and some design, as if you're starting any project. Treat existing code as some kind of asset/resource that is available (just as any toolkit, framework, device driver, etc) and see if it fits.

[swatts]

Abso-bloody-lutely!!!

[FabiolaDelaCueva]

Excellent post as usual.

I was getting ready to write about "extracting the meatballs from the spaghetti" when I read that you had already included it, thanks for the mention.

I agree with PiDi, definitely starting from scratch is indeed an option. 

My favorite part was the brutal honesty. Say what you mean, mean what you say. If you are always honest with your customer, they will trust you. Growing up, my parents would say "do you want a pretty lie or the explosive truth?" followed by "you should always go for the explosive truth, at least you start with the clean up sooner. The pretty lie just delays the cleanup and makes the mess much much bigger"

On your question of "How operational is the system?", we ask for them to do a demonstration where they run the application and show us what is working and what is not working. When we decide to work with them, the first thing is getting the code to work as they showed us, but on a different machine. That way we can find out not only dependencies and how portable the code is but also any "clever" things like... wait for it... VIs in vi.lib, instrument.lib that have been modified in that machine! 

Thanks again for sharing your wisdom with us.

Fab

[Taggart]

The demo is a great idea Fab.  Usually, customers are like "Oh yeah it works" but then when you dig deeper you find "Well that feature never really worked, and sometimes that other one doesn't work and this one requires a workaround and this other feature causes it to freeze, etc"