Clean Code
Intern Lunch & Learn
June 12, 2015
Reid Harrison
July 2013
Unified Presentation Framework
What is "Software Craftsmanship?"
A craftsman or artisan is a skilled manual worker who makes items that may be functional or strictly decorative.
•How is software a craft?
•How is software different from typical crafts?
•What things make programmers artisans?
•Why is it important to improve in your craft?
•What are important qualities of a skilled craftsman?
•How can you improve your crafting skills?
The Art of Software
Writing code is similar to painting a picture. A programmer who writes clean code is an artist who can guide a blank screen through a series of transformations until it is an elegantly coded system.
- Easy to recognize a good painting
- Hard to paint a good picture
Programmers are authors – responsible for communicating well with their readers.
- You write for readers who will judge your effort and skill
- The ratio spent reading vs. writing code is usually at least 10:1
From Journeyman to Master
Care about your craft
Think about your work
Continuously make small improvements
As a student:
- Solve it
- Test it
- Fix it
- Turn it in
- Forget about it
As a professional:
- Solve it
- Make it easy to understand
- Make it easy to use
- Make it easy to change
- Test it
- Fix it
- Commit it
- Release it
- Support it
throw new EPFSystemException("Structure exceeds two levels " + objEntry + " " + json);if (l2Temp != null && l2Temp.isValueNode()) {
logger.debug("Is nested value node {} stack id {}", objEntry, stackId);
buildAndAddEvtDetail(event, objEntry, l2Temp.asText(), stackId);
}
else {
throw new EPFSystemException("Structure exceeds two levels " + objEntry + " " + json);
}for (int x = 0; x < anode.size(); x++) {
Iterator<String> level2 = anode.get(x).fieldNames();
while (level2.hasNext()) {
String objEntry = level2.next();
logger.debug("Processing nested object {} stack id {}", objEntry, stackId);
JsonNode l2Temp = anode.get(objEntry);
if (l2Temp != null && l2Temp.isValueNode()) {
logger.debug("Is nested value node {} stack id {}", objEntry, stackId);
buildAndAddEvtDetail(event, objEntry, l2Temp.asText(), stackId);
}
else {
throw new EPFSystemException("Structure exceeds two levels " + objEntry + " " + json);
}
}
stackId++;
}if (temp.isValueNode()) {
logger.debug("Found value node, building detail {}", fname);
buildAndAddEvtDetail(event, fname, temp.asText(), 0);
}
else if (temp.isArray()) {
logger.debug("Found array, building detail {}", fname);
ArrayNode anode = (ArrayNode) temp;
if (anode.size() > 0) {
JsonNode obj = findNonNullObject(anode);
if (obj == null || obj.isValueNode()) {
logger.debug("Found Array of value nodes {} size {}", fname, anode.size());
int stackId = 1;
for (int x = 0; x < anode.size(); x++) {
if (anode.get(x) != null) {
buildAndAddEvtDetail(event, fname, anode.get(x).asText(), stackId);
}
stackId++;
}
}
else if (obj.isObject()) {
logger.debug("Found Array of objects {} size {}", fname, anode.size());
int stackId = 1;
for (int x = 0; x < anode.size(); x++) {
Iterator<String> level2 = anode.get(x).fieldNames();
while (level2.hasNext()) {
String objEntry = level2.next();
logger.debug("Processing nested object {} stack id {}", objEntry, stackId);
JsonNode l2Temp = anode.get(objEntry);
if (l2Temp != null && l2Temp.isValueNode()) {
logger.debug("Is nested value node {} stack id {}", objEntry, stackId);
buildAndAddEvtDetail(event, objEntry, l2Temp.asText(), stackId);
}
else {
throw new EPFSystemException("Structure exceeds two levels " + objEntry + " " + json);
}
}
stackId++;
}
}
else {
throw new EPFSystemException("error - found unexpected object type found " + fname + " " + json);
}
}
}
else if (temp.isObject()) {
logger.debug("Found object, building detail {}", fname);
Iterator<String> level1 = temp.fieldNames();
while (level1.hasNext()) {
String objEntry = level1.next();
logger.debug("Processing object {}", objEntry);
JsonNode l2Temp = temp.get(objEntry);
if (l2Temp != null && l2Temp.isValueNode()) {
logger.debug("Is a value node {}", objEntry);
buildAndAddEvtDetail(event, objEntry, l2Temp.asText(), 0);
}
else {
throw new EPFSystemException("error - found unexpected object type found " + objEntry + " " + json);
}
}
}The Perils of Messy Code

Mess builds, productivity of team decreases
Under pressure, the team makes more messes
Entropy - The amount of disorder in a system. When disorder increases in software, it is called "code rot."
The only way to develop quickly and meet deadlines is to keep the code as clean as possible at all times.


http://xkcd.com/844/
What is messy code?
- Smells bad
- Follows anti-patterns
- Contains spaghetti code
- Duplication
- Entities are too complex
- Intent isn't clear
- Logical flow is hard to follow
- Unused or unmaintained
- Inconsistent
What is clean code?
- Simple and direct
- Reads like well-written prose
- Never obscures the designer's intent
- Full of efficient abstractions
- Straightforward logical flow
- Can be read and enhanced by new developers
- Provides one way to do one thing
- Provides a clear and minimal API
- Has unit and acceptance tests
Mantras of a Software Craftsman
-
Broken windows
- One broken window, left un-repaired, instills in the community a sense of abandonment.
-
Boy Scout rule
- "Leave the campground cleaner than you found it."
-
Tests are your parachute
- Keep them free of holes and obstructions.
Your Knowledge Portfolio
Your knowledge and experience are your most important professional assets.
Unfortunately, they're expiring assets.
- Invest regularly
- Diversify
- Manage risk
- Buy low, sell high
- Review and rebalance
Expand your knowledge portfolio regularly; not only with technical expertise but also Capital One expertise.
public Map<String, UPFAppFeature> getAllFeature(DomainProfile domainInfo) {
Map<String, UPFAppFeature> upfAppFeatureMap = new Hashtable<String, UPFAppFeature>();
if (domainInfo.getSystemOfRecord().equalsIgnoreCase(FeatureToggleConstants.DB_SYSTEM_STORAGE)) {
try {
readItFromDB(domainInfo, upfAppFeatureMap);
}
catch (RuntimeException dbError) {
logger.error("Error reading from Database, the provided details were: App Name: " + domainInfo.getAppName()
+ " Domain Name:" + domainInfo.getDomainIndentifier()
+ ". Will try reading it from backup source (file system)", dbError);
readItFromFileSystem(domainInfo, upfAppFeatureMap);
}
}
else if (domainInfo.getSystemOfRecord().equalsIgnoreCase(FeatureToggleConstants.FILE_SYSTEM_STORAGE)) {
readItFromFileSystem(domainInfo, upfAppFeatureMap);
}
else {
logger.error("ERROR fetching features from system of record, no matching system of record found: ",
domainInfo.getSystemOfRecord());
return null;
}
return upfAppFeatureMap;
}private void readItFromDB(DomainProfile domainInfo, Map<String, UPFAppFeature> upfAppFeatureMap) {
logger.debug("Fetching features data from DB system, the system of record value is: "
+ domainInfo.getSystemOfRecord() + "App Name: " + domainInfo.getAppName() + " Domain Name:"
+ domainInfo.getDomainIndentifier());
Map<String, Feature> featureFromDB;
if (isMongoDBImpl()) {
featureFromDB = mongoDBFeatureStore.readAll(domainInfo.getAppName(), domainInfo.getDomainIndentifier());
}
else {
featureFromDB = jdbcFeatureStore.readAll(domainInfo.getAppName(), domainInfo.getDomainIndentifier());
}
loadUPFFeaturesMap(featureFromDB, upfAppFeatureMap);
// write it to the file system for backup activities:
try {
writeFeaturesOnFilesystemForBackup(featureFromDB);
}
catch (Exception excpetion) {
logger.error("Could not write to file system path: " + domainInfo.getFeaturesFilePath(), excpetion);
}
}public void toggleFeature(DomainProfile domainProfile, String regionIdentifier, String toggle, String featureName, String featureGroup) {
if (domainProfile.getSystemOfRecord().equalsIgnoreCase(FeatureToggleConstants.DB_SOR)) {
if (toggle.equalsIgnoreCase("enable")) {
if (isMongoDBImpl()) {
mongoDBFeatureStore.enable(featureName, featureGroup, regionIdentifier);
}
else {
jdbcFeatureStore.enable(featureName, featureGroup, regionIdentifier);
}
}
else if (toggle.equalsIgnoreCase("disable")) {
if (isMongoDBImpl()) {
mongoDBFeatureStore.disable(featureName, featureGroup, regionIdentifier);
}
else {
jdbcFeatureStore.disable(featureName, featureGroup, regionIdentifier);
}
}
else {
EPFMessage error = new EPFMessage(appConfiguration.getString(FeatureToggleConstants.INVALID_FLIPPING_OPTION_ERROR_CODE));
error.setInputParams(appConfiguration.getString(FeatureToggleConstants.INVALID_FLIPPING_OPTION_MESSAGE));
throw new RuntimeException(FeatureToggleConstants.INVALID_FLIPPING_OPTION_MESSAGE);
}
}
else if (domainProfile.getSystemOfRecord().equalsIgnoreCase(FeatureToggleConstants.FS_SOR)) {
if (toggle.equalsIgnoreCase("enable")) {
fileSystemStore.enable(featureName);
}
else if (toggle.equalsIgnoreCase("disable")) {
fileSystemStore.disable(featureName);
}
else {
EPFMessage error = new EPFMessage(appConfiguration.getString(FeatureToggleConstants.INVALID_FLIPPING_OPTION_ERROR_CODE));
error.setInputParams(appConfiguration.getString(FeatureToggleConstants.INVALID_FLIPPING_OPTION_MESSAGE));
throw new RuntimeException(FeatureToggleConstants.INVALID_FLIPPING_OPTION_MESSAGE);
}
}
else {
throw new EPFSystemException(FeatureToggleConstants.INVALID_SYSTEM_OF_RECORD_OPT);
}
}Single Responsibility Principle
-
The single responsibility should be entirely encapsulated by its context
-
A unit of code should have only one reason to change
-
Keeps a class focused (high cohesion)
-
Lowers coupling
A class or module should be defined by one, and only one, responsibility.
public String generateMessage(StreamMessageContext ctx, Object... data) {
int size = ((data == null) ? 1 : data.length + 1);
// serialize each object
int i = 1;
String[] jsonEntries = new String[size];
jsonEntries[0] = DataConversionSupport.serializeToJsonString(ctx);
for (Object datum : data) {
jsonEntries[i] = DataConversionSupport.serializeToJsonString(datum);
++i;
}
// then aggregate all the serializations together
StringBuilder sb = new StringBuilder();
sb.append('{').append('\"').append(ctx.getClass().getSimpleName()).append("\":").append(jsonEntries[0]); // the key
for (int j = 0; j < i - 1; ++j) {
sb.append(',\"').append(data[j].getClass().getSimpleName()).append("\":"); // item name
sb.append(jsonEntries[j + 1]); // item json
}
sb.append('}');
return sb.toString();
}
public String generateMessage(StreamMessageContext ctx, String[] dataKeys, Object... data) {
int size = ((data == null) ? 1 : data.length + 1);
// serialize each object
int i = 1;
String[] jsonEntries = new String[size];
jsonEntries[0] = DataConversionSupport.serializeToJsonString(ctx);
for (Object datum : data) {
jsonEntries[i] = DataConversionSupport.serializeToJsonString(datum);
++i;
}
// build up keys
String[] keys = processDataKeys(dataKeys, data);
// then aggregate all the serializations together
StringBuilder sb = new StringBuilder();
sb.append('{').append('\"').append(ctx.getClass().getSimpleName()).append("\":").append(jsonEntries[0]); // the key
for (int j = 0; j < i - 1; ++j) {
sb.append(',\"').append(keys[j]).append("\":"); // item name
sb.append(jsonEntries[j + 1]); // item json
}
sb.append('}');
return sb.toString();
}The Evils of Duplication
-
Requirements, understanding, and environments change. Your code will need to change with it.
-
It's not a matter of if you'll remember, but when you'll forget.
DRY - Don't Repeat Yourself
- Imposed duplication
- Inadvertent duplication
- Inter-developer duplication
- Impatient duplication
if (appObject.isDsclsrOnlineRequiredInd() || appObject.isDsclsrCreditScoreRequiredInd()) {
if (appObject.isDsclsrOnlineRequiredInd()) {
// Pull account disclosure
setDisclosureVariableFromContent(AOD, aodVariableReplacer,
appObject.getDecisionedOfferID(), model, evalContext);
}
if (appObject.isDsclsrCreditScoreRequiredInd() && appObject.getMinCreditScore() != null
&& appObject.getMinCreditScore().getBureauCode()!=null) {
// Pull credit disclosure
if(appObject.getMinCreditScore().getBureauCode().equals("")){
setDisclosureVariableFromContent(CSD, csdVariableReplacer,
NO_HIT_KEY, model, evalContext);
}else{
setDisclosureVariableFromContent(CSD, csdVariableReplacer,
appObject.getMinCreditScore().getBureauCode(), model, evalContext);
}
}
}
if (appObject.getApplicant() != null && appObject.getApplicant().get(0) != null ) {
userInfo.setFirstName(appObject.getApplicant().get(0).getFirstName());
userInfo.setLastName(appObject.getApplicant().get(0).getLastName());
userInfo.setLangCode(appObject.getApplicant().get(0).getLangCode());
userInfo.setSsoId(null);
userInfo.setUserId(appObject.getApplicant().get(0).getCustomerID());
if (appObject.getApplicant().get(0).getEmailAddresses() != null &&
appObject.getApplicant().get(0).getEmailAddresses().size() > 0) {
userInfo.setEmailAddress(appObject.getApplicant().get(0).getEmailAddresses().get(0));
}
}Coupling and Cohesion
With tightly coupled systems:
- A change in one module forces changes in others
- Modules are harder to reuse
- Modules are harder to test
With low cohesion systems:
- Modules are complex with more operations
- Modules are less maintainable and harder to work with
Coupling is a measure of how closely connected two routines or modules are.
Cohesion is a measure of how strongly related each piece of functionality is.
Coupling and Cohesion have an inverse relationship
Naming Conventions
Descriptive
int d; // elapsed time in days
int daysSinceCreation;Avoid disinformation
Account[] accountList;
boolean notActive;
Make meaningful distinctions
void arrayCopy(char[] a1, char[] a2);
void arrayCopy(char[] source, char[] destination);Pronounceable
String evtStCd, evtAudtg;Searchable
Date date, transactionDate;
public class RequestBuilder { ...Solution/Problem relevant
int tableUsage, loadFactor;
Node tortoise, hare;Function Conventions
- Small
- Do only one thing
- One level of abstraction per function
- Descriptive names
int getStatus();
int getResponseStatusCode();- Minimal parameters
- Don't pass codes
- Don't return null
- Verbs not nouns
int cardCount();
int getCardCount();
void newCard();
void addNewCard();Commenting Conventions
"Programs must be written for people to read and only incidentally for machines to execute" - Hal Abelson
- The only "comment" guaranteed to be accurate is the code its self
- Comments have to be maintained and updated same as the code
- The more complex the comments are, the more likely they are to be wrong or out of date
The best solution is to write easily comprehensible code.
Good Comments
Code can't explain why the program is being written, and the rationale for choosing this or that method. Code cannot discuss reasons certain alternative approaches were taken.
Comments are best used to provide contextual information that makes it easier to understand the code.
- How the code fits into the big picture
- Why this methodology was chosen and why others were rejected
- Code is written to solve a problem. Describe how your code solves the problem and why it is better than the alternatives
- Warning of consequences
Bad Comments
Single line comments are usually unnecessary and should only be used if the operation is complex.
j = j + 1; //Increment j
int a = c * 100; //convert to cents
double avg = a / n; //average cents per customer
int totalCents = totalDollars * 100;
double averageCentsPerCustomer = totalCents / customerCount;Instead of writing comments that are designed to make code more readable, rewrite the code.
BAD
GOOD
Do not release with TODOs or commented-out code
Code Formatting Conventions
- Density
- Distance
- Order
Horizontal formatting
- Density
- Alignment
- Indentation
Vertical formatting
Code Formatting Conventions
https://google-styleguide.googlecode.com/svn/trunk/javaguide.html
http://lars-lab.jpl.nasa.gov/JPL_Coding_Standard_Java.pdf
Common rules:
- Documentation (JavaDoc) conventions
- Upper-case class names; camel-case variable names
- New line after conditionals; always using braces
- One variable per declaration
- Avoid chained method calls
- Store common references for reuse
- Encapsulate and use positive conditionals
- Declare abstract type, instantantiate implementation
Follow team and company rules
On the Grind
- Be aware
- Don't be "lazy"
- Seek feedback
- Code reviews
- Books and Blogs
- Retrospectives
- Continuously improve
- PRACTICE!
Clean Code L&L
By Reid Harrison
Clean Code L&L
- 687