int createProcess();
// Which one is correct?
errorCode = createProcess()
processNumber = createProcess()
processCount = createProcess()
createdInMillis = createProcess()
Example: Bad: Method doing more than one thing
// Are the lines in the proper order?
public synchronized void flush() throws IOException {
getContentBuffer().flush();
IOUtils.copy(new StringReader(getContentBuffer().getBuffer().toString()), getBufferedWriter());
getBufferedWriter().flush();
contentBuffer = new StringWriter();
}
Example: Good: Methods doing one thing
// Method complies with the contract
@Override // This is also important, not only for the compiler
public synchronized void flush() throws IOException {
flushMyBuffer();
getBufferedWriter().flush();
}
private synchronized void flushMyBuffer() throws IOException {
contentBuffer.flush();
IOUtils.copy(new StringReader(getContentBuffer().getBuffer().toString()), getBufferedWriter());
contentBuffer = new StringWriter();
}
// Now it is clear that it has two buffers. Is is a good solution?
// Clean code is not the final goal, it is a way to the goal:
// is the code correct?
Example: Bad: Method doing more than one thing (from the book)
// It does three things
public void pay() {
for (Employee e : employees) {
if (e.isPayday()) {
Money pay = e.calculatePay();
e.deliverPay(pay);
}
}
}
Example: Good: Methods doing one thing (from the book)
public void pay() {
for (Employee e : employees) {
payIfNecessary(e);
}
}
private void payIfNecessary(Employee e) {
if (e.isPayday()) {
calculateAndDeliverPay(e);
}
}
private void calculateAndDeliverPay(Employee e) {
Money pay = e.calculatePay();
e.deliverPay(pay);
}
Example: Bad: Side effect: modifying the input parameter
// Incorrect name
private SelectedDirectionDTO createSelectedOption(OptionDTO option, String fareFamily) {
SelectedDirectionDTO so = new SelectedDirectionDTO();
so.setId(option.getId());
so.setOrigin(option.getOrigin());
so.setOriginCountry(option.getOrginCountry());
so.setDestination(option.getDestination());
so.setDepartureDate(option.getDepartureDate());
so.setDepartureTime(option.getDepartureTime());
so.setArrivalDate(option.getArrivalDate());
so.setArrivalTime(option.getArrivalTime());
so.setMultipleOrigin(option.isMultipleOrigin());
so.getFlights().addAll(option.getFlights());
// Non-standard TODO comment
// Not informative comment
//FB_TODO: the code from here might need changes
// Negative condition first: harder to understand
if (!moreOptions) {
// Inline implementation
for (RoutingFareDTO cFare : option.getFares()) {
if (routingFareMatches(fareFamily, cFare)) {
so.setFare(cFare);
}
}
} else {
// Inline implementation
String fam = famMoreOptionMapping.get(fareFamily);
if (fam != null) {
for (RoutingFareDTO fare : option.getFares()) {
if (fam.equalsIgnoreCase(fare.getCompartment())) {
so.setFare(fare);
// Modifying the input parameter!
so.getFare().setFareFamily(fareFamily);
}
}
}
}
return so;
}
Avoid Bad Input Parameter Types
Object
String for non-textual types
boolean for switches - use constants or enum instead
Program to Methods
Stateless, independent methods:
Prefer return values to modifying input variables
Prefer methods that depend only on the input parameters -No configuration parameters or data mining inside
Independent = easily testable
At the end of the method calls there should always be a method that depend only on the input parameter and whose only result is the return value (or exception).
Overloading
Use overloading only for convenience
For example for default values
Do not use it for hiding differences
They should call each other - code smell if they don't
Hiding
Hide irrelevant details in a separate method
Do not hide relevant details in a separate method
Example: Bad: Overloads and Hiding
// Overloads doing different things
// relevant details are hidden (what lists are created)
List<Integer> scores = initScores(10, 0);
List<Integer> scores = initScores(10);
Example: Good: No overloads and Hiding
// Method intention is expressed in their names
// only irrelevant details are hidden (how lists are created)
List<Integer> scores = createListWithEqualNumbers(10, 0);
List<Integer> scores = createListWithDifferentNumbers(10);
Two types of methods
"Coordinator" or "orchestrator"
"Technical" or "Algorithm"
Example: Good: Types of methods
// Orchestrator method - Easy to read - Business logic
public void modifySomething(Something s) {
s.setAnything( s.getThis(), s.getThat() );
if( s.isBig() ) {
s.setSomethingElse( DEFAULT_SOMETHING_ELSE );
}
}
// Algorithm method - Easy to test
public Anything calculateAnything(This this, That that) {
return this.getCount() * that.getSize();
}
How to use these types:
Do not mix business lines with technical lines
Do not pollute the code with technical details
Hide irrelevant details
Do not hide relevant details
Example: Bad: Pollution by repeating irrelevant technical details
