Spring cleaning code: Or deleting code that was actually important

People are good at gathering things. Likewise, systems are good at gathering features. Rarely do we consider if we need all the things around us or if the system needs all those features. I got the chance to mull over long-lived codebases when a customer reported an error. Something was wrong with the article save function.

When faced with an error many peoples' eyes glaze over and they become blind to it. It happens to everyone, even coders. But what does the error say, really?

In this case it said that the web page code had trouble talking to Google Maps.

My guess was that some code was set to run every time someone saved an event. So, I went to look at the active plugins in the web site, and there was one plugin that stood out to me named “barecraft utilities”. This plugin carried the same name as the codebase itself. Maybe there was some custom code there quite specific to this web page?

So, I delved into the folders to uncover the custom plugin.

And what greeted me was this.

(Just keep scrolling and you’ll find a commented version below. :))


<?php
namespace Craft;

require_once 'vendor/autoload.php';

class BarecraftPlugin extends BasePlugin
{
	public function init() {
		craft()->on('entries.saveEntry', function(Event $event) {
			$curl = new \Ivory\HttpAdapter\CurlHttpAdapter();
			$geocoder = new \Geocoder\Provider\GoogleMaps($curl);

			$entry = $event->params['entry'];
			if ($entry->section->handle == 'events') {
				$place = $entry->place->first();
				$block = craft()->matrix->getBlockById($place->id);
				if ($block && $block->type == 'placeName') {
					$address = trim($block->address . ' ' . $block->postalcode . ' ' . $block->city);
					if (empty($address)) {
						return;
					}
					$result = $geocoder->geocode($address);
					if (count($result) > 0) {}
						$latlng = $result->first();

						$block->getContent()->setAttributes([
							'latitude' => $latlng->getLatitude(),
							'longitude' => $latlng->getLongitude()
						]);

						craft()->matrix->saveBlock($block);
					}
				}
			}
		);
	}
	private $_name = 'Barecraft Utilities';
	private $_developer = 'Netlife Research';
	private $_developerUrl = 'http://netliferesearch.com';
	private $_version = '1.0';

	public function getName()
	{
		return Craft::t( $this->_name );
	}

	public function getVersion()
	{
		return $this->_version;
	}

	public function getDeveloper()
	{
		return $this->_developer;
	}

	public function getDeveloperUrl()
	{
		return $this->_developerUrl;
	}
}

This was the whole plugin.

If we stare at the code in the init method we can get a little wiser about what it tries to do. I’ve added some comments to clarify. Do read on:

public function init() {
  # "Hey CMS! Please call this code if an entry is saved
  # and when you call this function be sure to pass in
  # an event object that describes what is being saved."
  craft()->on('entries.saveEntry', function(Event $event) {
    # Prepare some code that lets us call other websites.
    $curl = new \Ivory\HttpAdapter\CurlHttpAdapter();
    # Pass the "other-site-caller" code to a Geocoder service
    # so that it may communicate to another site.
    $geocoder = new \Geocoder\Provider\GoogleMaps($curl);
    # Drill into the save event to fetch the thing being saved
    $entry = $event->params['entry'];
    # is the thing being saved an 'events' thing by happenstance?
    if ($entry->section->handle == 'events') {
      # Find 'place' data connected to the event entry
      $place = $entry->place->first();
      # use the place data id to fetch more data.
      $block = craft()->matrix->getBlockById($place->id);
      # check if the fetched data happens to be placeName
      if ($block && $block->type == 'placeName') {
        # if it is a placeName block we might build an address.
        $address = trim($block->address . ' ' . $block->postalcode . ' ' . $block->city);
        if (empty($address)) {
          # if despite our best assembling of data,
          # we arrive at an empty address,
          # we decide to end our quest here.
          return; # exit, and and do not run any code below.
        }
        # If we have come this far we must have something resembling
        # an address. Time to let the Geocode service shine! We
        # call the service with an address and eagerly await the
        # result
        $result = $geocoder->geocode($address);
        # This next line does nothing (?). *Puzzled Jedi hand-wave*
        if (count($result) > 0) {}
        # The result seems to be a list, so grab the first thing
        # in the list.
        $latlng = $result->first();
        # Fetch the content of the block that we got through
        # the 'place' data connected to the event entry, and
        # modify that content by editing latitude and longitude
        # based on the response we got from the Geocode service.
        $block->getContent()->setAttributes([
          'latitude' => $latlng->getLatitude(),
          'longitude' => $latlng->getLongitude()
        ]);
        # Take the block data we just edited and save it!
        craft()->matrix->saveBlock($block);
      }
    }
  });
}

What happened next was three things.

First, I realised that Google was probably pushing back against this code because it was calling the Google map service too often. This could be fixed by getting a Google key but the documentation for the Geocoder library was a bit puzzling I must admit. Second, I knew about a robust SimpleMap plugin that could be used here. Third and finally I asked, “but what is this latitude and longitude data really used for?”

I searched the codebase for the use of latitude and longitude and discovered that it was not really used anywhere, despite it being computed and saved by this plugin.

Now you might say, “how could this have happened?” Well, not all features get completed in a sprint, and sometimes you’re left with a good foundation and some incomplete code. Sprinting is about prioritizing tasks all the while knowing that tasks at the bottom of the task list risk not being completed in this sprint.

I got on the phone with the client to learn what their current need was. By consulting the client and checking the code I concluded that we were asking users to fill out a bit too much when creating new events. And the latitude and longitude data points were not currently used anywhere.

So, I deleted that code, made event creation more ergonomic and fixed the bug.

Edit: I later realized I broke some other functionality elsewhere on the site. :(( So, I had to revert the code change and dial back the database to a backup. Thankfully it did not lead to data loss. And I ended up mending the code instead by adding a proper Google api key so that the calls wouldn’t get rejected by Google anymore. At first this article was meant to be a “yay for spring cleaning a codebase”. Now it’s more like, “yay for spring cleaning just don’t delete vital functionality. Best regards, one humbled programmer.”