A Simple Way to Improve Joomla Today.

One of my favorite business allegories goes something like this.

A manager and his CEO were talking about their training program. The manager said, “I’m concerned that if we invest in training our employees, they might find better jobs and leave our company.”  When the CEO heard this his face suddenly went pale.

The manager, thinking that the CEO must not have considered that possibility, started to apologize. But the CEO interrupted him and said, “No, it’s not what you said that concerns me. Actually I was just thinking, what happens if we don’t invest in training and our employees never leave?”

I think the moral of this story applies just as much to open source volunteer communities as it does to companies.

Which is why I believe part of getting Joomla back on track is to invest more in training our community. By giving our “people” the skills to contribute at a higher standard, we improve both their contributions to the project and their ability to provide for their families.

So I have decided to do my part by sharing my knowledge of clean coding practices and explain how clean code can improve Joomla!’s chance of surviving the next ten years.

Over the next few weeks I’ll be writing a series of articles about clean coding practices that I believe can help both Joomla and our community members operate at a higher standard.

If you’ve been keeping track of my contribution record so far, then you know most of my suggestions result in long winded theoretical debates, so for this series I’m going to avoid talking about the big controversial reforms and look at simple ways we can improve Joomla today.

Working with what we have

Anyone that has programmed anything for Joomla realizes that the legacy infrastructure has issues and many people are championing for code reform or a new beginning.

Unfortunately the ambiguous nature of the Joomla backwards compatibility policy and the degree of dysfunction within the system make it impractical to try to resolve any of the major issues within the 3.x series.

Ideally I would like to see the ambiguities in our backwards compatibility policy resolved, but since this series is about actionable solutions , I’ll save my views on that topic for another post.

Major refactoring is very difficult even for seasoned programmers and working within the restraints of any BC obligation isn’t easy, but there is still a lot we can do to improve our legacy code.

With some very small adjustments anyone can reduce the overall complexity of Joomla code without worrying about breaking backwards compatible. So to start I want to introduce the easiest of those adjustments.

Remove the ELSE clause

I believe that one of the most abused control structures in programming today is the IF/ELSE statement. This is because the ELSE clause is redundant and leads to unnatural or confusing implementations of otherwise simple logic.

By its very nature an IF clause implicitly has an ELSE clause. Or in other words you cannot have an “if” without an “else”. So for the majority of cases using the ELSE clause is completely unnecessary.

Fortunately removing these redundant statements is very easy. Most of the time it is just a matter of reorganizing the internal logic and can be done without even fully understanding the function being refactored.

I know this might sound a little far fetched, so I want to use some real life examples to illustrate how it works  and how you can use this method to improve your code and Joomla.

We’ll start with a super simple example and then look at some  progressively more complex examples.

Because I like to practice what I preach, all of the examples here are taken from the Joomla code base and I’ve included links to the pull requests with the changes, just in case you feel like test them (HINT, HINT).

Super Simple Example

As you can see we’ve started with a very simple method that even a entry level programmer could read and understand.

Original
libraries/cms/component/router/base.php lines 35 ~ 62 – PR #7985 

 /**
* Class constructor.
*
* @param   JApplicationCms  $app   Application-object ...
* @param   JMenu            $menu  Menu-object ...
*
* @since 3.4
*/
public function __construct($app = null, $menu = null)
{
    if ($app)
    {
        $this->app = $app;
    }
    else
    {
        $this->app = JFactory::getApplication();
    }

    if ($menu)
    {
        $this->menu = $menu;
    }
    else
    {
        $this->menu = $this->app->getMenu();
    }
}

One might think that there is no way we could possible make this method simpler, but as I mentioned earlier every “IF”  already has an implied “ELSE” and by making the else statement explicit we are actually making the logic look more complex than it really is.

This is an example of what I call a “Conditional Default“. If we look at one of the statements we can see that we’re checking the input and and then setting a property IF we have a value ELSE we set it with a default value.

    if ($app)
    {
        $this->app = $app;
    }
    else
    {
        $this->app = JFactory::getApplication();
    }

In super simple cases like this, we start by reversing the condition in the IF clause. Since we are checking if $app has a value, we now check if $app doesn’t have a value.

Next we overwrite the input variable with the default value inside the IF clause.Which allows us to remove the redundant ELSE clause. The last step is to make sure we set the class property using the input variable.

As you can see from the below refactored version, it is more direct and much easier to understand which makes it easier for everyone to work with.

Refactored

/** ... */
public function __construct($app = null, $menu = null)
{
    if (is_null($app))
    {
        $app = JFactory::getApplication();
    }
    
    $this->app = $app;

    if (is_null($menu))
    {
        $menu = $this->app->getMenu();
    }

    $this->menu = $menu;
}
Simple Example

This next example is a little more complex, but still a relatively easy. The same mechanics are at play here, but we have a side effect that needs to be kept when removing the else statement.

Original
libraries/cms/application/cms.php lines 576 ~ 604 – PR #7975

/**
* Gets the value of a user state variable.
*
* @param   string  $key      The key ...
* @param   string  $request  The name ...
* @param   string  $default  The default ...
* @param   string  $type     Filter for the variable...
*
* @return object The request user state.
*
* @since 3.2
*/
public function getUserStateFromRequest($key, $request, $default = null, $type = 'none')
{
    $cur_state = $this->getUserState($key, $default);
    $new_state = $this->input->get($request, null, $type);

    // Save the new value only if it was set in this request.
    if ($new_state !== null)
    {
        $this->setUserState($key, $new_state);
    }
    else
    {
        $new_state = $cur_state;
    }

    return $new_state;
}

Again we start by reversing the condition in the IF clause.

Since $new_state is being overridden with $cur_state in the else clause, we move that into the IF clause.

We also move setUserState so that it executes after the Conditional.

/** ... */
public function getUserStateFromRequest($key, $request, $default = null, $type = 'none')
{
    $cur_state = $this->getUserState($key, $default);
    $new_state = $this->input->get($request, null, $type);

    if ($new_state === null)
    {
        $new_state = $cur_state;
    }

    $this->setUserState($key, $new_state);

    return $new_state;
}

This is good, but as we can see now ,getting the $cur_state is only relevant when the $new_state is null.

So we can remove the $cur_state variable and just overwrite the $new_state variable using $this->getUserState(…) inside the IF clause.

Refactored

/** ... */
public function getUserStateFromRequest($key, $request, $default = null, $type = 'none')
{
    $new_state = $this->input->get($request, null, $type);

    // If there no new state, use the current state
    if ($new_state === null)
    {
        $new_state = $this->getUserState($key, $default);
    }

    $this->setUserState($key, $new_state);

    return $new_state;
}

Now one thing that someone might point out is that the side effect has changed slightly compared to the original.

Indeed with this refactor the method now sets the user state whenever it is called. Which is a change in the execution algorithm. However since the content of the resulting side effect is the same, this change does not constitute a break in backwards compatibility.

Refactoring allowed us to see that having a “Getter” method that sets the user state is fundamentally illogical. However removing that line would be a BC break. So the next best thing is to make the behavior consistent. Which was already achieved by removing the redundant ELSE clause.

Moderately Difficult Example

The last two examples showed how to deal with “Conditional Defaults”.  In the next example I’ll show you how to deal with what I call “Conditional Logic”.

Conditional Logic is slightly more complicated to refactor than conditional defaults, but can still be achieved by even a novice programmer once they get the hang of it.

Original
libraries/cms/editor/editor.php lines 127 ~ 193 – PR #7993

/**
* Attach an observer object
*
* @param object $observer An observer object to attach
*
* @return void
*
* @since 1.5
*/
public function attach($observer)
{
    if (is_array($observer))
    {
        if (!isset($observer['handler']) 
            || !isset($observer['event']) 
            || !is_callable($observer['handler']))
        {
            return;
        }

        // Make sure we haven't already attached 
        // this array as an observer
        foreach ($this->_observers as $check)
        {
            if (is_array($check) 
                && $check['event'] == $observer['event'] 
                && $check['handler'] == $observer['handler'])
            {
               return;
            }
        }

        $this->_observers[] = $observer;
        end($this->_observers);
        $methods = array($observer['event']);
    }
    else
    {
        if (!($observer instanceof JEditor))
        {
            return;
        }

        // Make sure we haven't already attached 
        // this object as an observer
        $class = get_class($observer);

        foreach ($this->_observers as $check)
        {
            if ($check instanceof $class)
            {
                return;
            }
        }

        $this->_observers[] = $observer;
        $methods = array_diff(get_class_methods($observer), 
                              get_class_methods('JPlugin'));
    }

    $key = key($this->_observers);

    foreach ($methods as $method)
    {
        $method = strtolower($method);

        if (!isset($this->_methods[$method]))
        {
            $this->_methods[$method] = array();
        }

        $this->_methods[$method][] = $key;
    }
}

More often than not, Conditional logic is a sign that your method is doing too much. So the easiest way to resolve this is to break the function up into smaller functions. However this presents a possible backwards compatibility issue.

The core of this issue is visibility. While Adding new public methods is always a bad idea, because it makes it harder to see the intention of the interface. Adding a protected method could cause an unintentional break, because there is a remote chance that someone somewhere might have implemented a public method with the same name.

So that leaves us with one option. To break these up into private methods. Doing so has two really nice benefits. The first is that you don’t have to worry about backwards compatibility when you implement the change. The second is that you don’t have to worry about backwards compatibility when you depreciate or refactor the private methods.

Looking at the example we see that the attach method handling two scenarios. In the first scenario, an array will be passed in as the $observer input and in the second is an instance of JEditor will be passed as the $observer.

So to start we’ll create two private methods to handle each scenario and move the content of the IF/ELSE clauses to their respective method.

We also need to add a check for any of the early returns in the original code. Because I like to be explicit about my return types, I changed the void returns in the new private methods to null returns.

Honestly, I don’t know if technical there is a difference between void and null, but intentionally returning a null here guarantees that the is_null check will work as expected. Thus reducing the anxiety that ambiguity tends to illicit in programmers.

/** ... */
public function attach($observer)
{
    if (is_array($observer))
    {
        $methods = $this->attachArrayObserver($observer);
    }
    else
    {
        $methods = $this->attachJEditorObserver($observer);
    }

    if(is_null($methods)
    {
        return;
    }

    $key = key($this->_observers);

    foreach ($methods as $method)
    {
        $method = strtolower($method);

        if (!isset($this->_methods[$method]))
        {
            $this->_methods[$method] = array();
        }

        $this->_methods[$method][] = $key;
    }
}

private function attachArrayObserver($observer)
{
    if (!isset($observer['handler']) 
        || !isset($observer['event']) 
        || !is_callable($observer['handler']))
    {
        return null;
    }

     // Make sure we haven't already attached 
     // this array as an observer
     foreach ($this->_observers as $check)
     {
         if (is_array($check) 
             && $check['event'] == $observer['event'] 
             && $check['handler'] == $observer['handler'])
         {
            return null;
         }
     }

     $this->_observers[] = $observer;
     end($this->_observers);
     return array($observer['event']);
}

private function attachJEditorObserver($observer)
{
    if (!($observer instanceof JEditor))
    {
        return null;
    }

    // Make sure we haven't already attached 
    // this object as an observer
    $class = get_class($observer);

    foreach ($this->_observers as $check)
    {
        if ($check instanceof $class)
        {
            return null;
        }
    }

    $this->_observers[] = $observer;
    return array_diff(get_class_methods($observer), 
                      get_class_methods('JPlugin'));
}

As you can see this is a little better, but I can hear some of you saying, “But Matt, you didn’t get rid of the ELSE clause!” and you are right.

This refactor isn’t complete yet. But with this first step we’ve begun isolating the logic, which allows us to more easily understand what varies between the two scenarios.

As we can see that both methods follow a very similar pattern.

  1. First we do some sanity checks
  2. Next we search to make sure that the observer hasn’t already been set.
  3. Then we attach the observer
  4. Finally we return an array.

Since each one starts with a sanity checks, we’ll start there too.

if (!isset($observer['handler']) 
        || !isset($observer['event']) 
        || !is_callable($observer['handler']))
    {
        return null;
    }
...
    if (!($observer instanceof JEditor))
    {
        return null;
    }

Since they both have the same return value, duplicating these checks in both methods doesn’t make sense, so I’ll combine them and return them to the attach method.

I also give intention revealing names to the conditions, so I can skim the code 6 months from now and still understand what is going on.

/** ... */
public function attach($observer)
{
    $notValidArray = (!is_array($observer) 
                      || (!isset($observer['handler']) 
                          || !isset($observer['event']) 
                          || !is_callable($observer['handler'])
                          )
                      );

    $notValidObject = (!($observer instanceof JEditor));

    if($notValidArray && $notValidObject)
    {
        return;
    }

    if (is_array($observer))
    {
        $methods = $this->attachArrayObserver($observer);
    }
    else
    {
        $methods = $this->attachJEditorObserver($observer);
    }

    if(is_null($methods)
    {
        return;
    }

    $key = key($this->_observers);

    foreach ($methods as $method)
    {
        $method = strtolower($method);

        if (!isset($this->_methods[$method]))
        {
            $this->_methods[$method] = array();
        }

        $this->_methods[$method][] = $key;
    }
}

private function attachArrayObserver($observer)
{
     // Make sure we haven't already attached 
     // this array as an observer
     foreach ($this->_observers as $check)
     {
         if (is_array($check) 
             && $check['event'] == $observer['event'] 
             && $check['handler'] == $observer['handler'])
         {
            return null;
         }
     }

     $this->_observers[] = $observer;
     end($this->_observers);
     return array($observer['event']);
}

private function attachJEditorObserver($observer)
{
    // Make sure we haven't already attached 
    // this object as an observer
    $class = get_class($observer);

    foreach ($this->_observers as $check)
    {
        if ($check instanceof $class)
        {
            return null;
        }
    }

    $this->_observers[] = $observer;
    return array_diff(get_class_methods($observer), 
                      get_class_methods('JPlugin'));
}

Now we are free to move onto the next step.

// Make sure we haven't already attached 
     // this array as an observer
     foreach ($this->_observers as $check)
     {
         if (is_array($check) 
             && $check['event'] == $observer['event'] 
             && $check['handler'] == $observer['handler'])
         {
            return null;
         }
     }
...
    // Make sure we haven't already attached 
    // this object as an observer
    $class = get_class($observer);

    foreach ($this->_observers as $check)
    {
        if ($check instanceof $class)
        {
            return null;
        }
    }

looking closely we can see that these can also be combined rather easily.

So we move them back to the attach method like we did with the sanity checks. Also looking at how the observer is attached we can see that it is identical in both methods, so we move that back up to the attach method as well

/** ... */
public function attach($observer)
{
    $notValidArray = (!is_array($observer) 
                      || (!isset($observer['handler']) 
                          || !isset($observer['event']) 
                          || !is_callable($observer['handler'])
                          )
                      );

    $notValidObject = (!($observer instanceof JEditor));

    if($notValidArray && $notValidObject)
    {
        return;
    }

    // Make sure we haven't already attached this observer
    foreach ($this->_observers as $check)
    {
        $duplicateArray = (is_array($check) 
                            && $check['event'] == $observer['event'] 
                            && $check['handler'] == $observer['handler']);
        if ($isDuplicateArray)
	{
	    return;
	}

	$class = get_class($observer);
	if ($check instanceof $class)
	{
	    return;
	}
    }

    $this->_observers[] = $observer;

    if (is_array($observer))
    {
        $methods = $this->attachArrayObserver($observer);
    }
    else
    {
        $methods = $this->attachJEditorObserver($observer);
    }

    if(is_null($methods)
    {
        return;
    }

    $key = key($this->_observers);

    foreach ($methods as $method)
    {
        $method = strtolower($method);

        if (!isset($this->_methods[$method]))
        {
            $this->_methods[$method] = array();
        }

        $this->_methods[$method][] = $key;
    }
}

private function attachArrayObserver($observer)
{
     end($this->_observers);
     return array($observer['event']);
}

private function attachJEditorObserver($observer)
{
    return array_diff(get_class_methods($observer), 
                      get_class_methods('JPlugin'));
}

After all of that we can see that the only major difference between these two scenarios is how we return the methods.

Since having two methods for this doesn’t make sense, I combine them into one method and remove the IF/Else from the attach method. Below is the final refactored version.

We could take it even further by moving the validation checks and the duplication checks into their own methods, but for the purpose of this article, I think we’ve done enough.

Refactored

/** ... */
public function attach($observer)
{
    $notValidArray = (!is_array($observer) 
                      || (!isset($observer['handler']) 
                          || !isset($observer['event']) 
                          || !is_callable($observer['handler'])
                          )
                      );

    $notValidObject = (!($observer instanceof JEditor));

    if($notValidArray && $notValidObject)
    {
        return;
    }

    // Make sure we haven't already attached this observer
    foreach ($this->_observers as $check)
    {
        $duplicateArray = (is_array($check) 
                            && $check['event'] == $observer['event'] 
                            && $check['handler'] == $observer['handler']);
        if ($isDuplicateArray)
	{
	    return;
	}

	$class = get_class($observer);
	if ($check instanceof $class)
	{
	    return;
	}
    }

    $this->_observers[] = $observer;

    $methods = $this->getObserverMethods($observer);

    $key = key($this->_observers);

    foreach ($methods as $method)
    {
        $method = strtolower($method);

        if (!isset($this->_methods[$method]))
        {
            $this->_methods[$method] = array();
        }

        $this->_methods[$method][] = $key;
    }
}

private function getObserverMethods($observer)
{
    if(is_array($observer)
    {
        end($this->_observers);
        return array($observer['event']);
    }
    
    return array_diff(get_class_methods($observer), 
                      get_class_methods('JPlugin'));
}

Conclusion

As you can see removing redundant ELSE clauses makes it easier to understand the code. It also makes it easier to identify flawed logic, duplication of effort and bugs with in a function. This equates to greater stability.

ELSE clauses make things look more complicated that they really are and by removing them we can clearly see the execution path as a whole which gives us a chance to think of better solutions. Or in other words it makes the code base more adaptable.

Although this isn’t as exciting as adding a new feature, removing the ELSE clause has a major impact on code clarity, stability and adaptability. By removing the ELSE clause from Joomla or any code base, you’re helping to make both your life and the life of others developers better.

Thanks for reading and I hope you’ll leave some comments and feedback below.  Happy Joomla!ng =^D

Published by

Mathew

Born in the Rocky mountains of western Montana, Mathew now lives in Japan where he is the active "Crazy Foreign Guy" of a small town in the Tohoku region on the main island of Honshu. He is a father, a husband and a hopeless dreamer who believes that it is better to fail at life than to never try to live.

2 thoughts on “A Simple Way to Improve Joomla Today.”

  1. Other approach to the removal of else in the getUserStateFromRequest is to use an early return. This way you are not doing a setUserState when it isn’t necessary.

    public function getUserStateFromRequest($key, $request, $default = null, $type = 'none')
    {
        $cur_state = $this->getUserState($key, $default);
        $new_state = $this->input->get($request, null, $type);
    
        if ($new_state === null)
        {
            return $cur_state;
        }
    
        // Save the new value only if it was set in this request.
        $this->setUserState($key, $new_state);
    
        return $new_state;
    }
    
    1. Peter thanks for commenting! I swear you’re like a brother in arms =^D Clean coders of the world unite! lol

      I defiantly like your refactor more than the original example. Using the early return the function works exactly as it did before! B-E-A-utiful

Leave a Reply

Your email address will not be published. Required fields are marked *