Notes Of Clean Code — Chapter 3 (Functions)
Previous Chapter : Meaningful Names
The first rule of functions is that they should be small. The second rule of functions is that they should be smaller than that. Functions should not be 100 lines long and even hardly ever be 20 lines long.
See the code example below, It’s long and include duplicated code, lots of odd strings and many strange data types:
public static String testableHtml(
PageData pageData,
boolean includeSuiteSetup
) throws Exception {
WikiPage wikiPage = pageData.getWikiPage();
StringBuffer buffer = new StringBuffer();
if (pageData.hasAttribute("Test")) {
if (includeSuiteSetup) {
WikiPage suiteSetup =
PageCrawlerImpl.getInheritedPage(
SuiteResponder.SUITE_SETUP_NAME, wikiPage
);
if (suiteSetup != null) {
WikiPagePath pagePath =
suiteSetup.getPageCrawler().getFullPath(suiteSetup);
String pagePathName = PathParser.render(pagePath);
buffer.append("!include -setup .")
.append(pagePathName)
.append("\n");
}
}
WikiPage setup =
PageCrawlerImpl.getInheritedPage("SetUp", wikiPage);
if (setup != null) {
WikiPagePath setupPath =
wikiPage.getPageCrawler().getFullPath(setup);
String setupPathName = PathParser.render(setupPath);
buffer.append("!include -setup .")
.append(setupPathName)
.append("\n");
}
}
buffer.append(pageData.getContent());
if (pageData.hasAttribute("Test")) {
WikiPage teardown =
PageCrawlerImpl.getInheritedPage("TearDown", wikiPage);
if (teardown != null) {
WikiPagePath tearDownPath =
wikiPage.getPageCrawler().getFullPath(teardown);
String tearDownPathName = PathParser.render(tearDownPath);
buffer.append("\n")
.append("!include -teardown .")
.append(tearDownPathName)
.append("\n");
}
if (includeSuiteSetup) {
WikiPage suiteTeardown =
PageCrawlerImpl.getInheritedPage(
SuiteResponder.SUITE_TEARDOWN_NAME,
wikiPage
);
if (suiteTeardown != null) {
WikiPagePath pagePath =
suiteTeardown.getPageCrawler().getFullPath (suiteTeardown);
String pagePathName = PathParser.render(pagePath);
buffer.append("!include -teardown .")
.append(pagePathName)
.append("\n");
}
}
}
pageData.setContent(buffer.toString());
return pageData.getHtml();
}
Do you understand the function in three minutes? There’s too much going on in there at too many different levels of abstraction.
Code turn into this with a few refactoring:
public static String renderPageWithSetupsAndTeardowns(
PageData pageData, boolean isSuite
) throws Exception {
boolean isTestPage = pageData.hasAttribute("Test");
if (isTestPage) {
WikiPage testPage = pageData.getWikiPage();
StringBuffer newPageContent = new StringBuffer();
includeSetupPages(testPage, newPageContent, isSuite);
newPageContent.append(pageData.getContent());
includeTeardownPages(testPage, newPageContent, isSuite);
pageData.setContent(newPageContent.toString());
}
return pageData.getHtml();
}It’s more readable and not long. But what is the second rule ? It should be smaller than that.
public static String renderPageWithSetupsAndTeardowns(
PageData pageData, boolean isSuite) throws Exception {
if (isTestPage(pageData))
includeSetupAndTeardownPages(pageData, isSuite);
return pageData.getHtml();
}First code example is doing lots more than one thing. Creating buffers, fetching pages, searching for inherited pages, rendering paths etc. But last code example doing one thing. It’s including setups and teardowns into test pages.
FUNCTIONS SHOULD DO ONE THING. THEY SHOULD DO IT WELL. THEY SHOULD DO IT ONLY.
The main problem is that it is hard to know what “one thing” is. Does the last code do one thing ? It’s doing three things: Determining whether the page is a test page, if so including setups and teardowns, rendering the page in HTML.
If a function is only doing the steps specified in its name, that function is doing one thing.
The first code example is doing more than one things. We create second one from first one by refactoring but it has still two levels of abstraction. And we create last one. But we can’t shrink the code in its last one. We could extract the if statement into a function, but that would be rewriting the code without changing the level of abstraction.
If you can extract another function from function with a name that function is doing more than one thing.
Don’t hesitate to make a name long. A long descriptive name is better than a short meaningless name. A long descriptive name is better than a long descriptive comments.
Don’t be afraid to spend time choosing a name. You should try several different names and read each parts of code.
Be consistent in your names. Use the same phrases, nouns and verbs in the function names you choose for your modules.
What is the ideal number of arguments for a function? Zero argument is fine. One or two arguments are okay. But avoid to three arguments. And more than three shouldn’t be used anyway.
If you give a one argument for a function, probably you have two reason: first one, you may be asking a question that argument, as in boolean fileExists(“MyFile”).
second one, you may be transforming it into something else and returning it. For example InputStream fileOpen(“MyFile”) transforms a file name String into an InputStream return value.
Once someone reads these two usages, they will understand what it is.
Another (less) common use way to single argument for a function is an event. In this form there is an input argument but no output argument. The program interprets the call of this function as an event and uses the argument to change the state of the system. For example, void passwordAttemptFailedNtimes(int attempts). It should be very clear to the reader that this is an event. Choose names and contexts carefully.
When you transform its input argument, you should use output argument. For example, StringBuffer transform(StringBuffer in) is better than void includeSetupPageInto(StringBuffer pageText) even if return StringBuffer. At least the return value appears.
Flag arguments are ugly. Passing a boolean into a function is a terrible practice. It means this function does more than one thing. It does one thing if the flag is true and another if the flag is false.
If a function needs more than two or three arguments, those arguments should be wrapped into a class. For example;
Circle makeCircle(double x, double y, double radius);
Circle makeCircle(Point center, double radius);
Creating objects seems like a cheating , but it’s not.
Side effects are lies. Your function says it will do one thing. But it also does other hidden things.
See the code example below:
public class UserValidator {
private Cryptographer cryptographer;
public boolean checkPassword(String userName, String password) {
User user = UserGateway.findByName(userName);
if (user != User.NULL) {
String codedPhrase = user.getPhraseEncodedByPassword();
String phrase = cryptographer.decrypt(codedPhrase, password);
if ("Valid Password".equals(phrase)) {
Session.initialize();
return true;
}
}
return false;
}
}The name of the function is checkPassword. The reader just expect that it checks the password. But this function also does Session.initialize(). Therefore, calling this function risks deleting the current session data. The name of function should be checkPasswordAndInitializeSession.
Try/Catch blocks are ugly. They complicate the code and mix error processing and normal processing. They should be extract the bodies of the try and catch blocks out into functions of their own.
public void delete(Page page) {
try {
deletePageAndAllReferences(page);
}
catch (Exception e) {
logError(e);
}
}
private void deletePageAndAllReferences(Page page) throws Exception {
deletePage(page);
registry.deleteReference(page.name);
configKeys.deleteKey(page.name.makeKey());
}
private void logError(Exception e) {
logger.log(e.getMessage());
}Next Chapter : Comments
