It appears that you have JavaScript disabled. Click here to find out what you're missing on this site.

BitWorking Theories on software development

by Joe Gregorio

Refactor, Refactor, Refactor...

The first order of business is to break out our two classes into their own H files and CPP files. Once things are broken out UnitTest is the first to go under the refactoring knife. The UnitTest::results() function is a bit messy.

	map<string, bool>::iterator i;
	int totalTests = 0;
	int totalPassed = 0;
	for(i = testResults_.begin(); i != testResults_.end(); i++) {
		o << (*i).first << "   " << ((*i).second  ? "Passed" : "Failed") << endl;
		totalTests++;
		totalPassed += ((*i).second  ? 1 : 0);
	}
	o << endl;
	o << "Total Test Cases: " << totalTests << endl;
	o << "Total Test Cases Passed: " << totalPassed << endl;
	if (totalTests != totalPassed) {
		o << "100% of the test cases must pass before release!" << endl;
	}

Two tasks are being done in the for loop, adding up the scores, and displaying the results of the individual tests. These are two different activities and need to be broken up.

First break out adding up the scores. Looping over a container and adding up stuff is the perfect application of the accumulate template. To apply accumulate we will need a helper function.

int incIfPassed(int score, const pair & unitTestResult) {
	return score + unitTestResult.second;
}

Now to get the total number of tests that passed:

	unsigned int totalPassed = accumulate(testResults_.begin(), testResults_.end(), 0, incIfPassed);

Next break out printing the results of the individual results. Adding the ability to stream out pair<string, bool> simplifies the matter. This function encapsulates how we want to format an individual entry in testResults_:

ostream & operator<<(ostream & o, const pair & unitTestResult) {
	o << unitTestResult.first << "   " << (unitTestResult.second  ? "Passed" : "Failed");
	return o;
}

Now to print out the whole container we just copy the contents of testResults_ to cout, using ostream_iterator to turn the stream cout into an iterator.

    copy(testResults_.begin(), testResults_.end(), ostream_iterator<pair<string, bool> >(cout, "\n"));

Now our refactored UnitTest::results() is much cleaner:

void UnitTest::results(ostream & o) {
	unsigned int totalPassed = accumulate(testResults_.begin(), testResults_.end(), 0, incIfPassed);
   		
	copy(testResults_.begin(), testResults_.end(), ostream_iterator<pair<string, bool> >(cout, "\n"));
	o << endl << "Total Test Cases: " << testResults_.size() << endl;
	o << "Total Test Cases Passed: " << totalPassed << endl;
	if (testResults_.size() != totalPassed) {
		o << "100% of the test cases must pass before release!" << endl;
	}		
}

Now look back at forth.cpp. The parsing method and token lookup are a bit crude. Start by adding the "*" operator, this will highlight the difficulties in the current approach. First add the test case:

	f.execute("2 3 *");
	test.score(f.stack_.front() == 6, "Multiplication");

Compiling and running causes the unit tests to fail since we haven't added any code to implement "*".

	Addition   Passed
	Multiplication   Failed
	StackSize   Passed
   		
	Total Test Cases: 3
	Total Test Cases Passed: 2
	100% of the test cases must pass before release!

Our multiplication test case fails as expected. (This actually displays an ugly little problem itself. Why is Multiplication in the middle and not at the end if the list of tests when it was the last test we did? Test results are stored in a map<> so we have no guarantee on the order they are going to come out. That needs to be fixed. Something to save for the next installment.)

Add code to Forth::execute() to make it pass. The old code:

    bool Forth::execute(const string & command) {
	bool returnValue = true;
   				
	char * s = strdup(command.c_str());
	char * token = strtok(s, " ");
	while (token) {
		if (string("+") == token) {
			int num1 = stack_.front();
			stack_.pop_front();
			int num2 = stack_.front();
			stack_.pop_front();
			stack_.push_front(num1 + num2);
		} else {
			stack_.push_front(atoi(token));
		}
		token = strtok(NULL, " ");
	}
	return returnValue;
    }

now becomes:

    bool Forth::execute(const string & command) {
	bool returnValue = true;
   				
	char * s = strdup(command.c_str());
	char * token = strtok(s, " ");
	while (token) {
		if (string("+") == token) {
			int num1 = stack_.front();
			stack_.pop_front();
			int num2 = stack_.front();
			stack_.pop_front();
			stack_.push_front(num1 + num2);
		} else if (string("*") == token) {
			int num1 = stack_.front();
			stack_.pop_front();
			int num2 = stack_.front();
			stack_.pop_front();
			stack_.push_front(num1 * num2);
		} else {
			stack_.push_front(atoi(token));
		}
		token = strtok(NULL, " ");
	}
	return returnValue;
    }

and now our test cases pass:

Addition   Passed
Multiplication   Passed
StackSize   Passed
   		
Total Test Cases: 3
Total Test Cases Passed: 3

So now things are starting to get ugly. We need a way to add new functions without making Forth::execute() longer and longer. A simple solution is to create a dictionary, a map from function names to straight C functions that implement the functionality. One item to note in the implementation of "+" and "*" is that they change the state of forth as they execute, so the straight C functions will need to be passed a pointer or reference to the instance of Forth that they are operating on. The changes to Forth.h add a typedef for Forth_Function, the dictionary_ member variable and the addFunction() member function:

class Forth;
typedef bool (*Forth_Function)(Forth &);
   		
class Forth {
  private:
	map dictionary_;
  public:
	bool addFunction(const string & name, Forth_Function);

The implementation of addFunction is very simple:

    void Forth::addFunction(const string & name, Forth_Function fun) {
	dictionary_[name] = fun;
    }

And allows a vastly simplified Forth::execute():

    bool Forth::execute(const string & command) {
	bool returnValue = true;
   		
	char * s = strdup(command.c_str());
	char * token = strtok(s, " ");
	while (token) {
		if (dictionary_.find(token) != dictionary_.end()) {
			returnValue = (*dictionary_[token])(*this);		
		} else {
			stack_.push_front(atoi(token));
		}
		token = strtok(NULL, " ");
	}
	return returnValue;
    }

Compiling and running confirms that all the tests pass. This is a very important step and higlights the importance of a testing framework even this early in the project. Major surgery was just performed on the class Forth and because unit test cases were already written we can do a regression test to confirm that the new class behaves like the old class.

Next installment we go back to UnitTest and substitute map for a better data structure, then we'll go on a function adding frenzy for Forth: -, /, mod, drop, dup, over and pick.