Review quality of code [duplicate]

Question

I have been asked to quality review two code bases. I've never done anything like that, and need advice on how to perform it and report it.

Background

There are two providers of code, one in VB and one in C (ISO 9899:1999 (C99)). These two programs do not work so well together, and of course, the two suppliers blames each other. I will therefore as a independent person review both codes, on a comprehensive level review the quality of the codes to find out where it is most likely that the problem lies. I will not try to find problems, but simply review the quality and how simple it is to manage and understand the code.

Edit: I have yet not received much information about what the problem consists of. I've just been told that I will examine the code in terms of quality. Not so much more. I do not know the background to why they took this decision.

Answer 1

Drop that "background" out of your head - it's useless for the assignment you've got.

For both programs, do the same: read the code, file by file, line by line. Anything that doesn't feel right, add it to the list of issues. Typos, duplicate or dead code, things that are hard to understand, IDE/compiler warnings, anything. Use free form comments, don't worry about the structure at this moment, just fill the list with what catches your attention.

After done with the list, review it. Re-check if your notes are correct. If you can see some sort of structure / grouping in the issues you noticed, rearrange the list to fit. Submit the list to whoever asked you about it.

^{At past project, my colleague has been told to review a component I was assigned to maintain, 5 or 10 K LOC if memory serves. He went over it and submitted a list of 180 items (one hundred eighty issues noticed). Week later, management gave me a full power and unlimited time to rework that component. I spent about a month, addressed all the items that guy listed. Five years of intensive use that passed after review, there were two or maybe three bugs submitted against this component.}

Answer 2

For me your question sounds like a "blaming game".

Is the issue really the code quality of the two code providers or more about the interface description between the two?

Written requirements or unit tests might help to settle this kind of problems in the future:

• What are valid inputs/outputs what are not?
• How should error management be done?

Answer 3

If your task is not to find the actual bugs, but as you say, simply measure the quality, then you will need a way to measure it.

In C this is quite easy, you can simply check how well the code conforms to a known coding standard. The most well-known and recognized authority is probably MISRA-C, but there is also CERT C. Either can be used, though the former is more suitable for high integrity systems and the latter for PC programming. Both standards focus on banning dangerous practice.

From such a coding standard you can pick a number of rules that should be part of the review. For example, it would make a lot of sense to include all MISRA-C rules that ban reliance on undefined behavior. As a result, you should get a number of rules which can be used as a mean to measure quality.

Then comes the actual review. You would certainly need to do it manually, but the professional way would be to also use a static analysis tool. There are plenty of them supporting MISRA-C, and some supporting CERT C. The fancier tools can also find various other bugs such as data anomalies, buffer overruns, needless complexity etc ect. They can also perform code coverage, to see if there is any code in the program that will never be executed.

As for VB, that is far harder. There is probably no widely-recognized authority you can cite, although there are perhaps coding standards you can use, maybe this one? (found it in Google, I've never used it). Microsoft would be the best authority to cite, they seem to have some vague, shallow guidelines for how to write VB, but those won't be anywhere near as professional as the C coding standards.

Answer 4

Sounds to me like what you really need is some instrumentation to sit between the to applications and spy on what messages they are passing each other (depending on how they communicate this may be something you need to write your self or something already available like wireshark)

once you can see the messages you can decide who is right and who is wrong compared to the interface spec. of course its possible the spec is ambiguous in which case you either need to fix the spec (and possibly incur more costs for changes to one of the components) or convert your spy tool into a compatibility shim

reviewing their code is certainly not without its merits but it will not help you trace interface issues (especially if the spec is wrong)

Answer 5

(Regarding the code, just dive in - it is the only way to learn it).

I would suggest you look into having an API for the two parties to work across (a contract essentially). If they conform to the API then the code should work together.

Very much like how it is done with a WSDL in the Web Service world.

If at all possible then have a test suite on the API so you can see that stuff works. Otherwise you can play the blame tag game for years.

Answer 6

Doing code reviews in a way which is acceptable for all parties involved is hard. A good process will help, as well as an objective standard, which together ensure the review is verifiable and repeatable. Also, analysis and getting the technical facts is one, reporting in a way that resolves the problem situation, including managing the different perspectives, positions, feelings, is another.

Visual inspection and making a list of all things that are wrong with the code is typically only a good idea there are no time and budget constraints or the code base is really small. Otherwise, the inspections themselves might just be too much work, or the corrections that you would suggest could be too time consuming to perform. Please keep this in mind when you come with recommendations.

What research questions does your client have for the code review and what will be done with the results?

Answer 7

It really depends on what they want and mean by code quality.

But in general, your goal would be to give a review of how the code fares in terms of:

• ease of maintainability and extensibility,
• stability and security,
• performance.

Pretty much most of the metrics fall under these porte-manteau categories (actually, I'd not even consider performance in general, and consider it most of the time as a bonus side-effect of good scores in terms of stability and maintainability, hence good code design).

There are some more absolute things to look for, like code certifications that can be directly applied and measured on some codebases, but they don't necessarily give a fine-grained review of these categories.

I'd advise you:

• open an IDE and start browsing the code,
• put breakpoints to know what happens at runtime and identify interesting cross-cutting concerns and major bottlenecks,
• find some code checkers and have them flag violations and identify "hotspots" in your codebase,
• do some profiling on the application to identify bottlenecks,
• also have a look at the tooling used and the system's build (not what they directly asked, but some can be inferred from that, and it can help to identify other issues),
• define process and practices to then start improving the metrics you identified, and explain how that will benefit the codebase, the developers, and the end product.

I know you tagged vb, but just to give you an example, Sonar would be a good example of a tool that can greatly help you quantifying the quality of a code base in many regards, from code quality from a technical point of view or even from a business perspective, by linking (more or less accurately) some markers to business costs. You want to find an equivalent tool, or a set of tools allowing you to achieve similar results, in your area. Sonar does have a C plugin, if you want, and I'm pretty sure Visual Studio has a few good code quality analyzers built-in or external, for VB.

Answer 8

How much time have you been allocated - unless is rough order off magnitude is 20% - 50% of the development time for the apps you are reviewing, you are wasting your time and performing a political exercise to make someone feel good about it.

Most of the suggested things to look for are (although the best we know of) an inaccurate way to determine software quality, and very unlikely to standup up to rigorous scrutiny. Consider what you write in your report carefully - you may end up having to defend it in a witness box - how far is this dispute going to go, and how much money is at stake, are the parties wanting to find resolution or blame?

Assuming you are a programmer, not a forensic analyst with a programming specialization (In which case you would know not to have asked this question in a public forum), if I was you I would refuse the assignment in the strongest way I could. You are being set up to take a fall - playing software vendors off against each other with nothing but your opinion..... If one of those vendors finds this Q&A, how will that make you look....

Before you complete you report, you will need to be certain what it is going to be used for. Make sure it is phrased with statements such as "it appears" and "I believe" rather than absolutes such as "the code is crap". If you must make assertions - reference to published and widely accepted standards will be required. e.g. "The code does not meet the standards specified in the MISRA guidelines for C" is a fact, but stating that is should meet those guidelines would not be (unless your contract said so or they claimed it did).

Answer 9

Roll a 6 sided die. 1-3 is the VB code, 4-6 is the C code. Take a 2 month vacation and tell everyone which code is bad after you return. You are in a political fight it would appear.

Or better yet, tell them you are going to review the code, but don't. Come back with working fixes to both sets of code that allow them to work together.