Home / Digitec Coding Practices / Global Coding Standards / Smells and Heuristics - Functions

◀ Environment | General ▶

Functions

F1: Too Many Arguments

Functions should have a small number of arguments. No argument is best, followed by one, two, and three. More than three is very questionable and should be avoided with prejudice. (See "Function Arguments" on page 40. Text reproduced below.)

Function Arguments

The ideal number of arguments for a function is zero (niladic). Next comes one (monadic), followed closely by two (dyadic). Three arguments (triadic) should be avoided where possible. More than three (polyadic) requires very special justification—and then shouldn't be used anyway.

Arguments are hard. They take a lot of conceptual power. That's why I got rid of almost all of them from the example. Consider, for instance, the StringBuffer in the example. We could have passed it around as an argument rather than making it an instance variable, but then our readers would have had to interpret it each time they saw it. When you are reading the story told by the module, includeSetupPage() is easier to understand than includeSetupPageInto(newPageContent). The argument is at a different level of abstraction than the function name and forces you to know a detail (in other words, StringBuffer) that isn't particularly important at that point.

Arguments are even harder from a testing point of view. Imagine the difficulty of writing all the test cases to ensure that all the various combinations of arguments work properly. If there are no arguments, this is trivial. If there's one argument, it's not too hard. With two arguments the problem gets a bit more challenging. With more than two arguments, testing every combination of appropriate values can be daunting.

Output arguments are harder to understand than input arguments. When we read a function, we are used to the idea of information going in to the function through arguments and out through the return value. We don't usually expect information to be going out through the arguments. So output arguments often cause us to do a double-take.

One input argument is the next best thing to no arguments. SetupTeardownIncluder.render(pageData) is pretty easy to understand. Clearly we are going to render the data in the pageData object.

F2: OutputArguments

Output arguments are counterintuitive. Readers expect arguments to be inputs, not out- puts. If your function must change the state of something, have it change the state of the object it is called on. (See "Output Arguments" on page 45. Text reproduced below.)

Output Arguments

Arguments are most naturally interpreted as inputs to a function. If you have been programming for more than a few years, I'm sure you've done a double-take on an argument that was actually an output rather than an input. For example:

appendFooter(s);

Does this function append s as the footer to something? Or does it append some footer to s? Is s an input or an output? It doesn't take long to look at the function signature and see:

public void appendFooter(StringBuffer report)

This clarifies the issue, but only at the expense of checking the declaration of the function. Anything that forces you to check the function signature is equivalent to a double-take. It's a cognitive break and should be avoided.

In the days before object oriented programming it was sometimes necessary to have output arguments. However, much of the need for output arguments disappears in OO languages because this is intended to act as an output argument. In other words, it would be better for appendFooter to be invoked as

report.appendFooter();

In general output arguments should be avoided. If your function must change the state of something, have it change the state of its owning object.

F3: Flag Arguments

Boolean arguments loudly declare that the function does more than one thing. They are confusing and should be eliminated. (See "Flag Arguments" on page 41. Text reproduced below.)

Flag Arguments

Flag arguments are ugly. Passing a boolean into a function is a truly terrible practice. It immediately complicates the signature of the method, loudly proclaiming that this function does more than one thing. It does one thing if the flag is true and another if the flag is false!

In Listing 3-7 (code below) we had no choice because the callers were already passing that flag in, and I wanted to limit the scope of refactoring to the function and below. Still, the method call render(true) is just plain confusing to a poor reader. Mousing over the call and seeing render(boolean isSuite) helps a little, but not that much. We should have split the function into two: renderForSuite() and renderForSingleTest().

package fitnesse.html;

import fitnesse.responders.run.SuiteResponder; 
import fitnesse.wiki.*;

public class SetupTeardownIncluder { 
	private PageData pageData;
	private boolean isSuite;
	private WikiPage testPage;
	private StringBuffer newPageContent; 
	private PageCrawler pageCrawler;
	
	public static String render(PageData pageData) throws Exception { 
		return render(pageData, false);
	}
	
	public static String render(PageData pageData, boolean isSuite) throws Exception {
		return new SetupTeardownIncluder(pageData).render(isSuite);
	}
	
	private SetupTeardownIncluder(PageData pageData) { 
		this.pageData = pageData;
		testPage = pageData.getWikiPage();
		pageCrawler = testPage.getPageCrawler(); 
		newPageContent = new StringBuffer();
	}
	
	private String render(boolean isSuite) throws Exception { 
		this.isSuite = isSuite;
		if (isTestPage())
			includeSetupAndTeardownPages(); 
		return pageData.getHtml();
	}
	
	private boolean isTestPage() throws Exception { 
		return pageData.hasAttribute("Test");
	}
	
	private void includeSetupAndTeardownPages() throws Exception { 
		includeSetupPages();
		includePageContent();
		includeTeardownPages();
		updatePageContent(); 
	}
	
	private void includeSetupPages() throws Exception { 
		if (isSuite)
			includeSuiteSetupPage(); 
		includeSetupPage();
	}
	
	private void includeSuiteSetupPage() throws Exception { 	
		include(SuiteResponder.SUITE_SETUP_NAME, "-setup");
	}
	
	private void includeSetupPage() throws Exception { 
		include("SetUp", "-setup");
	}
	
	private void includePageContent() throws Exception { 
		newPageContent.append(pageData.getContent());
	}
	
	private void includeTeardownPages() throws Exception { 
		includeTeardownPage();
		if (isSuite)
			includeSuiteTeardownPage(); 
	}
	
	private void includeTeardownPage() throws Exception { 
		include("TearDown", "-teardown");
	}
	
	private void includeSuiteTeardownPage() throws Exception { 
		include(SuiteResponder.SUITE_TEARDOWN_NAME, "-teardown");
	}
	
	private void updatePageContent() throws Exception { 
		pageData.setContent(newPageContent.toString());
	}
	
	private void include(String pageName, String arg) throws Exception { 
		WikiPage inheritedPage = findInheritedPage(pageName);
		if (inheritedPage != null) {
			String pagePathName = getPathNameForPage(inheritedPage);
			buildIncludeDirective(pagePathName, arg); 
		}
	}
	
	private WikiPage findInheritedPage(String pageName) throws Exception { 
		return PageCrawlerImpl.getInheritedPage(pageName, testPage);
	}
	
	private String getPathNameForPage(WikiPage page) throws Exception { 
		WikiPagePath pagePath = pageCrawler.getFullPath(page);
		return PathParser.render(pagePath);
	}
	
	private void buildIncludeDirective(String pagePathName, String arg) { 
		newPageContent
			.append("\n!include ")
			.append(arg) 
			.append(" .") 
			.append(pagePathName) 
			.append("\n");
	}
}

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.

◀ Environment | General ▶