I’m currently working on a brute-force VBScript->C language porting of an existing application. About half the source code was ported by another person at my company who appears to not really understand C as yet–and who appears to have just gone through her files without even trying a basic compile so the same compile error level stuff is repeated (semi-consistently) throughout all the files. Of course, fixing those probably isn’t all that hard (just time consuming) but there’s a lot to make sure is safe in C that isn’t a compile error or warning, meaning there’s no knowing how safe her code is. So I’m going to need to do a side-by-side code review of all of her source just to make sure that a) what the VBScript is doing is what her code is doing, and b) that the code isn’t flubbered up the butt.
So, the question…how to deal with this?
Usually I would try and write a little “Here’s the coding errors you’ve made and here’s why they are bad C code” tutorial thing, since I don’t really view anyone as being incapable to writing in a language, just inexperienced. But, a) I would have to do this in Japanese* which is slow for me and would probably be painful enough to read that she would ignore it, and b) there’s enough disparate errors that this would be too long to seem like a little, “Hey, you have an error here”-type minor issue thing; it would pretty much come off as a slam.
But, I don’t want to have to be sidestepping the issue for the next six months if we’re going to be working on further C projects.
A long time ago I heard about a technique used by SW houses called ‘code walking’.
The idea is to have a number of people reviewing code in a small committee, they are not critiquing style, just looking for errors (bugs). Criticism is flatly banned - it is purely scanning for bugs.
The side effect is that people know that their code is going to be gone over in detail by other people, so they put a lot more thought and effort into it.
Actually I already knew the principle, but putting a name on it was useful, I implemented a variation of the technique and it worked well.
‘Management’ may not like this at first, they may see it as unproductive, but it strikes me that quality control is a lot more important than lines of code.
In your case, you would probably be better off quietly sorting out the mess, there is a danger of humilliating her and it sounds as if she might be overloaded.
Your real objective is to make sure that it does not happen again.
Another technique I found useful was to try to libraryize as much as possible, one programmer would ‘own’ a routine, another would be ‘backup’, everybody can look over the code, but only the ‘owner’ or ‘backup’ were allowed to modify it.
This turned our team into a combination of ‘clients’ and ‘suppliers’, each programmer was both - it was not hierarchical.
The trick is to make people very aware that their code is going to be scrutinized by their peers.
Sounds like someone who has thrown herself in at the deep end (not that there’s always something wrong with that) and is frantically treading water. I don’t think there’s necessarily anything wrong with honestly pointing out that there is some kind of fundamental problem of comprehension - glossing over it is only going to make it come back to bite you even harder on the ass at some point in the future.
But you don’t necessarily have to list each and every error - it sounds like it would be more productive to try to discover the root cause of the problem - perhaps it can be simply resolved by sending her on an intensive training course for a week, or something like that.
Is it possible that she may not have ported the code herself at all, but might be using some kind of automated translation utility? (in which case, the problem is even bigger).
First, requiring that anything handed over to you must compile is not an unreasonable request. Not only will this point out the common errors (the easiest way to point them out), but requiring a clean compile will relieve YOU of having to repeatedly fix them.
Verifying functionality may be tougher. Without knowing the specifics of what you’re trying to do, one thought that comes to mind is unit testing. If you/whoever can create a test environment that exercises each piece of VBScript, then it would seem you could create a similar test environment for the C code and check the inputs/outputs, etc… Not sure how practical this is, but it would go a long way to giving confidence the functionality is the same.
Code reviews CAN be very useful and productive. I’ve been at a number of companies, and it took a WHILE to come across one where the reviews were done well.
The typical code review goes something like this: developer hands out copies of his code to the group, group then starts going line by line through the code looking for bugs. After about 45 minutes, everyone is bored out of their skulls and reviewing starts to degrade.
Here’s the FAR superior alternative:
developer distributes code to be reviewed along with review comment sheet.
reviewers review code (on their own), and fill out review comment sheet by stated date
Reviewers return comment sheets to developer no later than stated date
Developer goes through each comment sheet, reconciles similar items, and either agrees to make change or disagrees to make a change
a meeting is held ONLY to go over the disagreed changes. All the other changes aren’t discussed. It is useful to have a “review lead” (supervisor) oversee the meeting. This review lead (not the developer) has final say on whether a disputed change should or should not be made.
This limits the actual meeting to only the items that need to be discussed, and the meetings can be as short as 5 minutes (or at times, no meeting at all). Reviewers get to review the code at their leisure, so they can do it during builds or what not.