1. The below functions has duplicated code, odd strings, unobvious data types. However, with a few simple method extractions, some renaming, and a little restructuring will make the functions more understandable.
/* use the first code snippet instead of second one */
/* first code snippet */
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();
}
/* second code snippet */
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();
}
2. "Small"
"The first rule of functions is that they should be small. The second rule of functions is that they should be smaller than that."
//USE: (How short should your functions be?) public static String renderPageWithSetupsAndTeardowns(PageData pageData, boolean isSuite) throws Exception { if (isTestPage(pageData)) includeSetupAndTeardownPages(pageData, isSuite); return pageData.getHtml(); } //INSTEAD OF: 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(); }
"The blocks within if statements, else statements, while statements, and so on should be one line long. Probably that line should be a function call.
3."Blocks and Indenting"
The indent level of a function should not be greater than one or two. This, ofcourse, makes the functions easier to read and understand."
4."Do One Thing"
"FUNCTIONS SHOULD DO ONE THING. THEY SHOULD DO IT WELL.THEY SHOULD DO IT ONLY."
"...Another way to know that a function is doing more than “one thing” is if you can extract another function from it with a name that is not merely a restatement of its implementation."
5. "Switch Statements"
"There are several problems with this function. First, it’s large, and when new employee types are added, it will grow."
public Money calculatePay(Employee e)throws InvalidEmployeeType { switch (e.type) { case COMMISSIONED: return calculateCommissionedPay(e); case HOURLY: return calculateHourlyPay(e); case SALARIED: return calculateSalariedPay(e); default: throw new InvalidEmployeeType(e.type); } }
"For example we could have
isPayday(Employee e, Date date),
or
deliverPay(Employee e, Money pay),
or a host of others. All of which would have the same deleterious structure.
The solution to this problem is to bury the switch statement in the basement of an ABSTRACT FACTORY, and never let anyone see it.
The factory will use the switch statement to create appropriate instances of the derivatives of Employee, and the various functions, such as calculatePay, isPayday, and deliverPay, will be dispatched polymorphically through the Employee interface.
My general rule for switch statements is that they can be tolerated if they appear only once, are used to create polymorphic objects, and are hidden behind an inheritance relationship so that the rest of the system can’t see them."
public abstract class Employee { public abstract boolean isPayday(); public abstract Money calculatePay(); public abstract void deliverPay(Money pay); } public interface EmployeeFactory { public Employee makeEmployee(EmployeeRecord r) throws InvalidEmployeeType; } public class EmployeeFactoryImpl implements EmployeeFactory { public Employee makeEmployee(EmployeeRecord r) throws InvalidEmployeeType { switch (r.type) { case COMMISSIONED: return new CommissionedEmployee(r) ; case HOURLY: return new HourlyEmployee(r); case SALARIED: return new SalariedEmploye(r); default: throw new InvalidEmployeeType(r.type); } } }
6."Use Descriptive Names"
"Don’t be afraid to make a name long. A long descriptive name is better than a short enigmatic name. A long descriptive name is better than a long descriptive comment...Don’t be afraid to spend time choosing a name."
7. "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."
Argument Objects
"When a function seems to need more than two or three arguments, it is likely that some of those arguments ought to be wrapped into a class of their own."
//USE: Circle makeCircle(Point center, double radius); //INSTEAD OF: Circle makeCircle(double x, double y, double radius);
8."Command Query Separation"
public boolean set(String attribute, String value); if (set("username", "unclebob"))...
"Functions should either do something or answer something, but not both.
//USE THIS:
if (attributeExists("username")) {
setAttribute("username", "unclebob");
// ...
}
9."Prefer Exceptions to Returning Error Codes"
"Returning error codes from command functions is a subtle violation of command query separation. It promotes commands being used as expressions in the predicates of if statements."
if(deletePage(page) == E_OK)
10."Extract Try/Catch Blocks"
"Extract the bodies of the try and catch blocks out into functions of their own.
The delete function is all about error processing. It is easy to understand and then ignore.
The deletePageAndAllReferences function is all about the processes of fully deleting a page."
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());
}
"Functions should do one thing. Error handing is one thing. Thus, a function that handles errors should do nothing else."
"The whole testableHtml function refactored according to the principles described here."
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");
}
}
Robert C Martin - Clean Code: A Handbook of Agile Software Craftsmanship References:
No comments:
Post a Comment