bookmark_borderStuff I’ve learned #03

Another week has passed:

  • Unlike in Windows; in Chrome you cannot easily focus on your bookmarks bar with a keyboard short key on Mac OS X.
  • If you want to run rake tasks in your specs in a before block, be sure to set a line
    Rake::Task[name].reenable

    so you can re-execute them every time. Rake seems to remember which task has been executed, so you cannot execute it twice.

  • If you want to stub out STDOUT messages (like with ‘puts’) in your spec, use:
    STDOUT.stubs(:puts)
  • When in doubt, speak up. Always.
  • With Scrum, big stories are big risks. Split them up.
  • Don’t use PID files to remember which proces has been started and when it should be stopped. Especially if you want to reboot a deamon process automatically once it has died. Instead wait for it when the deamon has quit and act upon a not-normal exit code.
  • Sometimes using ‘git fetch -p’ is not enough to prune all your local branches (which do not exist anymore on remote). You can use a rather long command (see below, from stackoverflow question)
    git branch -r | awk '{print $1}' | egrep -v -f /dev/fd/0 <(git branch -vv | grep origin) | awk '{print $1}' | xargs git branch -d
  • With editorconfig (*) you can create code formatting rules, nothing new here, but editorconfig has plugins for a lot of known editors, (I tested it in Vim & Sublime), meaning you can now share these rules cross-editor. Now that is cool!
  • With C++, when your function argument is using const, and you’re calling a non-const function on that argument you will end up with a message like:

    “error: passing ‘const xxx’ as ‘xxx’ argument of ‘function you where trying to call on xxx’ discards qualifiers”.

    You can fix this by telling the function body is const:

     bool myFunction() const { /* code here */ } 

* Thx to Arjen about editorconfig.

bookmark_borderA (C++) makefile using a maven like directory structure

When you’re used to Java, you build your software using the comforting build-tools. Popular examples are Ant and Maven. Personally I love Maven.

When you’re writing an application in C++ it seems like a big step backwards build-tools wise. If you’re an Ant God you might feel a bit more comfortable with using Make. However, writing makefiles (the ‘equivalent’ of the Ant’s build.xml) are a more tough.

Since I am used to Maven, there are some tasks I want to perform, like:

– clean
– compile
– compile tests
– package

To start with. I have set up a little makefile that does this for me and for those who just want to get started, share the makefile with you. But what will you get? It won’t be exactly like Maven

It took a little while, and I bet it is far from perfect for real C++ developers who eat makefiles for breakfast. But I am happy with it for now.

The directory structure that needs to be in your project is like this (cpp vs mavenized):

CPP 					Mavenized
------------------------------------------
srcmain 				srcmainjava
srcmainresources 		srcmainresources
srcinclude			none
srctest 				srctestjava
binmain 				target
bintest 				targettest-classes
lib 					srcmainresources

When you run ‘make’ you will be doing all tasks. Meaning you will clean, compile everything (your ‘normal’ code and test code) and then copy any libraries, resources and binaries.

Caveats:
– I am compiling without using the -c flag, this simplifies a lot of stuff for me (it works). But, the larger your project the longer it will take to compile, since it will compile everything all the time. So even though you change one file, everything will be recompiled.
– You need to specify sub-dirs when you use them, when there are no files in those dirs the compilation fails (atleast using G++)

The makefile:

CC=g++
CFLAGS=-Wall

# MAIN properties
SRC=src/main
INCLUDE=src/include
BIN=bin/main
RESOURCES=src/main/resources
LIB=lib

# TEST properties
SRC_TEST=src/test
BIN_TEST=bin/test
RESOURCES_TEST=src/test/resources

all: clean compile compile-test copy-resources copy-libraries

bin: clean compile copy-resources copy-libraries

copy-libraries:
	cp $(LIB)/*.* $(BIN)

copy-resources:
	cp -R $(RESOURCES)/ $(BIN)

compile: clean-main prepare-main
	$(CC) $(SRC)/*.cpp $(SRC)/domain/*.cpp -I$(INCLUDE) -I$(INCLUDE)/domain -o $(BIN)/d2tm -lmingw32 -lSDLmain -lSDL $(CFLAGS)

compile-test: clean-test prepare-test
	$(CC) $(SRC_TEST)/*.cpp -I$(INCLUDE) -o $(BIN_TEST)/tests $(CFLAGS)

clean:	clean-main	clean-test

clean-main:
	rm -rf binmain

prepare-main:
	mkdir binmain

clean-test:
	rm -rf bintest

prepare-test:
	mkdir bintest

Want to see how it is used in a project? Look here.

bookmark_borderAn example of refactoring

As I have promised in my previous post, I would post an example of small refactorings in order to greatly improve the readability and understandability of code.

I own a little project called Dune II – The Maker, and I started writing it a little over 10 years ago. In those years I have learned a lot. I did not have much time in those days to apply my new knowledge to the project. You could say the software was rotting. In order to make it better I need to refactor a lot and I encounter the best examples to improve code without pointing fingers :). In any case I have experienced you have to make mistakes in order to get better. I hope you will learn from the mistakes I made.

So here is a little example I have just checked in the dune2themaker repository, I’ll give you the before (revision 411) and after (revision 412). Of course, I have taken smaller steps to get to the end result. First the original piece of code:

Revision 411 (before)

void cGame::think_winlose() {
	bool bSucces = false;
	bool bFailed = true;

	// determine if player is still alive
	for (int i = 0; i < MAX_STRUCTURES; i++)
		if (structure[i])
			if (structure[i]->getOwner() == 0) {
				bFailed = false; // no, we are not failing just yet
				break;
			}

	// determine if any unit is found
	if (bFailed) {
		// check if any unit is ours, if not, we have a problem (airborn does not count)
		for (int i = 0; i < MAX_UNITS; i++)
			if (unit[i].isValid())
				if (unit[i].iPlayer == 0) {
					bFailed = false;
					break;
				}
	}

	// win by money quota
	if (iWinQuota > 0) {
		if (player[0].credits >= iWinQuota) {
			// won!
			bSucces = true;
		}
	} else {
		// determine if any player (except sandworm) is dead
		bool bAllDead = true;
		for (int i = 0; i < MAX_STRUCTURES; i++)
			if (structure[i])
				if (structure[i]->getOwner() > 0 && structure[i]->getOwner()
						!= AI_WORM) {
					bAllDead = false;
					break;
				}

		if (bAllDead) {
			// check units now
			for (int i = 0; i < MAX_UNITS; i++)
				if (unit[i].isValid())
					if (unit[i].iPlayer > 0 && unit[i].iPlayer != AI_WORM)
						if (units[unit[i].iType].airborn == false) {
							bAllDead = false;
							break;
						}

		}

		if (bAllDead)
			bSucces = true;

	}

	// On succes...
	if (bSucces) {
		// <snip>

	}

	if (bFailed) {
		// <snip>

	}
}

The intention of the think_winlose() function is to determine if the player has won or lost, and if so it transitions the game state. These transitions have been snipped.

So when does a player win or lose? It depends if there is a ‘win quota’, or not. The win quota is a number, whenever it is above zero it means the player has to collect at least that many of credits (spice) in order to win. If the win quota is not set, the default win rule : destroy everything of the enemy, will be used. (do you notice I need this much text for just a simple rule? Which I could have prevented If I had code that said this in the first place? At the bottom of this post you can see what I mean :))

Lets take a look at the code and point out what could be done better:

  • There are two booleans bSuccess and bFailed. Which is confusing and ambigious. What is succesfull? What did fail? Why aren’t they one boolean?
  • There are comments all over the place, meaning we could refactor these pieces to code so comments are not needed. (Comments are seen as clutter and should be removed)
  • The code formatting could be done better. If statements should start with { and end with }, even with one line.

And there are more things you will probably find yourself. What I’ll do is point out a few things that could be improved. If you just want to see the final result, just take a look below.

Lets start with the booleans bSuccess and bFailed. Why are there two booleans and whey are they called so vaguely? A little bit of searching in the code and we find out that bSuccess actually means “Mission is accomplished” (player has won), and bFailed means the player has no units and structures (which implicates the player has lost the game). They are not the same boolean, because a player could be alive and not have yet won the game of course. Now we know they are not actually the same boolean, but their naming was vague. A simple “rename variable” made things easier to understand!

void cGame::think_winlose() {
	bool bMissionAccomplished = false;
	bool isPlayerAlive= true;

(when posting this I realize the two booleans are named differently, consistency is also important to improve readability, so either both should start with “is” or both with a “b”, I prefer the first though)

Right after the booleans a few for loops are used just to find out if there is anything alive for the player. A little bit below we see such for loops again, but for the AI. This is duplicate code and should be removed. Extracting them into a method and make them return a boolean value is easy to do:

bool cGame::playerHasAnyStructures(int iPlayerId) {
    for (int i = 0; i < MAX_STRUCTURES; i++) {
		if (structure[i]) {
			if (structure[i]->getOwner() == iPlayerId) {
				return true;
			}
		}
	}
    return false;
}

(Again, while posting this I realize this could be even improved a bit more, the iPlayerId should be called ownerId (or the getOwner should be a getPlayerId), so it is obvious we match two of the same kind. Now it could confuse us: is an owner the same as the playerId? Since I know it is, why isn’t it called that way?… :))

Since we extract these for loops we can now set the isPlayerAlive boolean immidiately instead of setting a variable within the loop as it was done in the original example above. Reducing 24 lines into one!:

bool isPlayerAlive = playerHasAnyStructures(HUMAN) || playerHasAnyGroundUnits(HUMAN);

The final result of revision 412 is shown below. It will clearly show the major improvement regarding readability and understandability. Any other developer who comes to this code can see what it does and it is almost a no-brainer.

Result revision 412

void cGame::think_winlose() {
	bool bMissionAccomplished = false;
	bool isPlayerAlive = playerHasAnyStructures(HUMAN) || playerHasAnyGroundUnits(HUMAN);

    if (isWinQuotaSet()) {
		bMissionAccomplished = playerHasMetQuota(HUMAN);
	} else {
		bool isAnyAIPlayerAlive = false;
		for (int i = (HUMAN + 1); i < AI_WORM; i++ ) {
			if (playerHasAnyStructures(i) || playerHasAnyGroundUnits(i)) {
				isAnyAIPlayerAlive = true;
				break;
			}
		}

		bMissionAccomplished = !isAnyAIPlayerAlive;
	}

    if (bMissionAccomplished) {
		// <snip>
		
	} else if (!isPlayerAlive) {
		// <snip>

	}
}

bookmark_borderOverriding and methods

Today I had a little challenge. I had a Class, I call it Class A. It has a method, i call it “doSomething”. Class B extends Class A, and overrides this method with a totally new behavior. Class C who extends from Class B wants to have the original behavior from Class A.

Here is a picture:

The Problem

Now, the simplest but most stupid way to solve this is using copy / paste. Yes, you simply copy the contents of the method of Class A and paste it into the method of Class C and you’re all set right?

Wrong!

It might work for a while, but one of the first things you *should* be feel itchy about, is code duplication! (along with its problems you get when you want to maintain your code).

So what now?

Solution : Use method for shared behavior
Solution : Use method for shared behavior

Well, when you took the easy road and duplicated your code, you probably wanted to get rid of that duplicate code immidiatly… And how do you do that? .. Yes,  you create a new method which is put in Class A , and accessible from Class C and Bam! Code Duplication gone, and you got what you want…

Another way is to re-think your class hierarchy. You might want to consider to do this:

 

Solution : Change in hierarchy, extend C from A, B from C
Solution : Change in hierarchy, extend C from A, B from C

 

 

Yes, you’ve seen it right. Perhaps you can swap Class B and C.  So C extends now from A, and B from C.

But, if you do that, be careful. You need to know exactly what kind of behavior you wanted in Class B. Most likely you have changed that now by extending from C. Take a good look at what methods B was originally calling from Class A, and if it now calls an overridden method by Class C.

These 2 solutions came up today. For my particular problem I’ve used the first. (no, not copy paste smart ass)

I think the first solution is the easiest, I would not recommend anyone to do the second solution unless you really know what you are doing. If you encounter more of these problems like above, swapping might be better for you.  

Do you got another (better?) solution? Let me know!