Lets’ start with the original code, but already wrapped into the new class to isolate
```java
class SetupTeardownIncluder {
public String render(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();
}
}
```
If we squint our eyes (and collapse the most-outer if-statements) we can already see the most outer control flow.
1. initialize the page
2. if test, add setup
3. add content
4. if test, add teardown
5. return
That would be my first level of abstraction. The rest goes one step down:
```java
class SetupTeardownIncluder {
public String render(PageData pageData, boolean includeSuiteSetup) throws Exception {
WikiPage wikiPage = pageData.getWikiPage();
StringBuffer buffer = new StringBuffer();
includeSetupPageIfNecessary(pageData, includeSuiteSetup, buffer);
buffer.append(pageData.getContent());
includeTeardownPageIfNecessary(pageData, wikiPage, buffer, includeSuiteSetup);
pageData.setContent(buffer.toString());
return pageData.getHtml();
}
private void includeSetupPageIfNecessary(PageData pageData, boolean includeSuiteSetup, StringBuffer buffer) {
if (!pageData.hasAttribute("Test"))
return;
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");
}
}
private void includeTeardownPageIfNecessary(PageData pageData, WikiPage wikiPage, StringBuffer buffer, boolean includeSuiteSetup) {
if (!pageData.hasAttribute("Test"))
return;
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");
}
}
}
}
```
We see that there are a lot of objects that we suddenly need to pass around. Now it makes sense to put them as instance variables, and hide the initialization of the object behind a static function that creates the object and invokes the actual `render`-function, so that the client can’t accidently re-use an object with unexpected state.
```java
class SetupTeardownIncluder {
private PageData pageData;
private WikiPage wikiPage;
private boolean includeSuiteSetup;
private StringBuffer buffer;
private boolean isTestPage;
public static String render(PageData pageData, boolean includeSuiteSetup) throws Exception {
return new SetupTeardownIncluder(pageData, includeSuiteSetup).render();
}
private SetupTeardownIncluder(PageData pageData, boolean includeSuiteSetup) {
this.pageData = pageData;
this.includeSuiteSetup = includeSuiteSetup;
this.wikiPage = pageData.getWikiPage();
this.buffer = new StringBuffer();
this.isTestPage = pageData.hasAttribute("Test");
}
private String render() {
includeSetupPageIfNecessary();
buffer.append(pageData.getContent());
includeTeardownPageIfNecessary();
pageData.setContent(buffer.toString());
return pageData.getHtml();
}
private void includeSetupPageIfNecessary() {
if (!isTestPage)
return;
...
}
private void includeTeardownPageIfNecessary() {
if (!isTestPage)
return;
...
}
}
```
\
We now still have 2 somewhat complex methods. But much worse than their size is their duplication. They contain almost the same, just slightly differently parameterized code.
Therefore, my next attempt would be to abstract the lines that start with
```java
WikiPagePath ...
String ...
buffer.append(...
```
```java
class SetupTeardownIncluder {
private PageData pageData;
private WikiPage wikiPage;
private boolean includeSuiteSetup;
private StringBuffer buffer;
private boolean isTestPage;
public static String render(PageData pageData, boolean includeSuiteSetup) throws Exception {
return new SetupTeardownIncluder(pageData, includeSuiteSetup).render();
}
private SetupTeardownIncluder(PageData pageData, boolean includeSuiteSetup) {
this.pageData = pageData;
this.includeSuiteSetup = includeSuiteSetup;
this.wikiPage = pageData.getWikiPage();
this.buffer = new StringBuffer();
this.isTestPage = pageData.hasAttribute("Test");
}
private String render() {
includeSetupPageIfNecessary();
buffer.append(pageData.getContent());
includeTeardownPageIfNecessary();
pageData.setContent(buffer.toString());
return pageData.getHtml();
}
private void includeSetupPageIfNecessary() {
if (!isTestPage)
return;
if (includeSuiteSetup)
appendPathName(SuiteResponder.SUITE_SETUP_NAME, "!include -setup .");
appendPathName("SetUp", "!include -setup .");
}
private void includeTeardownPageIfNecessary() {
if (!isTestPage)
return;
appendPathName("TearDown", "!include -teardown .");
if (includeSuiteSetup)
appendPathName(SuiteResponder.SUITE_TEARDOWN_NAME, "!include -teardown .");
}
private void appendPathName(String inheritedPageName, String includeCommand) {
WikiPage inheritedPage = PageCrawlerImpl.getInheritedPage(inheritedPageName, wikiPage);
if (inheritedPage == null)
return;
WikiPagePath pagePath = wikiPage.getPageCrawler().getFullPath(inheritedPage);
String pathName = PathParser.Render(pagePath);
buffer.append("\n")
.append(includeCommand)
.append(pathName)
.append("\n");
}
}
```
Now we have a “bottom” level of abstraction, and 2 functions that use the lowest level and get called by the highest level. In my mind, this structure has 3 horizontally separated levels of abstraction
1. `render` → calls the next lowel level to enrich the `buffer` with the necessary content and updates the `pageData`
2. `includeSetupPageIfNecessary` & `includeTeardownPageIfNecessary` → knows the condition of when to include which content and calls the lowest level function which actually produces the content that the page will have to contain
3. `appendPathName` → gets the path of the setup/teardown page and enrich the `buffer` with the given command + found path
This is the point where I would be happy with the overall structure of the code and start doing some cleanup. This includes some name changes, moving code around, maybe some small refactorings until I feel happy with it.
### My Final Result
```java
class SetupTeardownIncluder {
private PageData pageData;
private WikiPage testPage;
private boolean includeSuiteSetupAndTeardown;
private boolean includeSetupAndTeardown;
// changes the content of pageData
public static void addTestSuiteToPage(PageData pageData, boolean includeSuiteSetup) throws Exception {
new SetupTeardownIncluder(pageData, includeSuiteSetup).addTestSuiteToPage();
}
private SetupTeardownIncluder(PageData pageData, boolean includeSuiteSetupAndTeardown) {
this.pageData = pageData;
this.testPage = pageData.getWikiPage();
this.includeSetupAndTeardown = pageData.hasAttribute("Test");
this.includeSuiteSetupAndTeardown = includeSuiteSetupAndTeardown;
}
private void addTestSuiteToPage() throws Exception {
if (includeSetupAndTeardown) {
String newPageContent = getSetupCommand() + pageData.getContent() + getTeardownCommand();
pageData.setContent(newPageContent);
}
}
private String getSetupCommand() throws Exception {
var result = new StringBuilder();
if (includeSuiteSetupAndTeardown) {
result.append(getPathCommandForInheritedPage(SuiteResponder.SUITE_SETUP_NAME, "-setup"));
}
result.append(getPathCommandForInheritedPage("SetUp", "-setup"));
return result.toString();
}
private String getTeardownCommand() throws Exception {
var result = new StringBuilder();
result.append(getPathCommandForInheritedPage("TearDown", "-teardown"));
if (includeSuiteSetupAndTeardown) {
result.append(getPathCommandForInheritedPage(SuiteResponder.SUITE_TEARDOWN_NAME, "-teardown"));
}
return result.toString();
}
private String getPathCommandForInheritedPage(String inheritedPageName, String command) throws Exception {
WikiPage inheritedPage = PageCrawlerImpl.getInheritedPage(inheritedPageName, testPage);
if (inheritedPage == null) {
return "";
}
WikiPagePath pagePath = testPage.getPageCrawler().getFullPath(inheritedPage);
String pathName = PathParser.Render(pagePath);
return "\n" + "!include " + command + " ." + pathName + "\n";
}
}
```
I can confidently say that my solution is already very clean in terms of function length. Even my longest method is only 7 lines of code. My class manages with 4 methods (excluding the static overload and the constructor) instead of Uncle Bob’s 16. The class itself is also smaller.
I also removed the `buffer` instance variable, because it got mutated at different places in the code and made it difficult to comprehend how its state changes over time. Instead I let the sub-methods return their part of the new page data content as a string. That way my code changes from being “commands” to “queries” in the sense of CQRS. The most upper level `addTestSuiteToPage` function remains a “command”-function.
I also changed the signature of the public method. If the client gives me a `PageData` object and I return an HTML string, I would force the client to operate on 2 different levels of abstraction. Therefore I removed the return value and made it a concern of the client to extract the HTML string. I also added a comment telling the client that the `pageData` object is mutated as a side effect of this function.
I tried to change the names of the methods to be more expressive about what they actually do. I do lack domain knowledge of the surrounding system to determine the best names, but I always try to convey what a method does in its name.
The method `addTestSuiteToPage()` has a more expressive implementation in how the inclusion of the test suite is achieved (by adding a setup command before and a teardown command after the page data content). That way, the reader already has an idea of how the implementation works without giving out too much detail about it.
I really think that my implementation follows the heuristics suggested in the book quite well. The only thing that I see violated is Uncle Bob’s definition of “do one thing”. `addTestSuiteToPage`, `getSetupCommand` and `getTeardownCommand` do two things each. `getPathCommandForInheritedPage` does 3 things. In my universe, this is fine if the methods themselves are simple enough.
Let’s compare it directly
### Uncle Bob’s Final Result
```java
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");
}
}
```
![[Pasted image 20240817214942.png]]
I don’t want to disregard the similarities between Uncle Bob’s and my code. We both extracted the function into its own class. In languages like Java, this makes it much easier to unit test isolated behavior and also makes the entire code more modular. We both provide the static function which invokes the entire functionality of the class and hides the initialization of the object. When we ignore the changes to the signature of my functions, my solution looks like an intermediate step to Uncle Bob’s solution. Therefore, at a second glance, the two solutions are more similar than they appear at first glance.