What a Good Function Should Do

Functions! They are great. They allow us to call upon relatively complex code with just one line and maybe a few input variables. This is what a good function should do:

  • Split up logic
  • Do one thing
  • Be small!
  • Abide by the DRY Principle

Let me explain each of these. Functions are made to take one piece of logic, say saving a data file in a certain format, so that you do not have to constantly write that code again. This ties into the second point.

It should only do one thing! If your function is called saveExperimentalData(); then it should only do that! It should not also create a directory to save it to, read the data or format the data and then save it. It should just save it. That is it!

Thirdly, your functions should be small. This is almost a consequence of the previous point. If you have a function that is hundreds of lines long, I can guarantee you that it is doing more than one thing! Keep code small. It also make it quicker and easier to read, therefore the logic is easier to understand. I was once told that "Code for a function should be no bigger than your head, mentally or physically!"

Finally the DRY principle. This is simply "Don't Repeat Yourself". If you need a repetitive task doing, get a function to do it! Save yourself some work! Do not repeatedly write the same code again and again. This will also avoud functions like getExperimentalData(); and getExperimentalResults();. These are too similar sounding functions. Pick one and stick with it!

Though most importantly of them all is The KISS Principle...


Function Arguments

This is a tricky concept and is often specific to the situation at hand. Though I would argue that an ideal function should take no arguments (niladic). Second best would be accepting just one argument (monadic), then two arguments (dyadic) and any more than three (triadic) is pushing it.

The argument is if you consider testing you functions (which is a whole section on itself but too advanced for this introductory course) then with an increasing number of arguments, it becomes more difficult to test your functions for every eventuality.

However, sometimes it just cannot be avoided. Though in those cases we should aim to make out functions signatures as readable as possible. For example bool fileExists("filePath"); is going to need an input and from the the context of a well named function, it is obvious what the input shout be. It is also easy for the eye to glide over and to gain an understanding of what is happening. Dyadics just add complexity but are sometimes necessary. For example Point p = new Point(0, 0); makes a lot of sense but with something like assertEquals(expected, actual); can cause confusion in the order in which the arguments appear. Someone with experience with this kind of convention may know but to a new reader, it just adds an unnecessary level of confusion.


An Example...

Considering all the things we described a good function should do, take a look at the adjacent code. First thing first, it is obviously way too long. So long in fact I had to make it its own scroll box. This leads me to believe that it is probably also doing more than one thing at a time too. Just from a glance, I can see that it has already broken two of our four criteria. Not a good start.

I don't expect you to read through this code in detail (I wouldn't) but scrolling through it a litte reveals some code that looks rather repetative. This is a prime target for something called method extraction. This is simply tearing out bits of code and putting them into their own function. Doing this along with some intelligent renaming and restructing and we can change this into something much more manageable and readable...

 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();
}
				

Have a look through this refactored code. It isn't necessary to understand all the details but you can see how it is much much shorter and easier to read. It probably only takes less than a minute to skim read this code and to have an idea as to what it is trying to achieve.

Although this code is much smaller, has successfully split up some of the logic, and abides by the DRY principle, does it truly do one thing? Here is a tip: Look inside the first if statement. Its content spreads over multiple line. I think we can make this function even cleaner...

 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();
}
				

If we look at the if statement here, we can see that it does not use curly braces ({}) to encompass its executing statement. This is because the statement is only one line long. This code has a neat and easy to follow indent structure which, as I'm sure you noticed on the previous examples, was difficult to follow. This change makes our code small. It also encapsulates the previous block into one function with a very descriptive and documentary name.

As for some rules for clean functions, I would say that you should aim to keep the function indent levels to be no greater than one or two lines. I would also recommend that no more than three input argumemts should be used. If you feel the need to use more, have the function accept an array, a data structure or an instance of a class instead.

 public static String renderPageWithSetupsAndTeardowns(
	PageData pageData, boolean isSuite)throws Exception {
	if (isTestPage(pageData))
		includeSetupAndTeardownPages(pageData, isSuite);
	return pageData.getHtml();
}