Comments
C1: Inappropriate Information
Comments should not include informations which are better held in source control systems, issue tracking system etc. In general, some informations like authors, last-modified date, SPR number shouldn't be held in comments. Comments = for technical notes for the code and design.
C2: Obsolete Comment
Get rid of old comments quickly or update it. Don't write comments which will be got old.
C3: Redundant Comment
"Comments should say things that the code cannot say for itself."
i++; // increment i --> This comment is unnecessary
C4: Poorly Written Comment
"If you are going to write a comment, take the time to make sure it is the best comment you can write. Use correct grammar and punctuation. Don’t ramble. Don’t state the obvious.Be brief."
C5: Commented-Out Code
"When you see commented-out code, delete it! Don’t worry, the source code control
system still remembers it. If anyone really needs it, he or she can go back and check out a
previous version. Don’t suffer commented-out code to survive."
Environment
E1: Build Requires More Than One Step
Building a project should be simple! "You should not have to search near and far for all the various little extra JARs, XML files, and other artifacts that the system requires. You should be able to check out the system with one simple command and then issue one other simple command to build it."
svn get mySystem
cd mySystem
ant all
E2: Tests Require More Than One Step
"You should be able to run all the unit tests with just one command. In the best case you
can run all the tests by clicking on one button in your IDE. In the worst case you should
be able to issue a single simple command in a shell."
Functions
F1: Too Many Arguments
"Functions should have a small number of arguments. No argument is best, followed by one, two, and three."
F2: Output Arguments
"Output arguments are illogical. Readers expect arguments to be inputs, not outputs.
If your function must change the state of something, have it change the state of the
object it is called on."
F3: Flag Arguments
"Boolean arguments loudly declare that the function does more than one thing. They are
confusing and should be eliminated."
F4: Dead Function
"Methods that are never called should be discarded. Keeping dead code around is wasteful.
Don’t be afraid to delete the function. Remember, your source code control system still
remembers it."
General
G1: Multiple Languages in One Source File
"Today’s modern programming environments make it possible to put many different languages
into a single source file. The ideal is for a source file to contain one, and only one, language. Realistically, we will probably have to use more than one. But we should take pains to minimize both the number and extent of extra languages in our source files."
G2: Obvious Behavior Is Unimplemented
"For example, consider a function that translates the name of a day to an enum that represents the day.
Day day = DayDate.StringToDay(String dayName);
We would expect the string "Monday" to be translated to Day.MONDAY. We would also expect
the common abbreviations to be translated, and we would expect the function to ignore
case.
When an obvious behavior is not implemented, readers and users of the code can no
longer depend on their intuition about function names. They lose their trust in the original
author and must fall back on reading the details of the code."
G3: Incorrect Behavior at the Boundaries
"Every boundary condition, every corner case, every quirk and exception represents something that can confound an elegant and intuitive algorithm. Don’t rely on your intuition. Look for every boundary condition and write a test for it."
G4: Overridden Safeties
"Exerting manual control over serialVersionUID may be necessary, but it is always risky. "
"Turning off certain compiler warnings (or all warnings!) may help you get the build to succeed, but at the risk of endless debugging sessions. "
"Turning off failing tests and telling yourself you’ll get them to pass later is as bad as pretending
your credit cards are free money."
G5: Duplication
"Every time you see duplication in the code, it represents a missed opportunity for
abstraction."
"A more subtle form is the switch/case or if/else chain that appears again and again in
various modules, always testing for the same set of conditions. These should be replaced
with polymorphism."
"Still more subtle are the modules that have similar algorithms, but that don’t share
similar lines of code. This is still duplication and should be addressed by using the TEMPLATE
METHOD or STRATEGY pattern."
G6: Code at Wrong Level of Abstraction
"We want all the lower level concepts to be in the derivatives and all the higher level concepts to be in the base class."
"Good software design requires that we separate concepts at different levels and place them in different containers. Sometimes these containers are base classes or derivatives and sometimes they are source files, modules, or components. "
G7: Base Classes Depending on Their Derivatives
"In general, base classes should know nothing about their derivatives."
"Deploying derivatives and bases in different jar files and making sure the base jar files know nothing about the contents of the derivative jar files allow us to deploy our systems in discrete and independent components. "
"When such components are modified, they can be redeployed without having to redeploy the base components. "
G8: Too Much Information
"Good software developers learn to limit what they expose at the interfaces of their classes and modules.
- The fewer methods a class has, the better.
- The fewer variables a function knows about, the better.
- The fewer instance variables a class has, the better.
- Hide your data. Hide your utility functions. Hide your constants and your temporaries.
- Don’t create classes with lots of methods or lots of instance variables.
- Don’t create lots of protected variables and functions for your subclasses.
- Concentrate on keeping interfaces very tight and very small.
- Help keep coupling low by limiting information."
What is dead code? An if statement that checks for a condition that can not happen, a catch block that exception is never throwed, a method that is never called etc...Delete them from the system.
G10: Vertical Separation
"Private functions should be defined just below their first usage." "Local variables should be declared just above their first usage and should have a small vertical scope."
G11: Inconsistency
To make code much easier to read and modify: "If within a particular function you use a variable named response to hold an HttpServletResponse, then use the same variable name consistently in the other functions that use HttpServletResponse objects."
G12: Clutter
"Keep your source files clean, well organized, and free of clutter!
- a default constructor with no implementation
- variables that aren't used
- functions that never called
- comments that add no information"
G13: Artificial Coupling
"For example, general enums should not be contained within more specific classes because this forces the whole application to know about these more specific classes. The same goes for general purpose static functions being declared in specific classes."
"Take the time to figure out where functions, constants, and variables ought to be declared. "
G14: Feature Envy
"The methods of a class should be interested in the variables and functions of the class they belong to, and not the variables and functions of other classes. "
G15: Selector Arguments
"Selector arguments are just a lazy way to avoid splitting a large function into several smaller functions. Consider:"
public int calculateWeeklyPay(boolean overtime) {
int tenthRate = getTenthRate();
int tenthsWorked = getTenthsWorked();
int straightTime = Math.min(400,
tenthsWorked);
int overTime = Math.max(0, tenthsWorked
- straightTime);
int straightPay = straightTime *
tenthRate;
double overtimeRate = overtime ? 1.5 : 1.0
* tenthRate;
int overtimePay = (int)Math.round(overTime*overtimeRate);
return straightPay + overtimePay;
}
|
"You call this function with a true if overtime is paid as time and a half, and with a false if overtime is paid as straight time. It’s bad enough that you must remember what calculateWeeklyPay(false) means whenever you happen to stumble across it."
" In general it is better to have many functions than to pass some code into a function to select the behavior. But the real shame of a function like this is that the author missed the opportunity to write the following:"
public int straightPay() {
return getTenthsWorked() * getTenthRate();
}
public int overTimePay() {
int overTimeTenths = Math.max(0,
getTenthsWorked() - 400);
int overTimePay =
overTimeBonus(overTimeTenths);
return straightPay() + overTimePay;
}
private int overTimeBonus(int overTimeTenths) {
double bonus = 0.5 * getTenthRate()
* overTimeTenths;
return (int)
Math.round(bonus);
}
|
G16: Obscured Intent
return iThsWkd * iThsRte + (int) Math.round(0.5 * iThsRte *Math.max(0, iThsWkd - 400);
There is a piece of code that is very dense and impossible to understand. It's worth taking the time to make more understandable of our code to the readers.
G17: Misplaced Responsibility
"One of the most important decisions a software developer can make is where to put code. "
"The principle of least surprise comes into play here. Code should be placed where a reader would naturally expect it to be. "
"Sometimes we get “clever” about where to put certain functionality. We’ll put it in a function that’s convenient for us, but not necessarily intuitive to the reader. "
G18: Inappropriate Static
HourlyPayCalculator.calculatePay(employee, overtimeRate).
"There is a reasonable chance that we’ll want this function to be polymorphic. We may wish to implement several different algorithms for calculating hourly pay, for example, OvertimeHourlyPayCalculator and StraightTimeHourlyPayCalculator. So in this case the function should not be static."
"In general you should prefer nonstatic methods to static methods. When in doubt, make the function nonstatic. If you really want a function to be static, make sure that there is no chance that you’ll want it to behave polymorphically."
G19: Use Explanatory Variables
"One of the more powerful ways to make a program readable is to break the calculations up into intermediate values that are held in variables with meaningful names.Example:"
Matcher match = headerPattern.matcher(line);
if(match.find())
{
String key = match.group(1);
String value = match.group(2);
headers.put(key.toLowerCase(), value);
}
"The simple use of explanatory variables makes it clear that the first matched group is the key, and the second matched group is the value."
G20: Function Names Should Say What They Do
Date newDate = date.add(5);
"Would you expect this to add five days to the date? Or is it weeks, or hours? Is the date instance changed or does the function just return a new Date without changing the old one?"
"If the function adds five days to the date and changes the date, then it should be called addDaysTo or increaseByDays.
If, on the other hand, the function returns a new date that is five days later but does not change the date instance, it should be called daysLater or daysSince."
G21: Understand the Algorithm
"Before you consider yourself to be done with a function, make sure you understand how it works. It is not good enough that it passes all the tests. You must know that the solution is correct.Often the best way to gain this knowledge and understanding is to refactor the function.."
G22: Make Logical Dependencies Physical
"If one module depends upon another, that dependency should be physical, not just logical. Rather it should explicitly ask that module for all the information it depends upon."
"For example, imagine that you are writing a function that prints a plain text report of hours worked by employees. One class named HourlyReporter gathers all the data into a convenient form and then passes it to HourlyReportFormatter to print it. "
public class HourlyReporter {
private HourlyReportFormatter formatter;
private List<LineItem> page;
private final int PAGE_SIZE = 55;
public HourlyReporter(HourlyReportFormatter
formatter) {
this.formatter = formatter;
page = new ArrayList<LineItem>();
}
public void generateReport(List<HourlyEmployee>
employees) {
for (HourlyEmployee
e : employees) {
addLineItemToPage(e);
if (page.size() == PAGE_SIZE)
printAndClearItemList();
}
if (page.size() > 0)
printAndClearItemList();
}
private void
printAndClearItemList() {
formatter.format(page);
page.clear();
}
private void addLineItemToPage(HourlyEmployee
e) {
LineItem item = new LineItem();
item.name = e.getName();
item.hours =
e.getTenthsWorked() / 10;
item.tenths =
e.getTenthsWorked() % 10;
page.add(item);
}
public class LineItem {
public String name;
public int hours;
public int tenths;
}
}
|
"This code has a logical dependency that has not been physicalized. It is the constant PAGE_SIZE. Why should the HourlyReporter know the size of the page? Page size should be the responsibility of the HourlyReportFormatter."
"We can physicalize this dependency by creating a new method in HourlyReport- Formatter named getMaxPageSize(). HourlyReporter will then call that function rather than using the PAGE_SIZE constant."
G23: Prefer Polymorphism to If/Else or Switch/Case
"First, most people use switch statements because it’s the obvious brute force solution, not because it’s the right solution for the situation.
So this heuristic is here to remind us to consider polymorphism before using a switch."
"Second, the cases where functions are more volatile than types are relatively rare. So every switch statement should be suspect.
I use the following “ONE SWITCH” rule: There may be no more than one switch statement for a given type of selection. "
"The cases in that switch statement must create polymorphic objects that take the place of other such switch statements in the rest of the system."
G24: Follow Standard Conventions
"Every team should follow a coding standard based on common industry norms.
This coding standard should specify things like where to declare instance variables; how to name classes, methods, and variables; where to put braces; and so on.
The team should not need a document to describe these conventions because their code provides the examples."
G25: Replace Magic Numbers with Named Constants
"In general it is a bad idea to have raw numbers in your code. You should hide them behind well-named constants. For example, the number 86,400 should be hidden behind the constant SECONDS_PER_DAY. "
"Some constants are so easy to recognize that they don’t always need a named constant to hide behind so long as they are used in conjunction with very self-explanatory code. "
G26: Be Precise
- "Expecting the first match to be the only match to a query is probably naive. "
- "Using floating point numbers to represent currency is almost criminal. "
- "Avoiding locks and/or transaction management because you don’t think concurrent update is likely is lazy at best. "
- "Declaring a variable to be an ArrayList when a List will due is overly constraining. "
- "Making all variables protected by default is not constraining enough."
- "When you make a decision in your code, make sure you make it precisely. Know why you have made it and how you will deal with any exceptions. "
- "Don’t be lazy about the precision of your decisions. "
- "If you decide to call a function that might return null, make sure you check for null. "
- "If you query for what you think is the only record in the database, make sure your code checks to be sure there aren’t others. "
- "If you need to deal with currency, use integers and deal with rounding appropriately. "
- "If there is the possibility of concurrent update, make sure you implement some kind of locking mechanism."
- "Ambiguities and imprecision in code are either a result of disagreements or laziness. In either case they should be eliminated."
G27: Structure over Convention
"Enforce design decisions with structure over convention. Naming conventions are good, but they are inferior to structures that force compliance. "
G28: Encapsulate Conditionals
"Boolean logic is hard enough to understand without having to see it in the context of an if or while statement. Extract functions that explain the intent of the conditional."
if (shouldBeDeleted(timer))
is
preferable to
if (timer.hasExpired()
&& !timer.isRecurrent())
|
G29: Avoid Negative Conditionals
"Negatives are just a bit harder to understand than positives. So, when possible, conditionals should be expressed as positives."
if (buffer.shouldCompact())
is
preferable to
if (!buffer.shouldNotCompact())
|
G30: Functions Should Do One Thing
"It is often tempting to create functions that have multiple sections that perform a series of operations. Functions of this kind do more than one thing, and should be converted into many smaller functions, each of which does one thing."
G31: Hidden Temporal Couplings
public class MoogDiver {
Gradient gradient;
List<Spline> splines;
public void dive(String
reason) {
saturateGradient();
reticulateSplines();
diveForMoog(reason);
}
...
}
A better solution is: ("Each function produces a result that the next function needs, so there is no reasonable way to call them out of order.")
public class MoogDiver {
Gradient gradient;
List<Spline> splines;
public void dive(String
reason) {
Gradient gradient =
saturateGradient();
List<Spline> splines =
reticulateSplines(gradient);
diveForMoog(splines, reason);
}
...
}
|
G32: Don’t Be Arbitrary
"If a structure appears arbitrary, others will feel empowered to change it. "
"If a structure appears consistently throughout the system, others will use it and preserve the convention."
"Public classes that are not utilities of some other class should not be scoped inside another class. The convention is to make them public at the top level of their package."
G33: Encapsulate Boundary Conditions
"Boundary conditions are hard to keep track of. Put the processing for them in one place. Don’t let them leak all over the code. "
if(level + 1
< tags.length) {
parts = new Parse(body,
tags, level + 1, offset + endTag);
body = null;
}
//A better
solution is:
//Notice that
level+1 appears twice. This is a boundary condition that should be
encapsulated within a variable named something like nextLevel.
int nextLevel = level
+ 1;
if(nextLevel < tags.length)
{
parts = new Parse(body,
tags, nextLevel, offset + endTag);
body = null;
}
|
G34: Functions Should Descend Only One Level of Abstraction
"Separating levels of abstraction is one of the most important functions of refactoring, and it’s one of the hardest to do well. "
"This function constructs the HTML tag that draws a horizontal rule across the page. The height of that rule is specified in the size variable.
Now look again. This method is mixing at least two levels of abstraction. The first is the notion that a horizontal rule has a size. The second is the syntax of the HR tag itself."
public String render()
throws Exception
{
StringBuffer html = new StringBuffer("<hr");
if(size > 0)
html.append("
size=\"").append(size + 1).append("\"");
html.append(">");
return html.toString();
}
|
"This change separates the two levels of abstraction nicely. The render function simply constructs an HR tag, without having to know anything about the HTML syntax of that tag.
"The HtmlTag module takes care of all the nasty syntax issues. Indeed, by making this change I caught a subtle error. The original code did not put the closing slash on the HR tag, as the XHTML standard would have it. "
public String render() throws Exception
{
HtmlTag hr = new HtmlTag("hr");
if (extraDashes > 0)
hr.addAttribute("size", hrSize(extraDashes));
return hr.html();
}
private String hrSize(int height)
{
int hrSize = height + 1;
return String.format("%d", hrSize);
}
|
G35: Keep Configurable Data at High Levels
"If you have a constant such as a default or configuration value that is known and expected at a high level of abstraction, do not bury it in a low-level function."
public static void main(String[]
args) throws Exception
{
Arguments arguments = parseCommandLine(args);
...
}
public class Arguments
{
public static final String
DEFAULT_PATH = ".";
public static final String
DEFAULT_ROOT = "FitNesseRoot";
public static final int DEFAULT_PORT = 80;
public static final int
DEFAULT_VERSION_DAYS = 14;
...
}
|
"The configuration constants reside at a very high level and are easy to change.
They get passed down to the rest of the application. The lower levels of the application do not own the values of these constants."
G36: Avoid Transitive Navigation
"In general we don’t want a single module to know much about its collaborators. More specifically, if A collaborates with B, and B collaborates with C, we don’t want modules that use A to know about C. (For example, we don’t want a.getB().getC().doSomething();.) "
"This is how architectures become rigid. Too many modules know too much about the architecture."
"We should not have to roam through the object graph of the system, hunting for the method we want to call. Rather we should simply be able to say:
myCollaborator.doSomething(). "
Java
J1: Avoid Long Import Lists by Using Wildcards
If your class have 80 lines or more of imports, then use wildcard imports.(e.g. import package.*;) And also wildcard imports don't need to exist particular classes. But also most modern IDEs shows a list of specific imports on a single line.
J2: Don’t Inherit Constants
public interface PayrollConstants {
public static final int TENTHS_PER_WEEK = 400;
public static final double OVERTIME_RATE = 1.5;
}
public abstract class Employee implements PayrollConstants {
public abstract boolean isPayday();
public abstract Money calculatePay();
public abstract void deliverPay(Money
pay);
}
public class HourlyEmployee
extends Employee {
private int tenthsWorked;
private double hourlyRate;
public Money calculatePay() {
int straightTime =
Math.min(tenthsWorked, TENTHS_PER_WEEK);
int overTime = tenthsWorked - straightTime;
return new Money(
hourlyRate * (tenthsWorked + OVERTIME_RATE * overTime)
);
}
//...
}
|
"Where did the constants TENTHS_PER_WEEK came from? Don’t use inheritance as a way to cheat the scoping rules of the language. Use a static import instead."
import static PayrollConstants.*;
public class HourlyEmployee
extends Employee {
private int tenthsWorked;
private double hourlyRate;
public Money calculatePay() {
int straightTime =
Math.min(tenthsWorked, TENTHS_PER_WEEK);
int overTime =
tenthsWorked - straightTime;
return new Money(
hourlyRate *
(tenthsWorked + OVERTIME_RATE * overTime)
);
}
//...
}
|
J3: Constants versus Enums
"Don’t keep using the old trick of public static final ints. The meaning of ints can get lost. The meaning of enums cannot, because they belong to an enumeration that is named."
public class HourlyEmployee extends Employee {
private int tenthsWorked;
HourlyPayGrade grade;
public Money calculatePay() {
int straightTime =
Math.min(tenthsWorked, TENTHS_PER_WEEK);
int overTime = tenthsWorked - straightTime;
return new Money(
grade.rate() * (tenthsWorked + OVERTIME_RATE * overTime)
);
}
//...
}
|
public enum HourlyPayGrade {
APPRENTICE {
public double rate() {
return 1.0;
}
},
LEUTENANT_JOURNEYMAN {
public double rate() {
return 1.2;
}
},
JOURNEYMAN {
public double rate() {
return 1.5;
}
},
MASTER {
public double rate() {
return 2.0;
}
};
public abstract double rate();
}
|
Names
N1: Choose Descriptive Names
N2: Choose Names at the Appropriate Level of Abstraction
N3: Use Standard Nomenclature Where Possible
N4: Unambiguous Names
N5: Use Long Names for Long Scopes
N6: Avoid Encodings
N7: Names Should Describe Side-Effects
Tests
T1: Insufficient Tests
T2: Use a Coverage Tool!
T3: Don’t Skip Trivial Tests
T4: An Ignored Test Is a Question about an Ambiguity
T5: Test Boundary Conditions
T6: Exhaustively Test Near Bugs
T7: Patterns of Failure Are Revealing
T8: Test Coverage Patterns Can Be Revealing
T9: Tests Should Be Fast
References:
Robert C Martin - Clean Code: A Handbook of Agile Software Craftsmanship
No comments:
Post a Comment