Code Reviews: Common Sources of Extreme Violations and How to Avoid Arguments about Fixing Them

Knives are drawn. Blades are sharpened for conflict. A dispute rages between developers. The passions of coders are inflamed not over buggy software but over egregiously concise or verbose code. Those lines are signs of a hack. Any programmer who disagrees is an amateur. Only a neophyte would produce methods and blocks that so clearly violate good taste. Yet, differing preferences, not laws of nature, are the source this conflict and the vitriol. Hatred between developers is, in this case, the result of different inclinations towards trading succinctness for different ends. These goals, and the tendency for them, are different for every developer, leading to constant conflict in certain areas. One such place is wordy or pithy code. To minimize combat, a team can use code reviews to highlight the most egregious segments, and the group can argue over those parts instead arguing over every line and block of a code base.

Certain code constructs or techniques are likely to produce the extreme violations and to lead arguments about correcting those infractions. Fixing those abuses leads to intense arguments. These disagreements can be resolved, or at least deescalated, for the particular linguistic features and techniques listed below.

Conditional Operator vs. If Statement

The linguistic elements, the conditional-operator and the if-statement, lead to arguments, with different camps arguing that each one is the superior technique for certain operations. These actions can be implemented in myriad ways, each technique bringing advantages and disadvantages.

If Statement: The if-statement can contribute to atrociously voluminous code, when the density of the conditions is high. A high density makes a block or method appear puffy. Yet, code written with if-statements is also highly debuggable, since a developer can step through each line.

if(label1IsRequired){ label1.Color = “red”;} else { label1.Color = “black”;}

if(label2IsRequired){ label2.Color = “red”;} else { label2.Color = “black”;}

if(label3IsRequired){ label3.Color = “red”;} else { label3.Color = “black”;}

Conditional Operator: The conditional-operator can lead to some flagrantly terse lines, when it used as a replacement for several if-statements. Embedded conditional-operators make code, when taken to the extreme, very difficult to read, test, or debug. Any block or method heavy on conditional-operators is also very compact, reducing the amount of code that a developer must scan.

healthIndicatorColor = (health == “Good”)? “green” : (health == “Fair”)? “yellow” : (health == “poor”)? “red” : (health == “life_support”)? “orange” : “purple”;

Potential Resolution: Conditional-operators are a beneficial, when they replace a high density of values set based on conditions implemented through if-statements. Conditional-operators are destructive, when they replace even a couple of decisions embedded within one another. Imperatives that fit readably on one line are a prime target for conditional operators, while conditions that require several lines are the domain of if-statements. Any egregious usage of if-statements or conditional-operators should be corrected to deploy the appropriate usage of one of those constructs. (Note: Modification might require significant refactoring.)

if(health == “Good”) { healthIndicatorColor = “green”;} else if(health == “Fair”) { healthIndicatorColor = “yellow”;} else if(health == “poor”) { healthIndicatorColor = “red”;} else if(health == “life_support”) { healthIndicatorColor = “orange”;} else { healthIndicatorColor = “purple”;}

label1.Color = (label1IsRequired)? “red” : “black”;label2.Color = (label2IsRequired)? “red” : “black”;label3.Color = (label3IsRequired)? “red” : “black”;

Multiple Return Statements vs. One Return Statement

Two particular styles that lead to arguments are multiple returns and single returns. Disagreements emerge over whether methods should have one return statement or whether multiple return statements are acceptable. Each approach has positives and negatives.

Multiple Return Statements: Multiple-return statements can contribute to code that is difficult to understand, follow, and test. However, methods with multiple returns can be shorter than functions with a single return.

SomeDataType someMethod(param1, param2, param3) { SomeDataType retVal; if(param1 == null) { retVal = null } if(retVal == null) { return retVal; }  if(param2 != null){ retVal = param2; } if(retVal.Equals(param2)){ return retVal; }  retVal = param3; return retVal;}

One Return Statement: A single return-statement can lead to long methods. Yet, these procedures have a single point exit, simplifying testing and debugging.

SomeDataType someMethod(){ SomeDataType retVal; //hundreds or thousands of lines code return retVal;}

Potential Resolution: Multiple returns make code difficult to understand, follow, and test, when they are used inconsistently. Single return-statements lead to long methods, when they are proceeded by lengthy stretches of code. Those drawn-out spans can be shortened, or at least made readable, by using several return-statements instead of one. Single returns are perfectly acceptable, when they follow short tracts of code. Any glaring misuse of a single return-statement or multiple returns should be fixed to apply an accepted use case of one of those styles. (Note: Correction might require significant refactoring.)

SomeDataType someMethod(param1, param2, param3){ if(param1 == null || param3 == null){ return null; }  SomeDataType retVal = null; if(param2 != null{ retVal = param2; } else if(param1 != null) { retVal = param1; } ele if(param3 != null) { retVal = param3; }  return retVal;}

SomeDataType someMethod(param1, param2){ SomeDataType retVal = null; for(int i=0;i<param1.length;i++) { if(param1[i].equals(param2)){ retVal = param1[i]; break; } } return retVal;}Break and Continue Usage in Loops

The break and continue constructs are the subject of intense debates. On one side of the argument, developers argue that break and continue can simplify control flow. Other programmers contend that these features complicate a program’s logic. Break and continue can definitely be used to simplify or complicate code. These lines can be spotted.

Break and Continue Usage: The elements can simplify code, but they can also needlessly complicate them.

SomeDataType someMethod(param1, param2){ SomeDataType retVal = null; for(int i=0;i<param1.length;i++) { if(param1[i] == null){ continue; } if(param1[i].equals(param2)){ retVal = param1[i]; break; } } return retVal;}

SomeDataType someMethod(data, param1) { SomeDataType retVal = null; dosomething: for(int i=0;i<retVal != null;i++){ if(i >= data.length){ break; } if(data[i].equals(param1)){ retVal = data[i]; } else { continue; } }

if(retVal == null){ data — refreshData(); goto dosomething; }

return retVal;}

Potential Resolution: Most developers argue that code should use simple mechanisms for control flow. Which specific mechanisms are simple is the source of the debate. This argument becomes much less heated, when each instrument is used in widely accepted ways. Accepted approaches do exist for break and continue. Stick to those conventions to prevent disagreements and to simplify control flow. Any means of control that egregiously violates those standards should be corrected without debate.

SomeDataType someMethod(data, param1) { SomeDataType retVal = null; for(int i=0;i<param1.length;i++) { if(data[i] == null){ continue; //skip rest of loop } if(data[i].equals(param2)){ retVal = data[i]; break; //don’t loop any more b/c I’m done } } return retVal;}

Defensive Exceptions

Exceptions are a means to indicate a problem or to ward off a future issue. What headaches should be indicated or warded off by what parts of code is a topic of fierce debate. On one end of the disagreement, coders argue that pervasive defensive exceptions prevent errors and make them easy to location. However, that laundry list of defense can make code bloated and difficult to understand, as some programmers have argued. Developers on both side of the debate have a point. Defensive exceptions have both benefits and detriments.

Benefits and Detriments of Defensive Exceptions: Protection against errors and other problems can be guarded against, with minimal drawbacks, using defensive exceptions. Those flaws become magnified, when the technique is indiscriminately.

void someMethod(param1, param2) { if(param1 == null || param2 == null){ throw new ArgumentNullException(“One or more parameters is missing”); } //do method stuff}

void someMethod(param1, param2) { //dozens of lines of defensive checks…..  //do rest of method}

Potential Resolution: The deficiencies of defensive exceptions are smallest, when they are deployed in accepted usages. Any deployment of the technique that strays from those conventions should be corrected, unless a compelling reason is provided.

public void someMethod(param1, param2) { //check each parameter in a public method if(param1 == null || param2 == null){ throw new ArgumentNullException(“One or more parameters is missing”); }  //ward off problems causd by invalid data if(!isValid(param1) || !isValid(param2)) { throw new InvalidParameterException(“One or more parameters is invalid”); }  //do something with parameters}

Wrap-up

These code constructs and techniques are used by both good and bad developers. Programmers are people. Human beings have tendencies. These inclinations manifest themselves in code. Occasionally, a developer’s impulses lead him to write code that other coders rightly criticize. The developer being pilloried is not necessarily a bad programmer. The coder criticizing him is not necessarily a good developer. Both people have likely been led astray, at one point, by their preferences. These desires should not lead development to degenerate into a never-ending stream of insults hurled at one another. Rather, programmers should review each other’s code, limit their battles to its worst sections, and agree to settle certain arguments through the rules laid out above.