Is it a code smell for one method to depend on another?

Question

I am refactoring a class so that the code is testable (using NUnit and RhinoMocks as testing and isolations frameworks) and have found that I have found myself with a method is dependent on another (i.e. it depends on something which is created by that other method). Something like the following:

public class Impersonator
{
    private ImpersonationContext _context;

    public void Impersonate()
    {
        ...
        _context = GetContext();
        ...
    }

    public void UndoImpersonation()
    {
        if (_context != null)
            _someDepend.Undo();
    }
}

Which means that to test UndoImpersonation, I need to set it up by calling Impersonate (Impersonate already has several unit tests to verify its behaviour). This smells bad to me but in some sense it makes sense from the point of view of the code that calls into this class:

public void ExerciseClassToTest(Impersonator c)
{
     try
     {
         if (NeedImpersonation())
         {
             c.Impersonate();
         }
         ...
     }
     finally
     {
         c.UndoImpersonation();
     }
}

I wouldn't have worked this out if I didn't try to write a unit test for UndoImpersonation and found myself having to set up the test by calling the other public method. So, is this a bad smell and if so how can I work around it?

Answer 1

Well, there is a bit too little context to tell, it looks like _someDepend should be initalized in the constructor.

Initializing fields in an instance method is a big NO for me. A class should be fully usable (i.e. all methods work) as soon as it is constructed; so the constructor(s) should initialize all instance variables. See e.g. the page on single step construction in Ward Cunningham's wiki.

The reason initializing fields in an instance method is bad is mainly that it imposes an implicit ordering on how you can call methods. In your case, TheMethodIWantToTest will do different things depending on whether DoStuff was called first. This is generally not something a user of your class would expect, so it's bad :-(.

That said, sometimes this kind of coupling may be unavoidable (e.g. if one method acquires a resource such as a file handle, and another method is needed to release it). But even that should be handled within one method if possible.

What applies to your case is hard to tell without more context.

Answer 2

Provided you don't consider mutable objects a code smell by themselves, having to put an object into the state needed for a test is simply part of the set-up for that test.

Answer 3

Code smell has got to be the single most vague term I have ever encountered in the programming world. For a group of people that pride themselves on engineering principles, it ranks right up there in terms of unmeasurable rubbish and about as useless a measure as LOCs per day for programmer efficiency.

Anyway, that's my rant, thanks for listening :-)

To answer your specific question, I don't believe this is a problem. If you test something that has pre-conditions, you need to ensure the pre-conditions have been set up first.

One of the tests should be what happens when you call it without first setting up the pre-conditions - it should either fail gracefully or set up it's own pre-condition if the caller hasn't bothered to do so.

Answer 4

This is often unavoidable, for instance when working with remote connections - you have to call Open() before you can call Close(), and you don't want Open() to automatically happen in the constructor.

However you want to be very careful when doing this that the pattern is something readily understood - for instance I think most users accept this kind of behaviour for anything transactional, but might be surprised when they encounter DoStuff() and TheMethodIWantToTest() (whatever they're really called).

It's normally best practice to have a property that represents the current state - again look at remote or DB connections for an example of a consistently understood design.

The big no-no is for this to ever happen for properties. Properties should never care what order they are called in. If you have a simple value that does depend on the order of methods then it should be a parameterless method instead of a property-get.

Answer 5

To answer the title: having methods call each other (chaining) is unavoidable in object oriented programming, so in my view there is nothing wrong with testing a method that calls another. A unit test can be a class after all, it's a "unit" you're testing.

The level of chaining depends on the design of your object - you can either fork or cascade.

• Forking: classToTest1.SomeDependency.DoSomething()
• Cascading: classToTest1.DoSomething() (which internally would call SomeDependency.DoSomething)

But as others have mentioned, definitely keep your state initialisation in the constructor which from what I can tell, will probably solve your issue.

Answer 6

Yes, I think there is a code smell in this case. Not because of dependencies between methods, but because of the vague identity of the object. Rather than having an Impersonator which can be in different persona states, why not have an immutable Persona?

If you need a different Persona, just create a new one rather than changing the state of an existing object. If you need to do some cleanup afterwards, make Persona disposable. You can keep the Impersonator class as a factory:

using (var persona = impersonator.createPersona(...))
{
   // do something with the persona
}