In the end, we write code.
There are certain best practices that “good” programmers follow, maybe without even thinking about them, or maybe because they have learned, to their sorrow, that doing otherwise will cost them, even if only in time and effort.
Good code is
Good code is
Readable
Good code is
Readable
Trusted
Good code is
Readable
Trusted
Has it been tested?
Good code is
Readable
Trusted
Has it been tested?
Good code is
Readable
Trusted
Has it been tested?
Good code is
Readable
Trusted
Has it been tested?
Does it smell?
A code smell is a section of code that exhibits a practice that the programming community considers ill-advised or even dangerous.
Good code is
Readable
Trusted
Has it been tested?
Does it smell?
A code smell is a section of code that exhibits a practice that the programming community considers ill-advised or even dangerous.
Maintainable
Does the code throw barriers in the way of people trying to fix it or change it? e.g.,
Good code is
Readable
Trusted
Has it been tested?
Does it smell?
A code smell is a section of code that exhibits a practice that the programming community considers ill-advised or even dangerous.
Maintainable
Does the code throw barriers in the way of people trying to fix it or change it? e.g.,
Is the code highly coupled so that changes in one place directly affect many others?
Good code is
Readable
Trusted
Has it been tested?
Does it smell?
A code smell is a section of code that exhibits a practice that the programming community considers ill-advised or even dangerous.
Maintainable
Does the code throw barriers in the way of people trying to fix it or change it? e.g.,
Is the code highly coupled so that changes in one place directly affect many others?
Is code duplicated, so that fixes/changes need to be applied simultaneously to many different places?
Martin relates a story about replaying an editing session via an editor that captures the entire history of edits.
Bob enters the module.
He scrolls down to the function needing change.
He pauses, considering his options.
Oh, he’s scrolling up to the top of the module to
check the initialization of a variable.
Now he scrolls back down and begins to type.
Ooops, he’s erasing what he typed!
He types it again.
He erases it again!
He types half of something else but then erases that!
He scrolls down to another function that calls the function
he’s changing to see how it is called.
He pops up another window and looks at a subclass. Is that
function overridden?
Or if Bob was not the original author of the code?
Code should be expressive of its intent.
Refactoring is the process of making changes to code that make it more expressive, but that should not alter its behavior in any way.
Common examples of refactoring include:
Many IDEs (including Eclipse) have build-in support for these and other refactoring activities.
Refactor
menu not only changes the name of the function in the declaration, but also of all calls to that function in all source code files where it is used.We do not write large amounts of code and then get around to testing it (or claiming we had no time to test it).
We do not even write large amounts of code without pausing to rerun our tests every few minutes.
We’ll pass on the details for now, as we will be discussing automated unit testing and test driven development in great detail later.
int nCstCnt; // number of customers
int numberOfCustomers;
There was a time when programmers could be forgiven for writing the code on the left.
None of that is true any more!
So please stop writing code like I had to back in the late 1970’s – S. Zeil
Be expressive of the intention.
Be pronounceable
Avoid tacking meaningless noise words like “Info”, “Data”, “Object”, …
What’s the difference between classes named Book
and BookData
?
Avoid encoding type information.
Avoid variable names like songList
.
album
, playlist
, or discography
.Use appropriate parts of speech
Avoid “nounifying” verbs.
generateSummaryReport
is a perfectly good function name.SummaryReportGenerator
is hardly better.class SummaryReport {
⋮
void generateAndPrint(OutputDevice);
The scope of a declared name is the range of code in which it can be legally referenced.
Place your declarations in as limited a scope as possible.1
This prevents accidental re-uses of the same variable name, including many uses that may interfere with one another.
Don’t do this:
String firstName;
String lastName;
int pos = fullName.find(',');
if (pos >= 0) {
lastName = fullName.substr(0, pos);
firstName = fullName.substr(pos+1);
println ("Hello " + firstName + ' ' + lastName);
} else {
println ("Hello " + fullName);
}
The scopes of firstName
and lastName
continue to run through the rest of this code block, till the end of the enclosing { }
.
Do this instead:
int pos = fullName.find(',');
if (pos >= 0) {
String lastName = fullName.substr(0, pos);
String firstName = fullName.substr(pos+1);
println ("Hello " + firstName + ' ' + lastName);
} else {
println ("Hello " + fullName);
}
Not only is there less chance of an accidental re-use of the names later, but there is also never a period of time in which the variables are uninitialized.
Don’t do this:
vector<Author>::iterator iter = authorList.begin();
while (iter != authorList.end())
{
doSomethingWith(*iter);
++iter;
}
The scope of iter
continues to run through the rest of this code block, till the end of the enclosing { }
.
Do this instead:
for (vector<Author>::iterator iter = authorList.begin();
iter != authorList.end(); ++iter)
{
doSomethingWith(*iter);
}
The scope of iter
runs only to the end of the loop body.
This helps to keep them small, and understandable.
Bad: Suppose a Library
class had this:
public void addBookToCollection (Book book)
{
List<Author> authors = book.getAuthors();
Title title = book.getTitle();
List<SubjectKeyword> subjects = new List<>();
try {
subjects = book.getSubjects();
} catch (SubjectsUnassigned ex) {
requestReviewOfSubjects(book);
}
try {
DecimalClassification dewey = oclc.getClassificationCode(book.getISBN());
ShelfLocation intendedLocation = getShelfFor(dewey);
book.setLocation (intendedLocation);
} catch (OCLCServiceUnavailable ex) {
log.error("Error contacting OCLC about " + book.toString(), ex);
}
try {
for (Author author: authors) {
authorIndex.add(author, book);
}
titleIndex.add (title, book);
for (SubjectKeyword subject: subjects) {
subjectIndex.add(subject, book);
}
} catch (CatalogingError ex) {
log.error ("Error cataloging " + book.toString(), ex);
}
}
Cleaner:
public void addBookToCollection (Book book)
{
addBookToCatalogs(book);
moveBookToIntendedLocation(book);
}
private void addBookToCatalogs(book) {
addBookToAuthorCatalog(book);
addBookToTitleCatalog(book);
addBookToSubjectCatalog(book);
}
private void addBookToAuthorCatalog(Book book) {
List<Author> authors = book.getAuthors();
try {
for (Author author: authors) {
authorIndex.add(author, book);
}
} catch (CatalogingError ex) {
log.error ("Error cataloging " + book.toString(), ex);
}
}
private void addBookToTitleCatalog(Book book) {
Title title = book.getTitle();
try {
titleIndex.add (title, book);
} catch (CatalogingError ex) {
log.error ("Error cataloging " + book.toString(), ex);
}
}
private void addBookToSubjectCatalog (Book book) {
try {
List<SubjectKeyword> subjects = book.getSubjects();
for (SubjectKeyword subject: subjects) {
subjectIndex.add(subject, book);
}
} catch (CatalogingError ex) {
log.error ("Error cataloging " + book.toString(), ex);
}
} catch (SubjectsUnassigned ex) {
requestReviewOfSubjects(book);
}
}
private void moveBookToIntendedLocation (Book book) {
try {
ShelfLocation intendedLocation = getIntendedLocation(book);
book.setLocation (intendedLocation);
} catch (OCLCServiceUnavailable ex) {
log.error("Error contacting OCLC about " + book.toString(), ex);
}
}
private ShelfLocation getIntendedLocation (Book book)
throws OCLCServiceUnavailable
{
DecimalClassification dewey = oclc.getClassificationCode(book.getISBN());
return getShelfFor(dewey);
}
Look, in particular, at the top two functions. * Aren’t they clearly more expressive of what needs to happen? * Is there anything in there that took you out of the Library-level of abstraction and forced you to simultaneously think about how more detailed components of the library were designed to work?
Many of those functions were complicated by the pattern of
But those are two different “things”…
public void addBookToCollection (Book book)
{
addBookToCatalogs(book);
attemptToMoveBookToIntendedLocation(book);
}
private void addBookToCatalogs(book) {
attemptToAddBookToAuthorCatalog(book);
attemptToAddBookToTitleCatalog(book);
attemptToAddBookToSubjectCatalog(book);
}
private void attemptToAddBookToAuthorCatalog(Book book) {
try {
addBookToAuthorCatalog(book);
} catch (CatalogingError ex) {
log.error ("Error cataloging " + book.toString(), ex);
}
}
private void addBookToAuthorCatalog(Book book) {
List<Author> authors = book.getAuthors();
for (Author author: authors) {
authorIndex.add(author, book);
}
}
private void attemptToAddBookToTitleCatalog(Book book) {
try {
addBookToTitleCatalog (book);
} catch (CatalogingError ex) {
log.error ("Error cataloging " + book.toString(), ex);
}
}
private void addBookToTitleCatalog(Book book) {
Title title = book.getTitle();
titleIndex.add (title, book);
}
private void attemptToAddBookToSubjectCatalog(Book book) {
try {
addBookToSubjectCatalog (book);
} catch (CatalogingError ex) {
log.error ("Error cataloging " + book.toString(), ex);
}
}
private void addBookToSubjectCatalog(Book book) throws CatalogingError {
List<SubjectKeyword> subjects = getSubjectListOrRequestReview(book);
for (SubjectKeyword subject: subjects) {
subjectIndex.add(subject, book);
}
}
private List<SubjectKeyword> getSubjectListOrRequestReview(Book book) {
try {
return book.getSubjects();
} catch (SubjectsUnassigned ex) {
requestReviewOfSubjects(book);
return new Collections<SubjectKeyWord>.emptyList();
}
}
private void moveBookToIntendedLocation (Book book) {
try {
ShelfLocation intendedLocation = getIntendedLocation(book);
book.setLocation (intendedLocation);
} catch (OCLCServiceUnavailable ex) {
log.error("Error contacting OCLC about " + book.toString(), ex);
}
}
private ShelfLocation getIntendedLocation (Book book)
throws OCLCServiceUnavailable
{
DecimalClassification dewey = oclc.getClassificationCode(book.getISBN());
return getShelfFor(dewey);
}
We have a lot more functions now, but each one is almost trival to read.
Duplicated code is code that needs to be fixed/changed multiple times when fixing one bug or adding one feature.
Implicitly couples blocks of code in a way not visible from the interfaces.
Makes it hard to hide (“information hiding”) a design decision.
In our library example, the decision on how to handle cataloging errors has encoded three times:
private void addBookToCatalogs(book) {
attemptToAddBookToAuthorCatalog(book);
attemptToAddBookToTitleCatalog(book);
attemptToAddBookToSubjectCatalog(book);
}
private void attemptToAddBookToAuthorCatalog(Book book) {
try {
addBookToAuthorCatalog(book);
} catch (CatalogingError ex) {
log.error ("Error cataloging " + book.toString(), ex);
}
}
private void attemptToAddBookToTitleCatalog(Book book) {
try {
addBookToTitleCatalog (book);
} catch (CatalogingError ex) {
log.error ("Error cataloging " + book.toString(), ex);
}
}
private void attemptToAddBookToSubjectCatalog(Book book) {
try {
addBookToSubjectCatalog (book);
} catch (CatalogingError ex) {
log.error ("Error cataloging " + book.toString(), ex);
}
}
Better is to collect that common decision either like this:
private void addBookToCatalogs(book) {
attemptToAddBookToAuthorCatalog(book);
attemptToAddBookToTitleCatalog(book);
attemptToAddBookToSubjectCatalog(book);
}
private void handleCatalogingError (Book book, CatalogingError ex)
{
log.error ("Error cataloging " + book.toString(), ex);
}
private void attemptToAddBookToAuthorCatalog(Book book) {
try {
addBookToAuthorCatalog(book);
} catch (CatalogingError ex) {
handleCatalogingError (book, ex);
}
}
private void attemptToAddBookToTitleCatalog(Book book) {
try {
addBookToTitleCatalog (book);
} catch (CatalogingError ex) {
handleCatalogingError (book, ex);
}
}
private void attemptToAddBookToSubjectCatalog(Book book) {
try {
addBookToSubjectCatalog (book);
} catch (CatalogingError ex) {
handleCatalogingError (book, ex);
}
}
or like this:
private void attemptToAddBookToCatalogs(book) {
try {
addBookToCatalogs(book);
} catch (CatalogingError ex) {
log.error ("Error cataloging " + book.toString(), ex);
}
}
private void addBookToCatalogs(book) {
addBookToAuthorCatalog(book);
addBookToTitleCatalog(book);
addBookToSubjectCatalog(book);
}
Some classes/structs exist purely to specify a data structure, e.g.,
struct AuthorLinkedListNode {
Author data;
AuthorLinkedListNode* next;
AuthorLinkedListNode (const Author& author,
AuthorLinkedListNode* nextNode = nullptr)
: data(author), next(nextNode)
{}
};
This data type is not intended to be seen outside of Book
:
class Book {
private:
AuthorLinkedListNode* first;
AuthorLinkedListNode* last;
⋮
public:
so there’s really little reason to encapsulate its data members.
In some cases we might even enforce that idea
class Book {
private:
struct AuthorLinkedListNode {
Author data;
AuthorLinkedListNode* next;
AuthorLinkedListNode (const Author& author,
AuthorLinkedListNode* nextNode = nullptr)
: data(author), next(nextNode)
{}
};
AuthorLinkedListNode* first;
AuthorLinkedListNode* last;
⋮
public:
although this is not always possible.
“Talk to friends, not strangers.”
The law of Demeter: a method
f
of a classC
should only call the methods of:
C
- an object created by
f
- an object passed as an argument to
f
- an object held in a (data structure of a) data member of
C
This law is intended to discourage code like this:
class Library {
⋮
void checkOutBookToPatron (Book book, Patron patron)
{
if (patron.getBranch().getLendingRecord(patron).getNumCheckedOut()
< patron.getBranch().getLendingRecord(patron).getCheckOutLimit()) {
Date dueDate = Calendar.today().addDays(book.checkOutTerm());
patron.getBranch().getLendingRecord(patron)
.addCheckout(book, dueDate);
book.checkOutTo(patron, dueDate);
} else {
throw new CheckoutLimitReached(patron);
}
}
Some people find the chained calls hard to read.
Perhaps more to the point, this style hides a coupling of this function to the interface of classes LibraryBranch
and LendingRecord
.
Making the coupling explicit might be an improvement:
class Library {
⋮
void checkOutBookToPatron (Book book, Patron patron)
{
LibraryBranch patronsHomeBranch = patron.getBranch();
LendingRecord patronLendingRecord = branch.getLendingRecord(patron);
if (patronLendingRecord.getNumCheckedOut()
< patronLendingRecord.getCheckOutLimit()) {
Date dueDate = Calendar.today().addDays(book.checkOutTerm());
patronLendingRecord.addCheckout(book, dueDate);
book.checkOutTo(patron, duedate);
} else {
throw new CheckoutLimitReached(patron);
}
}
but it doesn’t make the coupling go away.
If anything, it makes it clearer that we are not sticking to a uniform level of abstraction here. So, better still:
class Library {
⋮
void checkOutBookToPatron (Book book, Patron patron)
{
if (patron.canCheckOutMoreBooks()) {
Date dueDate = Calendar.today().addDays(book.checkOutTerm());
patron.addBookToCheckedOutList(book, dueDate);
book.checkOutTo(patron, duedate);
} else {
throw new CheckoutLimitReached(patron);
}
}
class Patron {
⋮
public void addBookToCheckedOutList (Book book, Date dueDate) {
LibraryBranch patronsHomeBranch = branch;
LendingRecord patronLendingRecord = branch.getLendingRecord(patron);
patronLendingRecord.addCheckout(book, dueDate);
}
public boolean canCheckOutMoreBooks() {
LibraryBranch patronsHomeBranch = branch;;
LendingRecord patronLendingRecord = branch.getLendingRecord(patron);
return patronLendingRecord.getNumCheckedOut()
< patronLendingRecord.getCheckOutLimit()
}
which still has multiple violations, so we need to keep going:
class Patron {
⋮
public void addBookToCheckedOutList (Book book, Date dueDate) {
LibraryBranch patronsHomeBranch = branch;
branch.addBookToCheckedOutList(patron, book, dueDate);
}
public boolean canCheckOutMoreBooks() {
LibraryBranch patronsHomeBranch = branch;;
return branch.getNumCheckedOut(patron)
< branch.getCheckOutLimit(patron);
}
class LibraryBranch {
⋮
public void addBookToCheckedOutList (Patron patron,
Book book, Date dueDate) {
LendingRecord patronLendingRecord = lendingRecords.get(patron);
patronLendingRecord.addCheckout(book, dueDate);
}
public boolean canCheckOutMoreBooks() {
LendingRecord patronLendingRecord = lendingRecords.get(patron);
return patronLendingRecord.getNumCheckedOut()
< patronLendingRecord.getCheckOutLimit();
}
throws
declarations (and their accompanying coupling) throughout the code.Martin argues strongly against large classes.
I.e., the number of responsibilities should be small.
get/set pairs probably count as only one.
But refactoring to accomodate some of his other recommendations (e.g., “functions should do one thing”) tends to make the number of function members explode.
The trick is that the added members are generally private, not public.
Cohesion
Martin: We should also be able to write a brief description of the class in about 25 words, without using the words “if,” “and,” “or,” or “but.”
1: This practice is not commonly included in writing about Clean Coding, but I think that’s because it is generally taken for granted that “every” programmer does this. But I see lots of violations of this practice in student coding every semester. – SJZ