Archive for the ‘General Programming’ Category

Reviewing code the wrong way   Leave a comment


Most companies today mandate a code review. It makes sense: while writing a substantial feature might take a week, reviewing it might take an hour; with a fraction of the time investment, you could in theory double the number of eyeballs that approve of the change, all the while focusing on eradicating new bugs or rewriting the change so as to avoid future bugs. Moreover, the reviewer could be a much more experienced engineer, which might not have the time to code this feature by herself, though it would have probably taken her less time.

Only at Google (I love calling out positive experiences) I ran into a thorough, thoughtful, positive, exact code review process [with the exception of code styling]. Once everyone’s in agreement on how to do reviews well, it’s easy to make sure a new engineer falls in line. However, it’s so much easier to not do them well. Let me categorize the reasons for having a sub-optimal code review process into three types:

1) Hard to track code changes – the developer did something wonderful, perhaps solved a really complex bug in a single line of code change; however, he then went and refactored a bunch of files and renamed some methods, all in the same change. Now, the reviewer cannot really appreciate the beautiful bug resolution; on top of that, the reviewer might actually focus on making sense of and approving the refactoring, totally missing the complex bug situation. Actually, it’s more likely to be the reverse — you wrote some piece of code, introducing a rather complex bug, which might have been discovered by a fresh pair of [professional] eyes, were it not for you adding many other trivial changes, causing the reviewer to miss the important mishap. This category also includes misuses of tools, such as doing git rebase during a code review — simply put, git rebase rewrites your changes, causing the reviewer to lose track of what she already reviewed last time; don’t blame her if she doesn’t invest the same amount of effort re-reviewing the same change again. Lastly, sometimes the difficulty to thoroughly review the important parts comes from your current coding practices. Let’s say a test needs a fixture, which is, say, ~ 200 lines of a json file; don’t expect engineers to actually review this fixture; and the more such fixture files you collect, the less confidence you should have in the tests; I’d recommend thinking how to change the tests to not have to rely on such occasional haphazard code drops.

2) Superficial reviews – maybe it’s ego, maybe it’s being vocal, maybe it’s being too green, but many people devolve code reviews into nit-picking about code style (as an extreme example). Obviously, this is not why we’re investing precious time going over code changes. In fact, such tasks should be automated: code style ⇒ automatic code formatting, common mistakes ⇒ lint checks. Instead, one should focus on the crux of a change — is it needed? is the solution sound? is it written in a maintainable way? is it well tested? Commenting about smaller issues is okay too, but start with the big things; if you drop 20 comments on minute things and once on an important part in the same code review, the developer might spend a lot of time fixing up the small things, only to discover later that they have to throw it all away because of the big problem; frustration is how you get people to stop caring about or even avoiding code reviews. What’s worse, the developer might miss or down play the importance of the one big thing, which is likely to come back to bite your future selves.

3) Not taking feedback seriously – a senior engineer reviewed your changes and left one comment: “this will blow up in production”; you read it and comment back: “I will fix it in a follow-up change”; silence ensues. It’s really difficult to know whether you really meant what you said. If the reviewer is adamant, they might make you do it now. But there’s a chance this will slip up. There are other permutations of this situation: “I will add tests later”, “great idea, I’ll create a ticket for it”, “I have added a TODO comment“, etc. This is endemic of putting too much power, the power to take shortcuts, in new engineers’ hands; you should nip this behavior in the bud — it could lead to senior engineers losing trust in code reviews. Making code changes without test coverage and then following up with tests is okay once or twice if business deadlines deem so; but this shouldn’t be the norm; it’s really difficult to anticipate whether the test will eventually be coded; or perhaps having the test now would discover a big problem before users run into it. Like in our financial lives, technical debt accrues with vengeance. If this behavior comes from veterans in the organization, you have an even bigger problem.

On the other hand, it is also important to know when to skip code reviews. For instance, when someone reformats the entire codebase, let her do it as a separate change and only skim the changes (you should probably put more time into reviewing the formatting rules and not the outcome of invoking them). Finally, doing good code reviews is not in all companies’ interest — perhaps for a start up making mistakes carries less weight than to a more established company as there’s less financial implications, less user’s trust at stake, etc.

Epilogue: Do code reviews, but do them well. If you see problems, bring them up to decision makers, for hopefully investing the time in fixing these. Much of this is education, which should be an utmost priority, especially for the low price of reviewing code (i.e. 1 hour of review for 40 hours of coding). Lastly, when you see something, say something. Till next time.

Advertisements

How to Use Properties to Better Encapsulate Your Code?   Leave a comment


Download ExampleDifficulty: easy moderate challenging

In this article I will explore the usage of properties in order to simplify, bulletproof, and even better encapsulate your code. These are not merely slogans. By the advanced use of properties [and categories], you’ll see how we save coding time, write less implementation details, make our code less error-prone, and make other classes who use our class more independent.

 

The standard use of properties: to create fields your and other classes can modify and access was thoroughly explained in my previous post: What are Properties and how do they work?. Instead, in this article, we’ll deal with three distinct types of variable usages, whose names I made up, and standardize their implementation using properties. We’ll start with Internal Variables – variables that are to be used only within our implementation, we’ll continue to Seemingly Read-Only Variables – variables that are read-only to all classes but our implementation – and lastly, we’ll discuss General-Specific Variables – variables that are exposed to other classes as a superclass of what they are, whom merits will be explained. We trouble ourselves with coding these cases using properties in order to exclude any implementation details from our code, for instance, by not using retain or release at all. Plus, since the memory options of properties (retain, assign, and copy) are easily changeable from one to another, it will allow for faster and less-buggy code changes later.

Read the rest of this entry »

How to Properly Encapsulate Your Classes in Objective-C   1 comment


Difficulty: easy moderate challenging

The main task of a programmer is not simply to create code that works, but perhaps even more important, to create code that’s manageable, adaptable to new requirements, and allows easy debugging. These qualifications are intended to help programmers, either the original coder or his or her colleagues, to better support the code; the CPU doesn’t need any help – it knows precisely how to handle all instructions, be them cluttered or neat. So, to be able to support those requirements, we strive to divide big chunks of code into separate files, functions, and lines.
Read the rest of this entry »

Integrating Your Source Control System with Thy Bug-Tracker   Leave a comment


Difficulty: easy moderate challenging

In our day and age, working with a source control system and a bug-tracker is a must. The benefits of using each are clear and enormous. Even when you work on a small project alone, using one or even both options is useful. Combining them is extremely valuable, as you’ll soon discover.
Read the rest of this entry »

To use or not to use: Assertions   Leave a comment


Difficulty: easy moderate challenging Too many times I found out that programmers rarely and somewhat incorrectly use assertions. Hopefully this quick guide will help you understand how to avoid being one of those. As a rule of thumb, a programmer needs to put guards around his code to take into account inappropriate input and act accordingly. Sometimes, he might run into corrupted data from the network, invalid input from your user, or even getting his objects into unintended states. All of these should be somehow guarded against. He can use if statements accompanied with return, exit, pop-up alerts, throwing exception, or working around those issues. But, sometimes, these guards can be redundant, excessive, and not likely to happen in real life. Read the rest of this entry »

%d bloggers like this: