25

I have been given the role to improve development in our company. The first thing I wanted to start was code reviews since that has never been done here before.

There are 3 programmers in our company. I am a web programmer, my known languages are mainly PHP, ActionScript and JavaScript. The other 2 developers write internal applications in VB.net

We have been doing code reviews for a couple weeks now. I find it hard to understand VB code. So when they say what its doing, for the most part I just have to take their word for it.

If I do see something that looks wrong, I explain my opinion and I explain how I would address it in one of the languages I know.

Sometimes my suggestions are welcomed but many times I am told things like "this is the best way of doing it in this language" or "that doesn't apply to this language" or similar things of that nature.

This may be true, but without knowing the language I am not sure how to confirm or refute these claims.

I know one possible solution would be to learn vb so I can do better code reviews. I really have no interest in learning vb (especially since I have a list of other technologies I am trying to learn for my own projects) and would like to keep this as a last resort but it is an option.

Another idea that came to me is, they both have interest in C# and so do I. Its relative to them because its .net and relative to me because its more similar to the languages I know. Yet it is new to all of us. I thought about the benefits of us all collaborating on a pet C#.net project and reviewing each others code from that.

I guess theres also the possibility hiring a consultant to come in and give us some code reviews.

What would you recommend I do in this situation.

3

12 Answers 12

24

Your personal desires to learn other things should take a back seat to learning what you actually need right now for your job. Learn VB.net. You can effectively code review code you don't understand when you know the language it's in by asking lots of questions (usually that's a sign the code isn't well written if you know the language and can't figure out what its doing and why). But not understanding the code, the best you can do is get them to explain it to you and hope they will see any bugs through the process of explaining it. Not that I haven't found bugs in my own code in a review by doing just that, but it isn't the most effective way to code review. Code review is now part of your job, deal with it and learn what you need to learn to do it effectively.

While you are learning, when they say well that isn't the way we do it in this language, make them show you a source that says it is a good technique to use. It's up to them to justify to you in a code review not the other way around. You'll also get better at the language once you start seeing those links.

0
13

Actually, I disagree with all the above. With JS/PHP/ActiopnScript, you have a fundamental understanding of what a programming language has and how it works. In fact, I would argue that there are a lot of similarities between VB and JS. However, that's not my point. Even if you are very competent with the language, it's easy to overlook something when trying to follow someone else's thinking processes, so what the review should do is provide an opportunity for the programmer to explain what (s)he has done and why.

A friend once described this as "The Janitor Theory": by explaining the details to someone, anyone, even the janitor, the programmer exposes any weaknesses in the code to her/himself, which is, of course, the ultimate goal of the review process. It does require, though, that the code be explained thoroughly and openly (reviews don't work when the developers are defensive).

0
8

The short version

  1. Remember that code reviews are a chance for both the reviewee and the reviewer to learn.
  2. Phrase feedback as a question.
  3. Don't let lack of knowledge stop you from providing feedback (as long as you are doing #2).
  4. Avoid "preference reviews" or at least try to make it clear that they're your own personal preferences and they don't necessarily need to agree.
  5. Try to submit patches instead of being an "armchair code reviewer".

The longer version

First of all, remember that code review isn't just an opportunity for the reviewee to learn. It's also an opportunity for the reviewer to learn as well. In fact, I've heard of several organizations that make new programmers start out doing code reviews so they can get a feel for the code.

With this in mind, there's one piece of code review advice that I've always found useful in general, but is especially pertinent in your position. Phrase your feedback in the form of a question rather than as a statement. In other words, instead of saying "This code sucks!", you could say "Why did you write the code this way instead of doing ..." This makes the code review process more pleasant and allows you to learn as well.

Another piece of advice I have for you is not to let your lack of knowledge make you back down. If you see something that you perceive as wrong, and you get a hand-wavy answer from the reviewee, don't back down (at least not due to lack of knowledge). Remember, what makes good code in one language is rarely any different from what makes good code in any other language. Yes, certain languages have different idioms to help you write good code. But it's important to realize that those idioms are tools rather than ends in themselves.

Next of all, try to avoid doing "preference reviews". This is something that I (and plenty of others) have to make a very conscious effort at. In other words, try to avoid doing reviews that are along the lines of "You did x, but I prefer y". Now, there isn't anything wrong with stating preferences, but you should clearly label them as such and make a note that the other party is free to disagree. This is important, because most things that are different from language to language fall under this category.

Lastly, do you guys use a distributed version control system? One thing that might help is if rather than just noting what's wrong with the code, you could rewrite the code how you would have written it, test it, and then submit a patch for it. This helps show that you're genuinely interested in improving their code and not just being an "armchair code reviewer" and gives you a chance to learn the language better. Plus, it's usually easier to disagree with "I think you should do this" than it is "Here's how I would have done it, and here's a patch if you agree". I suppose you don't necessarily need a DVCS for this, but it certainly helps.

1
  • About the "preferences": Imagine I wrote the code, you reviewed it, and I had to change it because of your preferences. Now you make a small change, I review it, and I make you change it all back because of my preferences. It should be obvious that this is extreme nonsense.
    – gnasher729
    Jan 18, 2016 at 8:59
8

You've lost focus on the problem and came up with a poor solution. You've been given the task of improving development and your solution is to put a person in charge of code review (yourself) who doesn't understand the language. Who is reviewing your code? Why can't they review each other until you learn the language?

There has to be some other problem area to could have selected to have a more immediate improvement. This way they just blow smoke at you and nothing improves.

Directing new development to a language that none of you understand (C#) is going to take a long time to pay off especially if you everyone brings their bad habits with them.

Focus on design (That's been mentioned.). If they are having difficulty maintaining the current code, look into some refactoring tools for VB. Many of the basic practices are the same.

5

You can 'proof read' stuff that you don't really know, but you can't adequately review it. I'm highly competent in C, know C++ rather well, but I would not dream of reviewing something in C#.

I don't think you need to go as far as bringing in a consultant, as some companies specialize in running your code though a ton of tests and telling you what might be wrong with it.

Still, it would be up to the individual developer who knows the language to interpret the result. For instance, if a code reviewer dinged me for not using the return value of printf(), I'd look at them strangely and question their sobriety, then ask "Ok, great, what can I do when nothing can be printed to the console that would be helpful?"

What you might want to consider is talking to your bosses about setting up departments and team leads, so that you can be effective in your domain, while someone else is effective in their domain.

Still, I think you might be able to use a third party for auditing. Most programmers worth their salt will pay attention to legitimate concerns, even if they dismiss half as bogus (such as I would in my printf() example).

4

Providing guidance on something you do not understand is akin to the blind leading the blind as you are well aware.

One approach would be to make use of lint tools such as FxCop and StyleCop which will address the static analysis front of the code base. This would provide you with a starting point to debating the reports which are generated from the tools.

Another approach would be to turn the code reviews into design reviews. Design reviews more often then not uncover problems long before the code is even written. If a programmer has a design they can work from they are in general much more efficient in their approach and bugs decrease because of this. When a design is non-existent it becomes adhoc in the approach and the code suffers with the bug count increasing. Catch the problems in the design review before they surface in the code review by making sure each of you has a concrete design to implement; UML is your friend here and tools such as umlet are fast and quick to use.

0
4

The bad news is that in order to participate effectively in code reviews, you will have to learn VB. It will also be helpful to use VB in some sort of project (not necessarily for production).

The good news is that once you have done this, some of what you've learned will still be useful when you move on to C#.

9
  • 9
    Reading VB is not the same as Knowing VB. I read VB well enough to rewrite old VB code into Java. I don't (and can't) write VB. I think there's a middle ground of learning enough VB.
    – S.Lott
    Jan 5, 2011 at 17:56
  • 1
    @S.Lott - well articulated and quite applicable to any two random languages.
    – user131
    Jan 5, 2011 at 17:58
  • 2
    @S.Lott: If you can read VB well enough to rewrite it in Java, then you do know VB, and can write it. You may have to look up things as you go, but that would only last a couple of weeks. Jan 5, 2011 at 18:01
  • @Larry Coleman: I'm guessing you know VB pretty well. I couldn't write it. Really. I'm a Python/Java programmer and the limitations and weirdness of VB confuses me. A lot. I wouldn't just be looking up syntax. I would be pretty well incapable of writing proper programs because I just don't seem to think that way.
    – S.Lott
    Jan 5, 2011 at 18:05
  • @S.Lott: I do know VB quite well despite my best efforts to forget. If VB's weirdness/limitations are confusing to you, wouldn't that also cause issues when porting code to another language? Jan 5, 2011 at 18:33
3

The thought that you should keep at all times when doing peer review is:

"I AM THE NEXT ONE TO MAINTAIN THIS CODE!"

You must understand it well enough to be able to do so as part of your preparation to the review, and your job is to make the original programmer aware of any deficiencies that made it more cumbersome than necessary to understand the code well enough to maintain it.

If you cannot program in VB, you cannot maintain the code, and you are not qualified to be the peer reviewer.

1

A good code review is about things that require you to understand the language - appropriate use of the language, APIs and libraries, style, variable names etc. - and about how well the code solves the problem - good comments, proper architecture, relevant design patterns, considers all the error cases, etc. When you first start doing code reviews you tend to concentrate on the former. They are easier to see and easier to pick on. (e.g. I don't like your variable names. You should use XXXX style names.)

Code review becomes more valuable when there is more focus on how well the code solves problems. Since you can't provide as much value in the first area now, concentrate on asking questions and offering advice about the solution to the problem, not on how it's made.

Of course there is overlap between the two. Knowing VB.NET would let you provide advice on why a certain design pattern isn't a good choice in a particular situation for example.

Above all, be humble at this stage. Changing process is tough. Even if you were a VB.NET guru the change likely wouldn't be easy. People who haven't used code review don't like it at first. Having others look at your code is a tough experience. It takes time to see the value, and patience all around. It's a great process when you get buy-in but it will take time.

1

You shouldn't review code you don't understand, that will only annoy the developers who have to explain every strange looking thing they have done.

What you can do choose/define coding guidelines and check the code against these guidelines. When something doesn't comply with the guidelines you can ask the developer for explanation.

I would start with choosing existing guidelines (I don't know any VB.net coding standards but google gave me:

Use stylecop like tools for VB .net

Analyse the sources with NDepend (it has rules about cyclomatic complexity, lengths, depths etc)

When you have done that you can say that the code complies with the chosen standards, it doesn't say anything about the functionality being correct or the code using proper OOP principles. But at least it is something.

0

Could you shift the focus to be more on tests rather than looking at the code directly? I'm not saying abandon the code reviews, but initially it may make more sense to get those internal apps to have enough tests that can help decode some of what is happening. The idea here being that the tests could also help get you a bit more familiar with some of the functionality. I just see this as a different route to take. The idea here being that the reviews come back later and can be done in a couple of parts as it may be worthwhile to have an overview/upfront session and then a bit of a break. That break being until the next day or two so that there is enough time for anyone that may want to get a night's sleep thinking about the code or something similar before coming back with questions and having a discussion. Granted this may not always be realistic, it may be worth trying and see what happens.

Course if you already have tests this isn't that meaningful unfortunately. The other thought is to give an example of where they are claiming that in VB.Net this is done a particular way, as that may help make this question more clear in a way as I could imagine this varying from little code standards to part of the heart of how VB.Net was built in a sense.

0

Even if you learn the basics of VB, performing a code review while not knowing all of the features of the language will not enable you to detect the usage of insecure features in the language.

Suppose that you weren't aware that the input() function in Python 2 actually evaluated the input before returning it, in order to make it easier to parse non-string input types. Then the code would be vulnerable to Arbitrary Code Execution, allowing the user to enter something like __import__('os').execl('/bin/sh', '/bin/sh') on a Linux system to turn the Python process into a shell. Instead, raw_input() should be used to get the unprocessed input data.

Attempting to perform a code review while not aware of all of the features of the language could not only prevent you from realizing a better way to do a certain procedure in the language; it could also lead to detrimental security flaws.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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