Definition: A merge request becomes “Stale” when subsequent (accepted) merges would result in a conflict with the stale merge request.
Zero is OK for a “trivial” change. What’s trivial? It depends:
One is the “normal” level.
Two is appropriate for projects, or merges, that are deemed “critical.” That word is, of course, subjective. One example: a hot fix for a just-discovered bug, that’s going to ship today. The more eyes on it, the better.
Three is probably excessive. (We don’t want merges to go stale waiting for the third approval.) Also, the more people required to examine a merge request, the more likely someone asked to approve will be unfamiliar with the project. That’s not necessarily a Bad Thing™, but a certain level of engagement with the team will give the reviewer more interest in getting things right.
Five is right out! Diminishing returns and all that…
(For remote teams, “over a Zoom call” still counts as “in person.”)
Here’s how not to do a code review: NCIS 2 IDIOTS 1 KEYBOARD
That said, it’s often better to do your reviews in person, so the reviewer can ask the code owner questions interactively. Just leaving comments in the gutter of a merge request can often lead to misinterpretation (and even hurt feelings (developers do have feelings, right?)). This also helps keep merges timely.
One more thing about “in person” reviews: modern IDEs are very good at static analysis, which can find many types of bugs for you. While the reviewer has the code up in Github (or whatever tool you’re using for merges), the author can have the code up in the IDE, sharing any warnings that appear. Or the reviewer can look at multiple windows, with eyes darting back and forth; these IDEs make warnings easy to see at a glance.
Some of these are contrived examples. Some are from actual, production code. Please don’t let these pass your code review.
Sometime in the mid-1990s, I stumbled across the following production C code:
unsigned long reverse(unsigned long theWord) {
unsigned long result = 0;
int i;
for (i = 0; i < 32; ++i) {
if (theWord & (unsigned long) pow(2.0, (double) i))
result += (unsigned long) pow(2.0, (double) (31 - i));
}
return result;
}
We won’t even mention the assumption that the type long is 32 bits; I’ll just assume that this was correct for the time and place. Also, I’ll be kind and assume the author understood floating point math but also knew that integral powers of 2, from 0 through 31, actually can be expressed exactly as doubles. But I will not accept that the author thought this “algorithm” was more efficient than bitwise & and | operations or shifts.
[For what it’s worth, you can find this code snippet on a website called Computer Stupidities. Plagiarism? Not really, because this was my contribution. – Jim H]
Many years ago, I was interviewing at a financial data firm in New York. In my first round, the interviewer handed me a printout of a C function, and asked me to give it a code review. His next sentence was, “Don’t worry, I didn’t write it, you won’t hurt my feelings.” Philip, I think you were being modest, because this function was a stunningly beautiful work of art, in the medium of awful code. It included the following for loop:
for (a > 5; a < 100; a++) {...}
…which was the very first thing that caught my eye. “Uh, that initialization clause (a < 5) does nothing.” (I got the job, by the way.)
We often put debug or trace logging in code – but sometimes we don’t think things through. Consider this Java code:
private void logArray(Object[] array) {
int counter = 0;
for (Object obj : array) {
LOGGER.debug("object {}: {}", ++counter, debugCalculationOnObject(obj));
}
}
// debugCalculationOnObject might take a loooooong time
private String debugCalculationOnObject(Object obj) {...}
OK, so, I wanted to see what was going on with that array, and it took a long calculation to do it. Now, my code is working, and I want to drop my LOGGER level back to INFO. But logArray(Object[]) is still going to take a long time to do absolutely nothing.
Here’s the revised method, that does the Right Thing™:
private void logArray(Object[] array) {
if (!LOGGER.isDebugEnabled()) {
return;
}
int counter = 0;
for (Object obj : array) {
LOGGER.debug("object {}: {}", ++counter, debugCalculationOnObject(obj));
}
}
// debugCalculationOnObject might take a loooooong time (and might not!)
private String debugCalculationOnObject(Object obj) {...}
Beware of logging inside loops!
isDebugEnabled() will exit the method if debug tracing is turned off. One could just as well call LOGGER.isDebugEnabled() just before LOGGER.debug(); and one should, if any extra work happens as a result of calling LOGGER.debug().
There are similar methods on Java loggers for isTraceEnabled(); isInfoEnabled(); etc. Check for the level you're actually logging. (If you need to know the order of levels, it's Fatal, Error, Warning, Info, Debug, Trace. Setting a log level enables all previous levels.)
Consider autoboxed numbers in Java:
Integer first = 256;
Integer second = 256;
if (first == second) System.err.println("they're equal");
else System.err.println("they're different");
Perhaps surprisingly, this code will print "they're different" to the console. That's because == is actually comparing the two object references–and they really are different. If we instead write:
Integer first = 256;
Integer second = 256;
if (first.equals(second)) System.err.println("they're equal");
else System.err.println("they're different");
we will see "they're equal," because Integer.equals() actually tests the boxed values.
There is a similar (but not identical) problem in C++:
string first = "A funny thing happened"
string second = "Because I say so!"
if (first < second) cerr << "obj-obj: as expected" << endl;
if (first < "Because I say so!") cerr << "obj-stringliteral: as expected" << endl;
if ("A funny thing happened" < "Because I say so!")
cerr << "<we have no idea what happens here>" << endl;
What is going on here?
All this said, we could write operator<(const char *, const char *), and have it do the right thing. Please don't do that! Your reviewer (and your successor) will not expect that comparison to work, if she knows the rules for pointer comparisons in C and C++, and won't know to look for that operator overload.
Let's revisit the "primitives vs. objects" issue from Java. In Kotlin (in the JVM), declaring a var or val to be Int will mean it's treated as a primitive. A var or val declared to be nullable (that is, Int?) will, instead, be treated as the boxed value.
val first: Int = 256
val second: Int? = 256
val third: Int? = 256
if (first == second) println("equal")
if (second == third) println("still equal")
Yes, comparing two boxed values in Kotlin will work. Why? Because Kotlin uses operator overloading. In the case of comparing two Int? objects, the == operator will call second.equals(third), which will return true.
We love our design patterns, those repeatable architectural solutions for common problems. While all code is repeatable, and therefore could be called a pattern, not all patterns are worth repeating. The bad ones are “anti-patterns.” Here are some common anti-patterns (but see also the Anti Patterns Catalog):
Remember the SOLID principles? The S in SOLID is for Single-responsibility. A God Object is one that violates this principle. Consider an object design for a vehicle. What features do vehicles have? Some are specific to particular subclasses of vehicles.
It’s true, all of those items could go into a Vehicle class, but we don’t really want to carry a lot of those around for (say) a sports car; and a submarine would most likely not have any windows. Let’s refine this design:
Steering
(sub-classes include rack & pinion, rudder, etc.)
Axle
Vehicle
Derivatives of Vehicle:
Roadable extends Vehicle
Car extends Roadable
Truck extends Roadable
Bulldozer extends Truck
Boat extends Vehicle
Lifeboat extends Boat
Ship extends Boat
Submarine extends Boat
Aircraft extends Vehicle
Jetliner extends Aircraft
You get the idea.
You need to implement a feature quickly. There’s some code in another module that’s almost exactly what you need. You have two choices: you may either:
Choice 2 is the anti-pattern. How did we get here? Usually, copypasta is “justified” by claiming that a refactoring operation is error-prone – certainly true, if not done with care, if the offending developer does not understand the implicit assumptions, the original usage could be compromised. Bite the bullet anyway, and refactor. Copypasta has several implicit problems:
If you absolutely can’t refactor right now, and it’s the very first time you’re duplicating a particular code segment, at the very least put a TODO comment in each instance of the code, pointing to the other. You are incurring technical debt, so make sure you have a plan to retire that debt.
This is similar to "cargo cult programming," wherein an inexperienced coder just copies code because it appears near other code that he actually wants to copy (borrow/steal). "Good artists copy; great artists steal." – Pablo Picasso
An operation failed, but we’re not saying where or why it failed. Maybe we’re not even saying that it failed. Examples:
In C++:
try {
// do something }
catch (...) { }
In Python:
try:
something
except:
pass
In Java:
try {
// do something
} catch (Throwable t) { }
… or maybe there was an attempt:
try {
// do something
} catch (Throwable t) {
LOGGER.warn("something went wrong");
// or:
LOGGER.warn(t.getMessage());
}
Java’s exception handling is exceptionally robust. (See what I did there?) Probably more than 9 out of 10 times, the stack trace and the exception type are enough to tell you what went wrong, and either fix it or handle the case where it occurs. In that last snippet, we had all that information, and just threw it away. Here’s a better way:
try {
// do something
} catch(Exception e) {
LOGGER.warn("Exception caught: ", e);
// logs the type and stack trace
}
Even better, when multiple exception types can be thrown, they can be handled differently:
try {
// do something
} catch (NumberFormatException nfe) {
// handle that
} catch (FileNotFoundException fnfe) {
// handle that
} catch (Exception e) {
LOGGER.warn("Something I wasn't prepared for happened: ", e);
}
In none of these cases did we catch Throwable; those are generally more serious errors, requiring some reworked code. However, some code somewhere will catch Throwable, even if it’s the top level of the stack, to abort the program. Which brings up another principle: exceptions should only be caught by code that knows how to handle them. Uncaught exceptions will happily unwind the call stack until they are caught. “There’s no need for me to continue if I can’t handle the problem; I’ll make it Somebody Else’s Problem.”