Falling Into the Complexity Trap

So many times I find myself, as a programmer, diving into a problem with an initial thought of a solution that ends up being way too complex. I don’t know if it’s programmers in general or just me, but it seems that as I gain more and more coding knowledge and can take on more and more challenging problems, my brain can fall into a complexity trap. What I mean by that is I now have numerous tools (algorithms, patterns, etc.) at my disposal, and at times I tend to attack a problem with one or more of these tools without stepping back, taking a deep breath, and determining which is the simplest way to get something done.

These solutions usually work, but the code can become messy and hard to maintain versus the simple way that I failed to see at the beginning.

I could probably make this a series of articles, as unfortunately, I tend to do this more than I’d like. But perhaps, writing this down, will force me to think about this issue more and prevent further frustrations.

OK, so now for a real world example. I’ve been recently working on an admin panel for a web app using PHP and Javascript with the Prototype.js library. There is a section of the admin panel where the user can create polls. The user enters a question along with one or more answers and can then re-sort the answers if desired. The re-sorting issue is where I started losing control.

Each answer is stored in an answers table in the database, and to keep it simple, we’ll just define three columns:

id
answer
sort_order

For the UI, the answers look something like this:

As you would expect, the up and down arrows allow you to move the answers up and down the list, effectively changing their sort order in the database.

So I immediately started down the road of reacting to the click of an arrow button by determining the current order of answers, swapping the sort orders in the database, getting the new order and manipulating the HTML markup to show the new order. Here’s the code (Note: this is just for moving an answer up the list. I had a separate, similar function for moving an answer down. I hadn’t refactored yet):

function upAnswer(id) {
	//get sort order for this row
	var sortOrder = $('orderAnswer_' + id).innerHTML;

	//if this is the first row, no need to move it up
	if (sortOrder == lowestOrder) {
		return;
	}
	
	//find the row immediately above the selected row
	var idAbove = 0;
	$$('.answerRow').each(function(i) {
		var checkId = i.readAttribute('id').substr(7);

		if ( checkId == id ) {
			throw $break;
		}
		else {
			idAbove = checkId;
		}
	});
	
	// swap the display orders in the database
	new Ajax.Request('/AdminPoll/SwapAnswers/' + id + '/' + idAbove, {
		onSuccess: function(response) {
			
			//store the answer this is going to be moved
			var moveAnswer = $('answer_' + id);
			
			//store both display orders
			var origOrder = $('orderAnswer_' + id).innerHTML;
			var targetOrder = $('orderAnswer_' + idAbove).innerHTML;
			
			//insert the moved answer in the right place
			$('answer_' + idAbove).insert({before:
				'' + moveAnswer.innerHTML + ''
			});
			
			//remove the moved answer
			moveAnswer.remove();
			
			//swap the display orders on the page
			$('orderAnswer_' + id).update(targetOrder);
			$('orderAnswer_' + idAbove).update(origOrder);
			
			//hook up the action buttons
			assignActions(id);
		},
		onFailure: function(response) {
			alert('Unable to move answer');
		}
	});
}

The code above can be summarized in the following steps:

Determine the sort order for the row just clicked
Since we are in the “move up” function, exit the function if this row is already at the top
Loop through the rows to find the row immediately above this one
We now know the two rows involved in the reordering, so call the database to swap the two sort_order values
Copy the current row and move it to the position above the row just swapped with
Delete the original answer
Since we only moved the markup, call the assignActions function which wires up all of the events to the new input control and buttons

Yikes. Knowing that this was getting ugly, fast, I stepped back and looked at it again. Two key concepts drove the next iteration:

There is no need to maintain the actual sort orders. Just make sure the answers stay in order. For example, if the current order is
- [answer1 => 5, answer2 => 8, answer3 => 9]
and we are going to swap the first two answers, the new order does not have to be
- [answer2 => 5, answer1 => 8, answer3 => 9]
It can be
- [answer2 => 1, answer1 => 2, answer3 => 3]
Change thinking from altering the database first then the markup. Instead, swap the two rows in markup, determine what happened, then send the new order to the database.

Here’s the new code:

function moveAnswer(id, moveUp) {
	//get the answer element that is going to be moved
	var answerSource = $('answer_' + id);
	
	//get the current order of the list
	var answerOrder = new Array();
	var iCounter = 0;
	$$('.answerRow').each(function(i) {
		answerOrder[iCounter++] = i.readAttribute('id').substr(i.readAttribute('id').indexOf('_') + 1);
	});
	
	if (moveUp) {
		//make sure that the answer the user wants moved isn't already at the top of the list
		if (id == answerOrder[0]) {
			return;
		}
		else {
			//get the element above the source element
			var answerTarget = $('answer_' + id).previous();
			answerTarget.insert({
				before: answerSource
			});
		}
	}
	//move down
	else {
		if (id == answerOrder[answerOrder.length - 1]) {
			return;
		}
		else {
			//get the element below the source element
			var answerTarget = $('answer_' + id).next();
			
			answerTarget.insert({
				after: answerSource
			});
		}
	}
	
	resortAnswers();
}

function resortAnswers() {
	//get the order of the answer ids
	var answerOrder = new Array();
	
	$$('.answerRow').each(function(i) {
		if (i.readAttribute('id') != null) {
			answerOrder.push(i.readAttribute('id').substr(i.readAttribute('id').indexOf('_') + 1));
		}
	});
	
	if (answerOrder.length > 1) {
		new Ajax.Request('/AdminPoll/SortAnswers/' + answerOrder.join(), {
			onSuccess: function(response) {
			},
			onFailure: function(response) {
				alert('Unable to sort answers');
			}
		});
	}
}

First, note that this function handles moves both up and down by passing in a boolean. Here is a summary of the new code:

Get the answer that’s going to be moved
Get the current order of answers
If the answer is not at the end of the list already, insert it before or after the answer next to it, depending on which way we are moving
Call the database to store the new order

Ahhh, this feels so much better. As you may have noticed, the new code really isn’t any smaller (although there’s still improvement to be made), but it is much cleaner and handles the resorting more efficiently.

A couple of key indicators that I’m about to fall into the complexity trap and should stop me in my tracks are:

The code is getting ugly
The nagging feeling that there’s probably a function out there to handle something I’m trying to work out (in this case, Prototype’s next() and previous() functions)

It felt good to get back in there and clean up that code. But if I can be more aware of the warning signs, maybe I can do it right in the first place.

Leave a Reply Cancel reply