An initial refactoring

This is the first technical post in my new JOGRE series . As mentioned in the previous article, the goal behind this project is to explore how App Engine can efficiently host online games that scale to many users. Since I do not want to reinvent the wheel, I am basing the work on an existing Java-based platform (http://jogre.sourceforge.net/main.htm) that already happens to come with a lot of prewritten games. How hard is it going to be to retrofit that base to the App Engine platform? Over the next couple of weeks, I will hopefully find out.

The post is a little bit longer than usual, so I'll do my best to put in a few meaningful headlines inbetween. This way, readers can skip to the parts that are of particular interest to them.

A hobbyist's code of conduct to refactoring

A lot of my activities over the next couple of weeks will be about refactoring code in JOGRE, so I'd like to start out by summarizing the guidelines that I will try to follow in this work. Note that the "you" in the following paragraphs does not address the reader but myself. I know -- talking to myself is a bad habit, but it makes it easier to write the rules down. I will to try my best to abide by them in this and other refactoring posts.

The cardinal rule: Don't be a jerk

Life's too short to get angry, so why would anyone want to work with someone he or she does not like? This is even more important to consider when there is no transfer of money involved: people work on this kind of software because they have a passion for it and it is fun. So, try to avoid spoiling the project for anyone else!

Corollary #1: If it ain't broke, don't fix it.

You might have an extensive background in methodology xyz. You might have read the GOF book, Martin Fowler's Refactoring, and you might have quoted from Effective Java on occasion. You may think you know how to spot "code smells", or that you have read and written a ton of code. Good for you, but do yourself a favor: forget all of this right now! Unless you are a glutton for punishment, you have chosen to work on this project in your spare time because you like it, it works well 90% of the time, and you'd like to make it even better. This means that if you run into something that does not comply with what you might consider quasi-standards of good coding, it is probably like that for one of the following reasons:

• The code has evolved historically towards the way it currently is, and it has never been too much of a problem.
  
• The project team has a different philosophy or architectural view towards development than you. Happens all the time, and that doesn't make it bad code. Feel free to ask the team why it is the way it is, but do not expect them to change it for you.
  
• You ran into a bug or an area with "TODOs". If it's an issue and the team agrees, feel free to improve it -- as long as you do it with the consent of the others.
  

Bear in mind that long before you joined the project, other people have spent a lot of their spare time contributing and building the foundation of what you are using right now. Criticizing the code or calling any of it "smelly" could be considered impolite or disrespectful to other contributors.

Corollary #2: Honor the code's spirit and document the intentions of changes you make.

It's funny 'cause it's true: nobody should be looking at your changes and ask, "why the heck did he do that?!?" There are several things that can significantly increase the WTFs per minute ratio:

• changes that would make sense -- if the author had only documented why he or she is doing them
• too many concurrent modifications in one iteration
• combining a refactoring with writing new features
• incomplete or speculative refactorings (in other words, starting a change because "it might come in handy later" but never following through on that)
• major changes to the contract of a core class that require code changes throughout the project
• random changes just for the heck of it, or because one does not like the way a particular piece of code looks (see corollary #1). While it sometimes makes sense to clean up something small while you're in a particular class anyway, there is a fine line between cleanup and major modifications.

Refactorings can be like solving a Rubik's cube: while the overall change might be complex, it can usually be comprised of several smaller transformations, each of them simple and easy to understand. It often makes sense to document these smaller steps by checking them in independently into revision control (for example, today's refactoring steps can be traced from revisions 5 till 16 of the open source project.

Corollary #3: Contributorship does not imply ownership

Your open source work is branched off an existing project (Note: I contacted Bob Marks before I started any of this. We agreed to keep my work separate for now, but if I am successful and he finds the changes useful and beneficial, we will merge them back into the main project). The overall amount of changes you are going to make will most likely affect not even ten percent of the overall codebase. The new features you add will probably be even fewer. Therefore, do not imply that you "built" this platform, or forget to give credit where credit is due. If you build a new feature, feel free to put your name into the author tag -- but if all you are doing is pulling code from one class into an other class, that doesn't make you the author. Give credit where credit is due! Also, do not expect that your changes will definitely make it back verbatim into the main branch (in other words, don't get defensive if the other coders require some additional modifications first). Do not remove any branding that the original authors might have put in and replace it with your own -- it might be ok to do that in customized deploys, but not in the open source project itself. That being said -- of course you are free to take some liberties with the project, as long as you comply with its open source license (GPL v2 or higher in case of JOGRE). Just don't forget about the cardinal rule.

Corollary #4: Failure IS an option

Sometimes, as you get deeper into the code base, you might discover that the project does not really do what you need it to do. It might be designed in a way that you cannot easily adapt, there could be fundamental disagreements between you and the other team members -- or you simply happen to run out of time and need to focus on something else. It happens on occasion, so move on if you need to move on. However, if you choose to do so, communicate this clearly and in a nice manner. Do not promise to implement a particular feature and just never do it. Do not write a scathing goodbye post that explains why project xyz is so much better. Sometimes, the real problem actually lies somewhere between the keyboard and the seat.

The overall goal of my refactorings

Simply put, the goal of my initial work is to rework any code I might find in JOGRE that would prevent it from running on App Engine. Since I am just starting out in this effort, I do not have a very good idea yet what that might be. However, I am aware of a few things that work in a regular Java program but would be challenging in App Engine, such as:

• spawning threads
• network communication through anything but http
• storing data in files
• keeping data in a static field and expecting it to stick around (the next request might hit a completely different instance of my App Engine app)
• connecting via JDBC to a database
• longer living background processes
• anything that requires a "restart" of the system
• anything equivalent to a "global scan" (will the client ever need to get the list of all users?)
• anything with near-realtime requirements that relies on the accuracy of timestamps
• anything that uses native code or expects certain operating system commands to be available
• code that uses any class that may not be whitelisted
  

There's probably more stuff to look out for, but those are the first things that come to my mind. Some of these limitations will only be triggered (if ever) as I do the first tentative steps towards the port to App Engine, but others might be obvious from looking at the code. In the first couple of weeks, I will do my best to identify those more obvious areas and refactor them. I will do so in a way that is downwards compatible (it should be possible to merge those changes back into the main project, and older deployments of JOGRE should still work) and does not break the existing unit tests. If I break a unit test, I will revert my change and accomplish the goal in another way (minor changes to the test to cover API modifications are ok, though).

Today's refactoring in pictures

After having successfully built the server and played a game of Connect4 with myself, I started looking into the main method of the server code. From the documentation, I knew that the client-server protocol was xml based, which is good. I did not know however in which way those messages were exchanged. Many gaming engines use TCP directly (or, for better throughput, even UDP), since it provides a lot of nice properties (like establishing an keeping up a connection) that are more than sufficient for their needs.

I took a peek and found my expectations confirmed: JOGRE was using a well-established pattern that could have been straight from The Java Tutorial (see http://java.sun.com/docs/books/tutorial/networking/sockets/clientServer.html):

• a main class (JogreServer) creates a ServerSocket and listens in a loop to incoming connections
• for each incoming connection, it starts a new Thread (ServerConnectionThread) that then handles all the gaming logic.

Here is how the connection thread looks like:



ServerConnectionThread is a subclass of AbstractConnectionThread, which contains all the logic of how to fetch data from the connection, maintain the lifecycle of the socket, update connection state, and remember the name of the current user that owns the connection. AbstractConnectionThread, a subclass of java.lang.Thread, also had a couple of other subclasses that shared the connection handling logic but did other things with the arriving data. How could I start squeezing http support in here without breaking anything?

There is a very good chapter in Working Effectively with Legacy Code that introduces a concept that is called the Single-Responsibility Principle:

"Every class should have a single responsibility: It should have a single purpose in the system, and there should be only one reason to change it."

Using that as the foundation of my first refactoring, I decided to break parts of the AbstractConnectionThread out into a new class called SocketBasedMessageBus:



The refactoring itself was pretty straightforward: I took the content of the thread's run()-method, plus everything that used the socket object, and copied it into the new class. The thread's constructor would simply wrap the original Socket into the new object, and it's start/stop/run-methods would delegate calls to their equivalents in the new object.

I compiled the code and ran the unit tests -- everything still worked :-). The next step was to simply kick out those delegating, hollowed-out methods. I added a "getMessageBus" method to the base class and had all subclasses use that to get access to the new MessageBus object, and called the moved methods in the subclasses:



Now that the the SocketBasedMessageBus was doing all the socket-based work, AbstractConnectionThread no longer needed to subclass the Thread class. This enabled me to get rid of the run() method, and encapsulate the use of Threads in the message bus:



While looking at the new class, I realized that the names of the methods I had moved into SocketBasedMessageBus were focused around threading and loops -- but that was not necessarily what the class was responsible for. As the name said, SocketBasedMessageBus isolated the exchange of messages via a Java socket -- so its public API should reflect that. I decided to rename the methods accordingly:



While probably still not single responsibility, I now had a situation where the AbstractConnectionThread class no longer had any particular knowledge that it was using a socket -- except for its constructor. That was good enough for me (after all, if it ain't broke...), so I decided to wrap it up. One final cleanup step remained: the constructor of our base class should not have to know about sockets. Nor would our MessageBus really have to care about the user name of the connection thread, as long as there was a parse- and a cleanup method. I therefore chose to extract those aspects of the class into interfaces and have the base classes depend on those rather than the concrete implementations:



At the end of the day, the refactoring of AbstractConnectionThread resulted in the following modification:

• instead of managing sockets and threads, an AbstractConnectionThread is connected to a generic MessageBus object, which may choose any transport protocol (sockets, udp, http) it likes. The class no longer has any dependencies on sockets.
  
• AbstractConnectionThread has two remaining responsibilities: to manage the name of the user that the connection belongs to, and to provide a generalization for how to react to incoming messages from the MessageBus. The latter responsibility is represented by the MessageParser interface, which is what the MessageBus interfaces with.
• concrete subclasses like ServerConnectionThread still accept Sockets in their constructor (thus remaining compatible with the rest of the codebase), but they wrap the socket in a MessageBus before pushing it into the base constructor. I might choose to refactor that in a later stage, but if I do so, I can do it on an individual class basis, without breaking any of the other peer classes.
  

Hardly a spectacular change, but that was never the goal in the first place ;-). The main target was to make the socket communication replaceable without major rewrites of the system. Now that that's done, I can keep scanning the code for other things that might be tough to do on App Engine.

The refactoring in code

For those amongst us who'd rather just read code, here are the classes affected by this refactoring:

The original class
http://code.google.com/p/gae-ogre/source/browse/trunk/api/src/org/jogre/common/AbstractConnectionThread.java?r=2

Main refactoring targets

• AbstractConnectionThread (refactored)
• MessageBus (new)
• SocketBasedMessageBus (new)

Affected classes (minor changes)

• AdminClientConnectionThread
  
• AdminConnectionPanel
  
• AdminConnectionThread
• ClientConnectionThread
• GameConnectionPanel
• JogreLogger
  
• JogreServer
  
• ServerConnectionThread
  
• ServerGameController
  
• TestConnectionThread