Not Crying Over Old Code

posted by Craig Gidney on November 26, 2013

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.

Discuss on Reddit

My Twitter: @CraigGidney

  • Andreas Wild

    Nice catch; made me think :-)

  • Vance Feld

    Never used the same language for more than a year, let alone a decade. But once you’ve used enough languages, they are all pretty similar and naming shit good and creating an architecture with modularity in mind applies to any language. Well they’re similar until you do functional programming, then you just suck at it.

  • Nick Cosentino

    I thought this was great! It’s true what you hear: my old code is always bad. To your point, there should come a time when old code is not “bad”, but just an alternate solution to what you would end up with if you wrote it now.

    Thanks for sharing!


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

More interesting posts (1 of 5 articles)

Or check out our Portfolio.