Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

This question already has an answer here:

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.

share|improve this question

migrated from stackoverflow.com May 29 '12 at 8:18

This question came from our site for professional and enthusiast programmers.

marked as duplicate by gnat, BЈовић, MichaelT, Dan Pichelman, Dynamic Jun 17 '13 at 2:54

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

3  
confused? if there is a specific problem making them not work together then you need to investigate that problem, code quality while correlating with bug count will not show you where the problem lies –  jk. May 29 '12 at 8:22
2  
2  
1  
"It works most of the time, but it is unstable". Were you able to isolate unstable cases? Are they repeatable? And more important, can you explain them? I'm not so sure a code review should be your first step. –  Bart May 29 '12 at 9:37
2  
Keep in mind it's perfectly possible for ugly code that violates standards to actually work, and pretty code following all sorts of good practices to still have bugs. –  Servy May 30 '12 at 21:05
show 7 more comments

9 Answers

up vote 16 down vote accepted

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.

http://i.stack.imgur.com/ARBSs.jpg

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.

share|improve this answer
    
+1 for dropping the background. Forgetting the background will make the review much more independent. –  Brian Jun 1 '12 at 16:55
add comment

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?
share|improve this answer
    
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. –  magol May 29 '12 at 9:46
add comment

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.

share|improve this answer
    
VB 6 solved...A great number of problems. I really wish you would get your pure opinion of VB out of your response. It holds no weight and just because you have not seen a high quality program written in VB 6 ( they do exists ) doesn't mean it he is not professional. –  Ramhound May 30 '12 at 11:35
1  
@Ramhound I didn't write any opinions in my answer, they were kept in a comment, apparently deleted. I worked part-time with VB development during a few years, in both VB6, VB.NET and VBA. My opinion comes mainly from my personal experience of designing VB programs. Even with 10+ years of experience in program design, I could not find a way to design quality code in VB. If you have ever seen such a beast, as high-quality VB code, do send me a link. –  user29079 May 30 '12 at 15:55
add comment

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)

share|improve this answer
    
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. –  magol May 29 '12 at 9:46
    
I'd ask what the problem is at least and why the review is necessary –  jk. May 29 '12 at 9:51
add comment

(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.

share|improve this answer
add comment

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?

share|improve this answer
    
Hello! Please don't post exactly the same answer twice ;) I've already undeleted your previous one after you edited it (next time please flag it so we can find out sooner), however since you already went and posted it again, I'll delete the negatively scored version (since all the downvotes were before you improved it). –  Yannis Rizos Jun 2 '12 at 6:21
1  
Thanks, I was unaware of this process and did not find a way to DM you. –  zwartbont Jun 2 '12 at 9:18
add comment

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.

share|improve this answer
add comment

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).

share|improve this answer
    
What does "unless is ROM is" mean? What is "ROM", here? –  David Conrad Jun 1 '12 at 0:26
1  
In computer science, ROM means Read Only Memory. It doesn't mean anything else. And yeah... that doesn't make any sense to me either. –  user29079 Jun 1 '12 at 13:53
1  
In context of planning the time a task takes ROM is "Rough Order of Magnitude" (answer editied). Sorry, I did not think that anyone would be silly enough to think "Read Only Menory" in this context. –  mattnz Jun 2 '12 at 22:25
add comment

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.

share|improve this answer
    
I suppose an engineer didn't like the reality check when being asked to review two unknown code bases for code quality so they can blame one side over the other instead of actually looking into ways of fixing the problem. –  Andrew Finnell Jun 4 '12 at 14:22
add comment

Not the answer you're looking for? Browse other questions tagged or ask your own question.