I often get sent to sites where I need to do a code review of a system I am not involved in, and once reviewed won't be involved in again. I spend time during the usual tool tests and looking for patterns but at some point you just need to read code.
After the first 5000 or so lines it just becomes impossible to focus on the code, especially since it is business tools and thus not the most interesting solutions. So what tips are their for staying focused on the code during a code review?
So far I am using the following
That's a really tough problem. I guess my question to you would be: What can you do to make it more likely that the first 5000 lines of code you look at are the right ones to look at?
In other words, get the most bang for the buck when your attention is the highest. You'll have to look at the code from a bird's eye view to figure that out.
For the code after that, do a brief overview of it, then write down questions that you want to answer about it. You probably will have some slight worries in your head as you look at code that may only come out if you write down the questions. Then look at the code with the intent of answering the questions.
The strategy should be to get them to walk through the code, explaining what they see it doing. The goal is to a) produce solid, maintainable, correct code and b) learn from doing so. You'll be surprised how much you learn reviewing others' code. Yes the idea of reading out code is PAINFUL, but in practice it's a opportunity for great educational experience, and it gives you ideas of what to look out for in future.
Of course too much of anything is a bad thing, and I've seen developers who have had to do code reviews for 3 months straight. They HAD to take breaks, fix bugs, check emails, jump around - anything that was't reviewing.
Perhaps take this time to learn more about the project, how it works, and it may make some of the boring parts more interesting as they'll mean more to you. Speak to the developers of each module, ask them what it does well, and what parts they're worried about.
And of course, caffeine ;)
Do a certain number of exercises for each block of code that you review (like 20 situps per thousand lines of code, or something like that). You can add little things like doing 5 pushups for every useless comment that you see, or 10 chinups for every duplicate function that you find. Physical activity keeps the mind focused, and given the state of most code in the world, you'll be seriously ripped in no time.
Warning: not recommended for classic ASP. Heart attack and stroke would be unavoidable.
I wouldn't recommend switching context. When you analyze the complex alien code, you have to keep a lot of information in your head. Unlike in OS world, switching context not only takes time, but lets you also forget about something you have just read but haven't memorized.
What really helps is doing something with your hands. Write comments in the code, explaining what it does. Leave notes when you see something unusual. Outline questions and TODO marks to return to them later. Use different colors for them.
This will make you 100% time busy and concentrated. And it also will visualize your progress and cheer you up!
I'd prefer you have an co-reviewer at the same time. This improves both speed and efficiency.
Unless you are very lucky, you should already have plenty of feedback in the first 5k lines of code.
Check this ebook on solid.
Don't focus just on a set of isolated classes. Start from top level usage at first (UI level - if there are multiple tiers go to the boundaries as well). Select a couple important flows, and follow the flow into more internal parts of the system. Select a couple classes that seem likely to have issues that u see along the way, and later do a full review of those.
Pay special attention to the issues with most impact i.e. chatty interaction across tiers, loading large set of db data, doing lots of round trips, big pages / or resources in it. You might want to give limited attention to other items if these are in.
Can you do the review in a top-down fashion? Review the top level (main method?) and write comments on that. Then drill down one step in each abstraction layer and comment on that. Sometimes I drill straight down 14 levels of methods and get totally lost when I come back up. Keeping your review wide may help.
Maybe doing it in an IDE would be best so you can bounce through the code.
Not something I'd envy. I don't like doing reviews with the developers nearby.
As for time management, try the Pomodoro method. That really helps me when I'm facing a marathon coding session.
I believe the best code review is when it proceeds like this:
If the code is in Java, use PMD or CheckStyle. The beauty of it is that you can write your own rules that flag violations that are based on that sites code standards.
Maybe your company should fix the code review process...
I usually traverse through a feature at a time, gui down to db/reporsitory layer.
If comments are not there, I will add comments of what I think is going on.
I'll keep a mindmap/wiki of my understanding of the system. A mindmap tends to be easier to pickup after taking a break.
I would talk about my findings with other teammates.
I doubt any human could extract anything meaningful from reading 5000 lines of code in a day.
My view is that code reviews should be interactive, which also keeps them interesting. If I'm responsible for reviewing someone else's code, I don't care for the approach of having them walk me through it. They might focus on the stuff they like, or what they think they did well, when I'm generally interested in the opposite.
The format I like is for me to sit at a laptop connected to a projector, and display the code on the screen so that I can easily hop from one section to another. Then while reading code on the screen, ask the developer questions: why did you do it this way? do you always do it like that? how did you decide on this structure? where is the unit test for x? -- while also making observations: could use more comments here, this code formatting doesn't meet standards, etc. Plus, having the dev or maybe someone else in the room take notes about things the dev should look at or fix later, with clear instructions that common problems should be fixed in all of their code, not just the code that was reviewed.
The idea is not to look at every line of code. That's a dev manager's job, or maybe QA. The idea in a code review is primarily to double-check the thought processes behind the code, including the architecture, flow, etc, as well as to look at the quality aspects: performance, scalability and security.
If you try to do or look at too much, not only will you burn out, but the results won't be useful to anyone.
You should have some kind of structure in your code reviews because reading the code one line at a time is just impossible.
You should limit the size of the code reviewed for the review to be effective - at my workplace we review before committing the code back to the source control and because we keep our commits small we limit the review duration as well.
During the review I ask questions that will direct me at interesting/problematic places in the code:
Code review was meant in order to learn and teach by improving the code. just reading 5000 lines of code straight just seem wrong...
I agree with the others that what you're doing isn't really a "code review". I think that the most important thing is to keep in mind what you're looking for and focus on that-- literally have a checklist of items and questions you need to answer.
An open-ended examination of a code-base takes a LONG time, and doesn't have a good chance of being effective if your involvement is supposed to be short-lived.
There's a pretty good podcast on the problem of reading code on se-radio: link. It is an interview with Dave Thomas (author of "The Pragmatic Programmer"). I think it applies to what you're trying to do.
Yes in code review you need to read code. But not just 5K lines straight with no purpose or plan. No wonder you can't stay focused.
Analysis tools that produce metrics are what can focus you. Run analysis tools and sort the results on their metrics. Each tool will tell you the "top 5 files, or functions" with problems on a given issue:
I'm guessing that you're being called in to give a one-day assessment of code quality. So do the above and then complement it with focused code reading to find a really clear example of each kind of problem. You won't be reading 5K lines straight, you'll be doing it with a purpose, and in search of a prize. It will not be boring and you will be focused.
Prepare your report with the metrics, references to the tools and concepts, and the examples of each type of problem in the codebase.
All done and they pay you the big bucks for telling them what they probably already knew but didn't want to admit to themselves.