There is a meme in the programming community which goes roughly as follows: I always hate my old code but that’s good because it means I’m improving.

I think that’s wrong.

Most Improved

If you join a team that plays a sport you’ve never played before, you can expect to get a “Most Improved” trophy at the end of the year. That’s a good sign… unless you keep getting it every year.

When you’re new to programming, you can legitimately expect to look back and hate the code you’re writing at the moment. It takes time to absorb the tacit knowledge that makes things work smoothly. This can continues for years, because programming well requires depth and breadth of experience.

(You also expect a learning curve when trying out a new language/library/framework/paradigm, but over a much shorter time scale.)

However, there comes a point where you should stop expecting to hate the code you’re writing now. If you’ve been programming in the same language for half a decade, but still consistently find yourself hating your old code, something is wrong. You’re either writing bad code without realizing it right now, or you’re too picky about what constitutes good code.

There are a myriad articles on how to not write bad code, so I’m just going to focus on how picky you should be.

Evaluating Code

When evaluating how good or bad some code is, it’s important to keep in mind how familiar you are with it. Otherwise you’ll end up equating “I already know it” with “good”. For example, it’s easy to think of your own code as more understandable than others’ code because you write what you understand.

The same effect comes into play when you look at your old code. Old code will always seem less understandable than you remember, because it’s not as fresh in your mind anymore. Comparing stale old code against fresh new code can create the illusion that you’re constantly improving. Alternatively, comparing against the unattainable feeling you get when looking at freshly written code might trick you into thinking all your old code is uniformly bad.

This is one of the reasons it’s so important to read lots of code written by other people. It calibrates your sense of what’s good and what’s not.

I suppose the rest of this post could discuss what makes code objectively good or bad… but I really don’t know a good way to summarize it. Effort required to get it working properly?… Average time for a new programmer to successfully update it?… Moles of dopamine released upon reading?…

Whatever. There’s plenty of other discussion about that. You already know your opinion about it. Let’s just go over some of my own old code.

Critiquing My Own

The oldest project I have on github is Tinker (a warcraft 3 hosting bot). Tinker was a hobby project, has about 35 thousand lines of code, and is written in VB.Net. Some would say those facts don’t bode well.

I have a surprisingly good memory for code. Especially when compared to my day to day memory. It’s been years, but I still remember how Tinker is laid out and which classes I had trouble splitting up. Let’s look at one of the classes I remember being okay with, though not particularly happy about: GameServer. It’s been at least a year since I touched it, and years since I made any major changes. Here’s the class’ current code:

Public NotInheritable Class GameServer Inherits DisposableWithTask Private Shared ReadOnly InitialConnectionTimeout As TimeSpan = 5.Seconds Private ReadOnly inQueue As CallQueue = MakeTaskedCallQueue Private ReadOnly outQueue As CallQueue = MakeTaskedCallQueue Private ReadOnly _clock As IClock Private ReadOnly _logger As Logger Private ReadOnly _gameSets As New Dictionary(Of UInt32, GameSet)() Private ReadOnly _viewGameSets As New ObservableCollection(Of GameSet)(outQueue:=outQueue) Private ReadOnly _viewActiveGameSets As New ObservableCollection(Of GameSet)(outQueue:=outQueue) Private ReadOnly _viewGames As New ObservableCollection(Of Tuple(Of GameSet, Game))(outQueue:=outQueue) Private ReadOnly _viewPlayers As New ObservableCollection(Of Tuple(Of GameSet, Game, Player))(outQueue:=outQueue) Public Event PlayerTalked(sender As GameServer, game As Game, player As Player, text As String) Public Event PlayerLeft(sender As GameServer, game As Game, gameState As GameState, player As Player, reportedResult As Protocol.PlayerLeaveReason, reasonDescription As String) Public Event PlayerSentData(sever As GameServer, game As Game, player As Player, data As Byte()) Public Event PlayerEntered(sender As GameServer, game As Game, player As Player) Private Sub ObjectInvariant() Contract.Invariant(_clock IsNot Nothing) Contract.Invariant(_viewGames IsNot Nothing) Contract.Invariant(_viewGameSets IsNot Nothing) Contract.Invariant(_viewActiveGameSets IsNot Nothing) Contract.Invariant(_viewPlayers IsNot Nothing) Contract.Invariant(_gameSets IsNot Nothing) Contract.Invariant(_logger IsNot Nothing) Contract.Invariant(inQueue IsNot Nothing) Contract.Invariant(outQueue IsNot Nothing) End Sub Public Sub New(clock As IClock, Optional logger As Logger = Nothing) Contract.Assume(clock IsNot Nothing) Me._logger = If(logger, New Logger) Me._clock = clock End Sub Public ReadOnly Property Logger As Logger Get Contract.Ensures(Contract.Result(Of Logger)() IsNot Nothing) Return _logger End Get End Property Public ReadOnly Property Clock As IClock Get Contract.Ensures(Contract.Result(Of IClock)() IsNot Nothing) Return _clock End Get End Property ''' Handles new connections to the server. ' Private Async Sub AcceptSocket(socket As W3Socket) Contract.Assume(socket IsNot Nothing) _logger.Log("Connection from {0}.".Frmt(socket.Name), LogMessageType.Positive) Dim socketHandled = New OnetimeLock() 'Setup initial timeout' Call Async Sub() Await _clock.Delay(InitialConnectionTimeout) If Not socketHandled.TryAcquire Then Return socket.Disconnect(expected:=True, reason:="Timeout") End Sub 'Try to read Knock packet' Try Dim data = Await socket.AsyncReadPacket() If Not socketHandled.TryAcquire Then Return HandleFirstPacket(socket, data) Catch ex As Exception socket.Disconnect(expected:=False, reason:=ex.Summarize) End Try End Sub Public Function QueueAcceptSocket(socket As W3Socket) As Task Contract.Requires(socket IsNot Nothing) Contract.Ensures(Contract.Result(Of Task)() IsNot Nothing) Return inQueue.QueueAction(Sub() AcceptSocket(socket)) End Function Private Sub HandleFirstPacket(socket As W3Socket, data As IRist(Of Byte)) Contract.Requires(socket IsNot Nothing) Contract.Requires(data IsNot Nothing) If data.Count < 4 OrElse data(0) <> Protocol.Packets.PacketPrefix OrElse data(1) <> Protocol.PacketId.Knock Then Throw New IO.InvalidDataException("{0} was not a warcraft 3 player connection.".Frmt(socket.Name)) End If 'Parse' Dim pickle = Protocol.ClientPackets.Knock.Jar.ParsePickle(data.SkipExact(4)) Dim knockData = pickle.Value 'Handle' Dim oldSocketName = socket.Name _logger.Log(Function() "{0} self-identified as {1} and wants to join game with id = {2}".Frmt(oldSocketName, knockData.Name, knockData.GameId), LogMessageType.Positive) socket.Name = knockData.Name _logger.Log(Function() "Received {0} from {1}".Frmt(Protocol.PacketId.Knock, oldSocketName), LogMessageType.DataEvent) _logger.Log(Function() "Received {0} from {1}: {2}".Frmt(Protocol.PacketId.Knock, oldSocketName, pickle.Description), LogMessageType.DataParsed) inQueue.QueueAction(Sub() OnPlayerIntroduction(knockData, socket)) End Sub ''' Handles connecting players that have sent their Knock packet. ' Private Async Sub OnPlayerIntroduction(knockData As Protocol.KnockData, socket As W3Socket) Contract.Assume(knockData IsNot Nothing) Contract.Assume(socket IsNot Nothing) 'Get players desired game set' If Not _gameSets.ContainsKey(knockData.GameId) Then _logger.Log("{0} specified an invalid game id ({1})".Frmt(knockData.Name, knockData.GameId), LogMessageType.Negative) socket.SendPacket(Protocol.MakeReject(Protocol.RejectReason.GameNotFound)) socket.Disconnect(expected:=False, reason:="Invalid game id") Return End If Dim entry = _gameSets(knockData.GameId) Contract.Assume(entry IsNot Nothing) 'Send player to game set' Try Dim game = Await entry.QueueTryAcceptPlayer(knockData, socket) _logger.Log("{0} entered {1}.".Frmt(knockData.Name, game.Name), LogMessageType.Positive) Catch ex As Exception _logger.Log("A game could not be found for {0}.".Frmt(knockData.Name), LogMessageType.Negative) socket.SendPacket(Protocol.MakeReject(Protocol.RejectReason.GameFull)) socket.Disconnect(expected:=True, reason:="A game could not be found for {0}.".Frmt(knockData.Name)) End Try End Sub Private Function AddGameSet(gameSettings As GameSettings) As GameSet Contract.Requires(gameSettings IsNot Nothing) Contract.Ensures(Contract.Result(Of GameSet)() IsNot Nothing) Dim id = gameSettings.GameDescription.GameId If _gameSets.ContainsKey(id) Then Throw New InvalidOperationException("There is already a server entry with that game id.") Dim gameSet = New GameSet(gameSettings, _clock) _gameSets(id) = gameSet _viewGameSets.Add(gameSet) _viewActiveGameSets.Add(gameSet) Dim activeAdder As WC3.GameSet.StateChangedEventHandler = Sub(sender, active) inQueue.QueueAction( Sub() If _viewActiveGameSets.Contains(sender) <> active Then If active Then _viewActiveGameSets.Add(sender) Else _viewActiveGameSets.Remove(sender) End If End If End Sub) AddHandler gameSet.StateChanged, activeAdder Dim gameLink = gameSet.ObserveGames( adder:=Sub(game) inQueue.QueueAction(Sub() _viewGames.Add(Tuple.Create(gameSet, game))), remover:=Sub(game) inQueue.QueueAction(Sub() _viewGames.Remove(Tuple.Create(gameSet, game)))) Dim playerLink = gameSet.ObservePlayers( adder:=Sub(game, player) inQueue.QueueAction(Sub() _viewPlayers.Add(Tuple.Create(gameSet, game, player))), remover:=Sub(game, player) inQueue.QueueAction(Sub() _viewPlayers.Remove(Tuple.Create(gameSet, game, player)))) 'Automatic removal' Call Async Sub() Await gameSet.DisposalTask _gameSets.Remove(id) _viewGameSets.Remove(gameSet) _viewActiveGameSets.Remove(gameSet) Call Async Sub() Await gameLink.DisposeAsync() Call Async Sub() Await playerLink.DisposeAsync() RemoveHandler gameSet.StateChanged, activeAdder End Sub Return gameSet End Function Public Function QueueAddGameSet(gameSettings As GameSettings) As Task(Of GameSet) Contract.Requires(gameSettings IsNot Nothing) Contract.Ensures(Contract.Result(Of Task(Of GameSet))() IsNot Nothing) Return inQueue.QueueFunc(Function() AddGameSet(gameSettings)) End Function Protected Overrides Function PerformDispose(finalizing As Boolean) As Task If finalizing Then Return Nothing Return inQueue.QueueAction( Sub() For Each entry In _gameSets.Values entry.Dispose() Next entry End Sub) End Function Private Async Function AsyncFindPlayer(username As String) As Task(Of Player) Contract.Assume(username IsNot Nothing) 'Contract.Ensures(Contract.Result(Of Task(Of Player))() IsNot Nothing)' Dim findResults = Await Task.WhenAll(From entry In _gameSets.Values Select entry.QueueTryFindPlayer(username)) Return (From player In findResults Where player IsNot Nothing).FirstOrDefault End Function Public Function QueueFindPlayer(userName As String) As Task(Of Player) Contract.Ensures(Contract.Result(Of Task(Of Player))() IsNot Nothing) Return inQueue.QueueFunc(Function() AsyncFindPlayer(userName)).Unwrap.AssumeNotNull End Function Private Async Function AsyncFindPlayerGame(username As String) As Task(Of Game) Contract.Assume(username IsNot Nothing) 'Contract.Ensures(Contract.Result(Of Task(Of Game))() IsNot Nothing)' Dim findResults = Await Task.WhenAll(From entry In _gameSets.Values Select entry.QueueTryFindPlayerGame(username)) Return (From game In findResults Where game IsNot Nothing).FirstOrDefault End Function Public Function QueueFindPlayerGame(userName As String) As Task(Of Game) Contract.Ensures(Contract.Result(Of Task(Of Game))() IsNot Nothing) Return inQueue.QueueFunc(Function() AsyncFindPlayerGame(userName)).Unwrap.AssumeNotNull End Function Public Function ObserveGameSets(adder As Action(Of GameSet), remover As Action(Of GameSet)) As Task(Of IDisposable) Contract.Requires(adder IsNot Nothing) Contract.Requires(remover IsNot Nothing) Contract.Ensures(Contract.Result(Of Task(Of IDisposable))() IsNot Nothing) Return inQueue.QueueFunc(Function() _viewGameSets.Observe(adder, remover)) End Function Public Function ObserveActiveGameSets(adder As Action(Of GameSet), remover As Action(Of GameSet)) As Task(Of IDisposable) Contract.Requires(adder IsNot Nothing) Contract.Requires(remover IsNot Nothing) Contract.Ensures(Contract.Result(Of Task(Of IDisposable))() IsNot Nothing) Return inQueue.QueueFunc(Function() _viewActiveGameSets.Observe(adder, remover)) End Function Public Function ObserveGames(adder As Action(Of GameSet, Game), remover As Action(Of GameSet, Game)) As Task(Of IDisposable) Contract.Requires(adder IsNot Nothing) Contract.Requires(remover IsNot Nothing) Contract.Ensures(Contract.Result(Of Task(Of IDisposable))() IsNot Nothing) Return inQueue.QueueFunc(Function() _viewGames.Observe( adder:=Sub(item) adder(item.Item1, item.Item2), remover:=Sub(item) remover(item.Item1, item.Item2))) End Function Public Function ObservePlayers(adder As Action(Of GameSet, Game, Player), remover As Action(Of GameSet, Game, Player)) As Task(Of IDisposable) Contract.Requires(adder IsNot Nothing) Contract.Requires(remover IsNot Nothing) Contract.Ensures(Contract.Result(Of Task(Of IDisposable))() IsNot Nothing) Return inQueue.QueueFunc(Function() _viewPlayers.Observe( adder:=Sub(item) adder(item.Item1, item.Item2, item.Item3), remover:=Sub(item) remover(item.Item1, item.Item2, item.Item3))) End Function End Class

Right off the bat I can see that this class’ purpose, how it fits into the overall code base, is not explained. There should be a doc comment stating that GameServer is, roughly speaking, the model part of an MVC pattern. That it tracks what games the bot is running, places joining players into those games, and is controlled by a game server manager. That the manager class exposes GameServer as a bot ‘component’ (so it can be created/disposed/commanded by the user). That unlike most of the other components, server components are not created manually (although they could be). That when the bot is asked to host a game, it automatically gets-or-creates a server component and then uses that from then on. And so on.

The next thing that should be fixed is the commented out Contract.Ensures lines. I remember that they’re commented out because they were crashing the static verifier, but that should be explained. Actually, because I remember having so many issues with crashing/confusing the static verifier that I don’t think they’re worth it anymore, I’d probably just cut all of the contracts stuff.

Looking at the code with my current knowledge, I can see I didn’t know about Rx and hadn’t figured out perishable collections when I wrote it. Refactoring the ‘ObservableCollection’ fields and associated accessor methods to expose IObservable<Perishable<T>> instances would simplify things a lot. For example, it would trivialize the tricky interleaved-into-everything-else code making it possible to observe all of the players in all of the games in all of the game sets.

I also notice that GameServer is designed as an actor, with all incoming requests going onto a call queue to be run in order. This can now be done more succinctly by awaiting into the call queues, as I explained in the post Emulating Actors in C#.

The method AsyncFindPlayerGame forwards the search to all the game sets, and uses WhenAll to await them all completing. Using OrderByCompletion could make the code more robust, because then the search could complete before all game sets had responded.

The order of the QueueAcceptSocket , AcceptSocket , and HandleFirstPacket methods is not reflective of the order they actually run.

Finally, if I were writing this class now, I would experiment with using cancellation tokens to control its lifetime instead of making it disposable. I’ve posted about why recently: cancellation tokens tend to remove boilerplate.

Verdict

The above probably sounds like I’m hammering on how terrible this code is, but I’m not. I actually think it’s fine, except for the lack of documentation and the mysteriously commented out lines. Everything else I mentioned is an alternative to explore, not a blocking issue. (Interestingly, I subconsciously switched from “should” for the blocking issues to “can/could/would” for the non-blocking issues.)

Most of the code in Tinker follows the same basic pattern: it averages ‘good’, except it lacks documentation. That’s my personal opinion of it, anyways. By far the most common comment I get from other people is just “VB? Bleh!”, with the occasional accompanying “Huh. That’s some nice VB.”.

Other classes I remember having difficulty simplifying are BnetClient and W3Game. W3Game has the line I’m most embarrassed about. The classes I still like the most are the protocol and format definitions.

Summary

Don’t expect to improve indefinitely without ever slowing down.

Adjust for familiarity when comparing how you feel about your old code to how you feel about your new code.

Distinguish between bad and different.

—

—

—



Twisted Oak Studios offers consulting and development on high-tech interactive projects. Check out our portfolio, or Give us a shout if you have anything you think some really rad engineers should help you with.



Archive