Mark Needham

Thoughts on Software Development

Archive for the ‘Reading Code’ tag

Reading Code: Assume it doesn’t work

with one comment

Jae and I have spent a reasonable chunk of the past few weeks pairing on code that neither of us are familiar with and at times we’ve found it quite difficult to work out exactly what it’s supposed to be doing.

My default stance in this situation is to assume that the code is probably correct and then try and work out how that’s the case.

After I’d vocalised this a few times, Jae pointed out that we couldn’t be sure that the code worked and it didn’t make sense to start with that as an assumption.

He pointed out that we were often going down paths of the code that were executed infrequently and since there were no tests around the code we couldn’t be sure what it did.

A more useful approach was to assume that it doesn’t work and then reason about it from scratch to determine whether or not that assumption is correct.

I quite like making this assumption because it makes you concentrate much more closely when you’re reading the code rather than skimming over it and assuming that it does what it’s supposed to do.

Reading the code has become a mini debugging session where we try to discover how some unfamiliar code actually works.

We’ve found a couple of bits of code that weren’t actually working by following this mindset but it is mentally quite tiring and obviously slower than other styles of reading code!

Written by Mark Needham

February 27th, 2013 at 11:12 pm

Posted in Reading Code

Tagged with

Reading Code: boilerpipe

without comments

I’m a big fan of the iPad application Flipboard, especially it’s ability to filter out the non important content on web pages and just show me the main content so I’ve been looking around at open source libraries which provide that facility.

I came across a quora page where someone had asked how this was done and the suggested libraries were readability, Goose and boilerpipe.

boilerpipe was written by Christian Kohlschütter and has a corresponding paper and video as well.

At a very high level this is my understanding of what the code is doing:

Boilerpipe highlevel

It is based around a pipes/filters architectural style whereby a TextDocument is passed through filters which perform transformations on it. After they’ve all been applied we can retrieve the main content of the article via a method call.

I’ve used the pipes/filters approach when playing around with clojure/F# but the problems I was working on were much smaller than this.

In the code there around about 7 or 8 fields being manipulated so I did sometimes find it difficult to know how fields could end up with certain values which often involved looking at other filters and seeing what they did to the document.

I always thought it should be possible to view each filter completely independently but when there’s state manipulation involved that doesn’t seem to be the case.

Luckily Christian has comments in his code which explain how you might compose the different filters again and why certain filters don’t make sense on their own, only if they’re combined with others.

For example the BlockProximityFusion class, which is used to merge together adjacent text blocks, contains the following comment:

Fuses adjacent blocks if their distance (in blocks) does not exceed a certain limit. This probably makes sense only in cases where an upstream filter already has removed some blocks.

I suppose the same thing could also have been achieved with some automated tests showing scenarios where different filters are composed.

Christian makes use of the logical OR (“|”) operator throughout the code base to ensure that all the filters get executed even if a previous one has successfully made changes to the document.

For example the main entry point into the code is ArticleExtractor which reads like this:

public final class ArticleExtractor extends ExtractorBase {
    public static final ArticleExtractor INSTANCE = new ArticleExtractor();
 
    public static ArticleExtractor getInstance() {
        return INSTANCE;
    }
 
    public boolean process(TextDocument doc) throws BoilerpipeProcessingException {
        return TerminatingBlocksFinder.INSTANCE.process(doc)
                | new DocumentTitleMatchClassifier(doc.getTitle()).process(doc)
                | NumWordsRulesClassifier.INSTANCE.process(doc)
                // cut for brevity
                | ExpandTitleToContentFilter.INSTANCE.process(doc);
    }
}

I noticed a similar thing in the underscore.js code but in that case the ‘&&’ operator was used to execute code on the right hand side only if the expression on the left had been successful.

If we’re not using any libraries that simulate first class collections in Java (totallylazy/Guava for example) then something like this could also work:

public final class ArticleExtractor extends ExtractorBase {
    ...
    public boolean process(TextDocument doc) throws BoilerpipeProcessingException {
        List<BoilerpipeFilter> filters = asList(TerminatingBlocksFinder.INSTANCE, new DocumentTitleMatchClassifier(doc.getTitle()), ExpandTitleToContentFilter.INSTANCE);
        boolean result = true;
 
        for (BoilerpipeFilter filter : filters) {
            result = result | filter.process(doc);
        }
 
        return result;
    }
}

I originally started just browsing the code and thought I roughly understood it before realising I couldn’t explain what it actually did. I therefore changed my approach and started writing some unit tests around it to see what the current behaviour was.

From what I can tell the main algorithm in the code is contained inside NumWordsRulesClassifier where each text block in the document is classified as being either content or non content.

I wrote tests covering all the scenarios in this class and then refactored the code to see if I could make it a bit more expressive. I ended up with this:

private boolean currentBlockHasContent(final TextBlock prev, final TextBlock curr, final TextBlock next) {
    if (fewLinksInCurrentBlock(curr)) {
        if (fewLinksInPreviousBlock(prev)) {
            return curr.getNumWords() > 16 || next.getNumWords() > 15 || prev.getNumWords() > 4;
        } else {
            return curr.getNumWords() > 40 || next.getNumWords() > 17;
        }
    }
    return false;
}
 
private boolean fewLinksInCurrentBlock(TextBlock curr) {
    return curr.getLinkDensity() <= 0.333333;
}
 
private boolean fewLinksInPreviousBlock(TextBlock prev) {
    return prev.getLinkDensity() <= 0.555556;
}

The logic is all based around examining the text blocks immediately before and after the current one to work out whether or not it’s likely to be boiler plate content.

The logic around the next/previous text blocks is written quite imperatively and feels like it could be made more concise by using something like F#’s ‘Seq.windowed’ over the collection but I can’t quite see how at the moment!

You can read more about the algorithm on pages 4-7 of the paper.

From running the code against a few articles I’ve got saved to ReadItLater it does seem to work reasonably well.

Overall…

I haven’t read every single bit of the code base but from what I have read I think boilerpipe is a pretty cool library and the approach to filtering content is neat.

I found it especially useful to be able to read parts of the paper and then go and look at the corresponding code. Often that type of thing remains up to the imagination of the reader from my experience!

Written by Mark Needham

February 13th, 2012 at 9:16 pm

Posted in Reading Code

Tagged with

Reading Code: Know what you’re looking for

with 2 comments

In the last week or so before Christmas I got the chance to spend some time pairing with my colleague Alex Harin while trying to understand how an existing application which we were investigating was written.

We knew from watching a demo of the application that the user was able to send some processing off to be done in the background and that they would be emailed once that had happened.

Our starting point was therefore to work backwards from the labels on the UI and finding which code got executed when the user submitted the task.

My initial approach was to find the entry point and then follow the method calls line by line, slowly building my knowledge of how the application actually worked.

Alex used a much quicker approach whereby he thought about how the code would be designed and then looked for the bit of code which proved his belief.

In this case we knew that the application had a 2 tier architecture and that it didn’t use any middleware which meant that it would more than likely be using the database as a queue to store the tasks to be processed.

Another colleague and I were then able to make use of this approach when looking at another piece of code.

It was being used to do pricing and we knew that there were different algorithms depending on which country you were in, which meant that we needed to look for anything country related to answer our questions.

Effectively we’re looking at the code with a hypothesis in hand and then trying to see whether or not what we see proves or disproves that hypotheses.

I need to practice a bit more but it seems to let you navigate code much more quickly and makes you much less likely to dive down a rabbit hole.

Written by Mark Needham

December 29th, 2011 at 2:43 am

Posted in Reading Code

Tagged with

Reading Code: underscore.js

with 9 comments

I’ve been spending a bit of time reading through the source code of underscore.js, a JavaScript library that provides lots of functional programming support which my colleague Dave Yeung pointed out to me after reading my post about building a small application with node.js.

I’m still getting used to the way that JavaScript libraries are written but these were some of the interesting things that I got from reading the code:

  • There are a couple of places in the code where the author has some code which runs conditionally and this is achieved by including that expression on the right hand side of an ‘&&’.

    For example on line 129 in the ‘filter’ function:

    123
    124
    125
    126
    127
    128
    129
    130
    131
    132
    
      // Return all the elements that pass a truth test.
      // Delegates to JavaScript 1.6's native filter if available.
      _.filter = function(obj, iterator, context) {
        if (nativeFilter && obj.filter === nativeFilter) return obj.filter(iterator, context);
        var results = [];
        each(obj, function(value, index, list) {
          iterator.call(context, value, index, list) && results.push(value);
        });
        return results;
      };

    I would probably have used an if statement to check the result from calling ‘iterator’ but this way is more concise and pretty neat.

    The same type of thing is done on line 150 in the ‘every’ function:

    145
    146
    147
    148
    149
    150
    151
    152
    153
    
      _.every = function(obj, iterator, context) {
        iterator = iterator || _.identity;
        if (nativeEvery && obj.every === nativeEvery) return obj.every(iterator, context);
        var result = true;
        each(obj, function(value, index, list) {
          if (!(result = result && iterator.call(context, value, index, list))) _.breakLoop();
        });
        return result;
      };

    The result is collected and the loop will also exit if the value of ‘result’ is ever false which is again a cool way to organise code.

  • It’s also quite cool that you can assign a value to a variable from within a conditional – this isn’t possible in any of the other languages that I’ve used previously as far as I’m aware.

    It’s even more evident in the ‘max’ function:

    191
    192
    193
    194
    195
    196
    197
    198
    199
    
      _.max = function(obj, iterator, context) {
        if (!iterator && _.isArray(obj)) return Math.max.apply(Math, obj);
        var result = {computed : -Infinity};
        each(obj, function(value, index, list) {
          var computed = iterator ? iterator.call(context, value, index, list) : value;
          computed >= result.computed && (result = {value : value, computed : computed});
        });
        return result.value;
      };

    ‘result’ is conditionally assigned on line 196 but only if the computed value is greater than the current computed value. Again an if statement is avoided.

    Another interesting thing about this function is that it specifically checks the type of the ‘obj’ passed in which reminded me about the discussion around Twitter having those sorts of checks in their Ruby code around a year ago. I guess that type of thing would be more prevalent in library code than in an application though.

  • I hadn’t come across the !! construct which is used to turn a JavaScript expression into its boolean equivalent:
    506
    507
    508
    
      _.isArray = nativeIsArray || function(obj) {
        return !!(obj && obj.concat && obj.unshift && !obj.callee);
      };

    Without using ‘!!” the expression would return ‘undefined’ in the case that one of those functions on ‘obj’ was not set. ‘!!’ forces the return value to be ‘false’.

  • Another technique used in this code base is that of dynamically adding methods to the prototype of an object:
    666
    667
    668
    669
    670
    671
    672
    673
    
      // Add all mutator Array functions to the wrapper.
      each(['pop', 'push', 'reverse', 'shift', 'sort', 'splice', 'unshift'], function(name) {
        var method = ArrayProto[name];
        wrapper.prototype[name] = function() {
          method.apply(this._wrapped, arguments);
          return result(this._wrapped, this._chain);
        };
      });

    This is quite a cool use of meta programming although it isn’t initially obvious how those functions end up on the object unless you know what to look for. It does significantly reduce the code needed to add these functions to the ‘wrapper’ object’s prototype, avoiding the ‘same whitespace, different values‘ problem that Neal Ford outlines in his article about harvesting idiomatic patterns in our code.

Written by Mark Needham

March 28th, 2010 at 8:02 pm

Posted in Reading Code

Tagged with

Reading Code: Unity

with 6 comments

I spent a bit of time reading some of the Unity code base recently and I decided to try out a variation of Michael Feathers ‘Effect Sketching’ which my colleague Dave Cameron showed me.

‘Effect Sketching’ is a technique Feathers describes in ‘Working Effectively With Legacy Code‘ and the idea is that we sketch a diagram showing the interactions between the fields and methods in a specific class while browsing through the code.

Dave suggested doing a similar thing but with methods and classes instead while stepping through the code with the debugger.

I set up this code to step my way through:

var container = new UnityContainer();
 
container.AddNewExtension<Interception>();
container.RegisterType(typeof (IIDProvider), typeof (HttpContextIDProvider));
container.Configure<Interception>().SetDefaultInterceptorFor(typeof (GetPaymentBreakdownsService), new VirtualMethodInterceptor());
 
object resolve = container.Resolve(typeof (GetPaymentBreakdownsService));

These are some of my observations from this exercise:

  • I found it was much easier for me to remember where I was in the call stack then normal.

    I often get quite engrossed in the individual method calls on objects that I forget where the code actually started before it ended up in the current location. Whenever this happened I was able to refer to my sketch to remind myself of where the code had started from.

  • Despite having the drawing I still got a bit lost when the PolicyInjectionBehaviourDescriptor made a call back to the container’s Resolve method which meant that the code went through the same path that I’d just followed:
    32
    33
    34
    35
    36
    37
    38
    
    public IInterceptionBehavior GetInterceptionBehavior(
                IInterceptor interceptor,
                Type interceptedType,
                Type implementationType,
                IUnityContainer container)
            {
                InjectionPolicy[] policies = container.Resolve<InjectionPolicy[]>();

    I was getting really confused watching various different injection policies being resolved instead of the type I was trying to resolve!

  • I’ve previously tried drawing diagrams which just contained the classes in a code base but I’ve found that including the methods that connect them makes it easier for me to understand what’s going on.

    I’ve been drawing these diagrams out by hand but I thought I’d translate some of it into dot notation so that I could create a version using graphviz to show on here.

    unity.png

    The nodes in orange represent classes and the dotted line represents where an event was fired.

  • I realised that I still need to spend more time reading code so that I know when to dive into an object and when the details are probably not important. At the moment I’m too prone to getting distracted by wanting to see how a specific method works instead of looking at those details later on when I actually need to know.
  • I felt as I was reading the code that in a lot of places the option type from functional programming would have come in quite useful. There is often code similar to this bit from LifeTimeStrategy:
    object existing = lifetimePolicy.GetValue();
    if (existing != null)
    {
        context.Existing = existing;
        context.BuildComplete = true;
    }

    Instead of existing returning a null it could return ‘None’ to indicate it hasn’t been resolved yet.

  • I’ve read about the ‘yield‘ construct before but I’ve never seen a need to use it yet in any code I’ve written so it was interesting to see it being used quite frequently inside PolicySet:
    public IEnumerable<InjectionPolicy> GetPoliciesFor(MethodImplementationInfo member)
    {
        foreach (InjectionPolicy policy in this)
        {
            if (policy.Matches(member))
            {
                yield return policy;
            }
        }
    }

    From my understanding of this construct it seems like it acts in a similar way to a stream i.e. it’s only evaluated when it’s actually needed.

  • In the ‘Fundamentals of Object Oriented Design in Uml‘ Meilir Page Jones suggests that we might want to avoid replicated behaviour in our public APIs since it leads to greater complexity.

    This would therefore seem to suggest that having overloads of methods on an object is something to be avoided. Interestingly in this code base the overloads for ‘UnityContainer’ are actually defined as extension methods which then delegate back to ‘UnityContainer’ and pass in default values for unspecified parameters.

    This seems like quite a neat way of getting around the problem since we keep the API clean while also providing users of the code an easy way to do so.

On the subject of reading code I recently came across an interesting post by designbygravity which describes some approaches for reading code more effectively.

In particular I really liked the section about not hating the code :

You can get sucked into hating the code, merely because it is not yours. Software people tend to be equipped with ample egos, and other people’s code can offend. But realize, their working code is better than your imagined code, because their working code exists right now. So put your ego aside and learn the code in front of you.

It’s so easy to drift into this mindset but it’s rarely helpful so if we can stop ourselves it’s almost certainly beneficial.

Written by Mark Needham

November 4th, 2009 at 1:22 am

Posted in Reading Code

Tagged with ,

Reading Code: Rhino Mocks

with 5 comments

I spent a bit of time recently reading through some of the Rhino Mocks to get a basic understanding of how some features work under the hood.

As well as just getting some practice at reading unfamiliar code I also wanted to know the following:

  • How does the ‘VerifyAllExpectations’ extension method work?
  • What’s the difference between the ‘GenerateMock’ and ‘GenerateStub’ methods on MockRepository?
  • How does the ‘AssertWasNotCalled’ extension method actually work?

These are some of the things I learnt from my exploration:

  • I’m using a Mac so I originally started out trying to read the code in TextMate with the C# plugin before eventually realising that I couldn’t really tell the difference between a delegate being passed around the code and a method call so I wanted an editor that would help me out with this.

    I decided to try out MonoDevelop to see if I could keep on reading the code outside my VM. Unfortunately I kept making that crash every time I tried to move between classes so back to Visual Studio it was! MonoDevelop looks like quite a nice tool but it just isn’t for me at the moment.

  • I’ve been playing around with an idea adapted from Michael Feathers’ Working Effectively With Legacy Code by drawing out the different classes and how they interact with each other. Where I could see a grouping between classes I’ve been drawing that into the diagram as well.

    Some of the guys on Twitter showed me a cool web based tool at yuml.me that allows you to easily draw class diagrams and then grab the png/jpeg file and do whatever you want with it.

    rhino-mocks.png

    Although these diagrams are quite simple I find them more useful than I had expected and I’ve started drawing more diagrams at work to help understand bits of code that I’m not very familiar with.

    I realised a couple of years ago when reading one of Scott Young’s posts about drawing diagrams that I seem to understand ideas more quickly if I’m able to draw them out so I should probably look to do it more frequently!

  • The way that stubs and mocks are generated is essentially the same which I’m told is also the case with Mockito although I haven’t read Mockito’s code yet.

    ‘GenerateMock’ eventually calls this bit of code:

    public T DynamicMock<T>(params object[] argumentsForConstructor)
        where T : class
    {
        if (ShouldUseRemotingProxy(typeof(T), argumentsForConstructor))
            return (T)RemotingMock(typeof(T), CreateDynamicRecordState);
        return (T)CreateMockObject(typeof(T), CreateDynamicRecordState, new Type[0], argumentsForConstructor);
    }

    ‘GenerateStub’ eventually calls this bit of code:

    public object Stub(Type type, params object[] argumentsForConstructor)
    {
        CreateMockState createStub = mockedObject => new StubRecordMockState(mockedObject, this);
        if (ShouldUseRemotingProxy(type, argumentsForConstructor))
            return RemotingMock(type, createStub);
        return CreateMockObject(type, createStub, new Type[0], argumentsForConstructor);
    }

    The main difference is that when the ‘Verify’ method is called on stubs (which would call the ‘StubReplayMockState’ class) we don’t do anything whereas with mocks a check is done to ensure that all the expectations setup in the test are met.

    I found it quite interesting that the new ‘Arrange-Act-Assert’ style syntax has been written to make use of the older ‘Record-Replay’ syntax which I guess is quite a nice metaphor for the two states that you use the framework.

    I haven’t looked at the Moq code yet but it would be interesting to see how the code for that differs as it was built from the ground up with the ‘Arrange-Act-Assert’ syntax.

  • The ‘AssertWasNotCalled’ method works fairly similarly to how I had imagined at a high level in that it goes and gets all the expectations for the mock for that method call and then checks that they aren’t called although I found the implementation of that first bit quite interesting.
    private static ExpectationVerificationInformation GetExpectationsToVerify<T>(T mock, Action<T> action, Action<IMethodOptions<object>> setupConstraints)
    {
    	// ...	
    	var mockToRecordExpectation = (T)mocks.DynamicMock(FindAppropriteType(mockedObject), mockedObject.ConstructorArguments);
    	action(mockToRecordExpectation);
    	// .. 
    }

    The code actually creates a mock in order to record the expectation that we are checking does not happen before it goes on to check which methods were called against that method. We therefore need to ensure that any calls to ‘AssertWasNotCalled’ are made after the call to the system under test otherwise it will always return true which may not be accurate.

  • I didn’t realise quite how much code there is in Rhino Mocks and I’ve only read a very small part of it. A few of the interesting parts of the code seem to be making calls to Castle DynamicProxy which is being used to do the intercepting of method calls.

    I’m never sure what the best way to approach a new code base is but this time I had a couple of starting points that I wanted to investigate and starting from those points seemed to work out quite well. I still find that I struggle to know when to stop digging down into how specific code works and when to just have a general understanding of that bit and move on to the next bit of code. I often find that I click all the way through to deeper methods and then I can’t remember why I did it in the first place.

Written by Mark Needham

July 28th, 2009 at 12:05 am

Posted in Reading Code

Tagged with