Clean Code Made Simple - Part 1

I’ve read Robert C. Martin’s Clean Code book a couple of years ago. It’s a great book, Especially for those who are in the junior years of their career. It helps you become a more mature software developer/engineer and write quality code more often.

I aggregated the tips, tricks, and practices I’ve learned from this book and will publish them in multiple parts.

Note that there are quotes and code examples that I’ve used from the original book in this series.

So without further ado, Let’s start.

#1 Short Functions Are Better

What uncle bob says about a function’s length is that the smaller a function is, The better. He suggests that the block of code inside an if statement, else statement, and while statement should be just one line. Also, the indent level of a function should not be more than one or two.

#2 Introduce Instance Variables When Possible

Passing instance variables instead of primitive parameters into a function is a good idea when it’s proper. But when it’s proper?

Consider a large function with many variables declared within it. Let’s say you want to extract one small part of that function into a separate function. However, the code you want to extract uses four of the variables declared in the function.

This is a situation where passing an instance variable may be a good idea.

class Sample 
{
  public function __construct() 
  {

  }

  public function render() 
  {
   // doing some logic
   $this->getHtml($var1, $var2, $var3, $var4);
  }

  private function getHtml($var1, $var2, $var3, $var4) {
   // doing some logic with vars
  }
}

Will become:

class Sample 
{
  private $var;

  public function __construct(VarThing $var) 
  {
    $this->var = $var;  
  }

  public function render() 
  {
   // doing some logic
   $this->getHtml();
  }

  private function getHtml() 
  {
   // $this->var is accessible here
  }
}

#3 The BUILD-OPERATE-CHECK Pattern

You’ve probably heard of Arrange-Act-Assert or AAA pattern before, Which is similar to the Build-Operate-Check pattern.

How do you think we can improve the following code?

public function testGetPageHieratchyAsXml()
{
    crawler()->addPage($root, PathParser()->parse("PageOne"));
    crawler()->addPage($root, PathParser()->parse("PageOne.ChildOne"));
    crawler()->addPage($root, PathParser()->parse("PageTwo"));

    request()->setResource("root");
    request()->addInput("type", "pages");

    $responder = new SerializedPageResponder();
    $response = $responder->makeResponse(
        new FitNesseContext($root), $request
    );

    $xml = $response->getContent();

    assertEquals("text/xml", $response->getContentType());
    assertSubString("<name>PageOne</name>", $xml);
    assertSubString("<name>PageTwo</name>", $xml);
    assertSubString("<name>ChildOne</name>", $xml);
}

public function testGetPageHieratchyAsXmlDoesntContainSymbolicLinks()
{

    $pageOne = crawler()->addPage($root, PathParser()->parse("PageOne"));

    crawler()->addPage($root, PathParser()->parse("PageOne.ChildOne"));
    crawler()->addPage($root, PathParser()->parse("PageTwo"));

    $data = $pageOne->getData();

    $properties = $data->getProperties();
    $symLinks = $properties->set(SymbolicPage::PROPERTY_NAME);

    $symLinks.set("SymPage", "PageTwo");
    $pageOne.commit($data);
    request()->setResource("root");
    request()->addInput("type", "pages");

    $responder = new SerializedPageResponder();
    $response = $responder->makeResponse(new FitNesseContext($root), $request);

    $xml = $response->getContent();

    assertEquals("text/xml", $response->getContentType());
    assertSubString("<name>PageOne</name>", $xml);
    assertSubString("<name>PageTwo</name>", $xml);
    assertSubString("<name>ChildOne</name>", $xml);
    assertNotSubString("SymPage", $xml);
}

public function testGetDataAsHtml()
{
    crawler()->addPage($root, PathParser()->parse("TestPageOne"), "test page");
    request()->setResource("TestPageOne");
    request()->addInput("type", "data");
    $responder = new SerializedPageResponder();
    $response = $responder->makeResponse(
        new FitNesseContext($root), $request);
    $xml = $response->getContent();
    assertEquals("text/xml", $response->getContentType());
    assertSubString("test page", $xml);
    assertSubString("<Test", $xml);
}

This is how we can improve it:

public function testGetPageHierarchyAsXml()
{
    makePages("PageOne", "PageOne.ChildOne", "PageTwo");
    submitRequest("root", "type:pages");
    assertResponseIsXML();
    assertResponseContains("<name>PageOne</name>", "<name>PageTwo</name>", "<name>ChildOne</name>");
}

public function testSymbolicLinksAreNotInXmlPageHierarchy() 
{
    $page = makePage("PageOne");
    makePages("PageOne.ChildOne", "PageTwo");
    addLinkTo($page, "PageTwo", "SymPage");
    submitRequest("root", "type:pages");
    assertResponseIsXML();
    assertResponseContains("<name>PageOne</name>", "<name>PageTwo</name>", "<name>ChildOne</name>");
    assertResponseDoesNotContain("SymPage");
}

public function testGetDataAsXml()
{
    makePageWithContent("TestPageOne", "test page");
    submitRequest("TestPageOne", "type:data");
    assertResponseIsXML();
    assertResponseContains("test page", "<Test");
}

The benefit of this pattern is that the vast majority of annoying details have been eliminated. The tests get right to the point and use only the data types and functions that they truly need. Anyone who reads these tests should be able to understand what they do very quickly, without being misled or overwhelmed by details.

#4 Single Responsibility and Error Handling

The best way to describe this is to quote directly from the book:

Functions should do one thing. Error handing is one thing. Thus, a function that handles errors should do nothing else. This implies that if the keyword try exists in a function, it should be the very first word in the function and that there should be nothing after the catch/finally.

#5 Better Error Handling

how do you think this piece of code can get better shaped?

$port = new ACMEPort(12);

try {
    $port->open();
} catch (DeviceResponseException $e) {
        reportPortError($e);
        logger()->log("Device response exception", $e);
    } catch (ATM1212UnlockedException $e) {
        reportPortError($e);
        logger()->log("Unlock exception", $e);
    } catch (GMXError $e) {
        reportPortError($e);
        logger()->log("Device response exception");
    } finally {
        /// ...
}

We can simplify our code considerably by wrapping the API that we are calling and making sure that it returns a common exception type:

$port = new LocalPort(12);

try {
    $port->open();
} catch (PortDeviceFailure $e) {
    reportError($e);
    logger()->log($e->getMessage(), $e);
} finally {
    // ...
}

class LocalPort {

    private $innerPort;

    public function __construct(int $portNumber) {
        $this->innerPort = new ACMEPort($portNumber);
    }

    public function open() {
        try {
            $this->innerPort->open();
        } catch (DeviceResponseException $e) {
            throw new PortDeviceFailure($e);
        } catch (ATM1212UnlockedException $e) {
            throw new PortDeviceFailure($e);
        } catch (GMXError $e) {
            throw new PortDeviceFailure($e);
        }
    }

    // ...
}

Wrappers like the one we defined for ACMEPort can be very useful.

Okay. This is it for this part. I’ll continue posting the next parts soon. You can join my Telegram channel to get notified of the latest posts. Also, you can follow me on Twitter and LinkedIn.