Which One Do You Prefer?
Posted by Davy Brion on December 7th, 2009
I’m not going to provide any context for the following two pieces of code. All you need to know is that they are two versions of something that is essentially the same (not entirely though, but that’s not the point). I just wonder which version you guys think is more clear.
Version 1:
public class AsyncRequestProcessorSpecs : AcidTest
{
private static IRequestProcessor requestProcessor;
private static AsyncRequestProcessor asyncRequestProcessor;
public AsyncRequestProcessorSpecs() : base(10, 10) {}
public override void SetUp()
{
requestProcessor = MockRepository.GenerateMock<IRequestProcessor>();
asyncRequestProcessor = new AsyncRequestProcessor(requestProcessor);
}
class ProcessRequestsAsynchronouslyWithoutException : MetaTransition<Tuple<Request[], Response[]>, ProcessRequestsAsyncCompletedArgs>
{
public ProcessRequestsAsynchronouslyWithoutException()
{
GenerateInput = () => Tuple.New(new Request[0], new Response[0]);
Execute =
input =>
{
ProcessRequestsAsyncCompletedArgs processRequestsAsyncCompletedArgs = null;
requestProcessor.Stub(r => r.Process(input.First)).Return(input.Second).Repeat.Once();
asyncRequestProcessor.ProcessRequestsAsync(input.First, args => processRequestsAsyncCompletedArgs = args);
// this uglyness is only here because of the async stuff
int counter = 0;
while (processRequestsAsyncCompletedArgs == null)
{
if (++counter == 5)
{
throw new InvalidOperationException("time out… the callback should’ve been called already");
}
Thread.Sleep(10);
}
return processRequestsAsyncCompletedArgs;
};
}
}
[SpecFor(typeof(ProcessRequestsAsynchronouslyWithoutException))]
public Spec ProcessRequestsAsyncCompletedArgsContainsExpectedResponses(Tuple<Request[], Response[]> input, ProcessRequestsAsyncCompletedArgs output)
{
return new Spec(() => Ensure.Equal(input.Second, output.Result));
}
[SpecFor(typeof(ProcessRequestsAsynchronouslyWithoutException))]
public Spec ProcessRequestsAsyncCompletedArgsDoesNotContainException(Tuple<Request[], Response[]> input, ProcessRequestsAsyncCompletedArgs output)
{
return new Spec(() => Ensure.Null(output.Error));
}
class ProcessRequestsAsynchronouslyWithException : MetaTransition<Tuple<Request[], Exception>, ProcessRequestsAsyncCompletedArgs>
{
public ProcessRequestsAsynchronouslyWithException()
{
GenerateInput = () => { return Tuple.New(new Request[0], new Exception()); };
Execute =
input =>
{
ProcessRequestsAsyncCompletedArgs processRequestsAsyncCompletedArgs = null;
requestProcessor.Stub(r => r.Process(input.First)).Throw(input.Second).Repeat.Once();
asyncRequestProcessor.ProcessRequestsAsync(input.First, args => processRequestsAsyncCompletedArgs = args);
// this uglyness is only here because of the async stuff
int counter = 0;
while (processRequestsAsyncCompletedArgs == null)
{
if (++counter == 5)
{
throw new InvalidOperationException("time out… the callback should’ve been called already");
}
Thread.Sleep(10);
}
return processRequestsAsyncCompletedArgs;
};
}
}
[SpecFor(typeof(ProcessRequestsAsynchronouslyWithException))]
public Spec ProcessRequestsAsyncCompletedArgsContainsExpectedException(Tuple<Request[], Exception> input, ProcessRequestsAsyncCompletedArgs output)
{
return new Spec(() => Ensure.Equal(input.Second, output.Error));
}
[SpecFor(typeof(ProcessRequestsAsynchronouslyWithException))]
public Spec ProcessRequestsAsyncCompletedArgsThrowsProperExceptionWhenTryingToAccessResponses(Tuple<Request[], Exception> input, ProcessRequestsAsyncCompletedArgs output)
{
return new Spec(() =>
{
try
{
var blah = output.Result;
Ensure.Fail();
}
catch (TargetInvocationException)
{
// all is well in the world!
}
});
}
public class ProcessOneWayRequestsAsynchronouslyWithException : MetaTransition<Tuple<OneWayRequest[], Exception>, AsyncCompletedEventArgs>
{
public ProcessOneWayRequestsAsynchronouslyWithException()
{
GenerateInput = () => { return Tuple.New(new OneWayRequest[0], new Exception()); };
Execute =
input =>
{
AsyncCompletedEventArgs eventArgs = null;
requestProcessor.Stub(r => r.ProcessOneWayRequests(input.First)).Throw(input.Second).Repeat.Once();
asyncRequestProcessor.ProcessOneWayRequestsAsync(input.First, args => eventArgs = args);
// this uglyness is only here because of the async stuff
int counter = 0;
while (eventArgs == null)
{
if (++counter == 5)
{
throw new InvalidOperationException("time out… the callback should’ve been called already");
}
Thread.Sleep(10);
}
return eventArgs;
};
}
}
[SpecFor(typeof(ProcessOneWayRequestsAsynchronouslyWithException))]
public Spec ProcessOneWayRequestsAsyncCompletedEventArgsContainsExpectedException(Tuple<OneWayRequest[], Exception> input, AsyncCompletedEventArgs output)
{
return new Spec(() => Ensure.Equal(input.Second, output.Error));
}
}
Version 2:
using RequestProcessorFunction = System.Func<QuickNet.Types.Tuple<Agatha.Common.Request[], Agatha.Common.Response[]>, Rhino.Mocks.Interfaces.IMethodOptions<object>>;
namespace Tests
{
public class AsyncRequestProcessorSpecs : AcidTest
{
#region Setup
private static IRequestProcessor requestProcessor;
private static AsyncRequestProcessor asyncRequestProcessor;
public AsyncRequestProcessorSpecs() : base(10, 10) { }
public override void SetUp()
{
requestProcessor = MockRepository.GenerateMock<IRequestProcessor>();
asyncRequestProcessor = new AsyncRequestProcessor(requestProcessor);
}
#endregion
#region Context
private static ProcessRequestsAsyncCompletedArgs lastProcessRequestsAsyncCompletedArgs;
private static bool lastProcessRequestsThrewException;
private static ProcessRequestsAsynchronouslyInput lastProcessRequestInput;
public class ProcessRequestsAsynchronouslyInput
{
public RequestProcessorFunction Stub;
public readonly Tuple<Request[], Response[]> RequestResponsePair = Tuple.New(new Request[0], new Response[0]);
}
private class ProcessRequestsAsynchronouslyInputGenerator : BaseGenerator<ProcessRequestsAsynchronouslyInput>
{
public ProcessRequestsAsynchronouslyInputGenerator()
{
AddGeneratorForField(
t => t.Stub,
new ChoiceGenerator<RequestProcessorFunction>(
new RequestProcessorFunction[]
{
input => requestProcessor.Stub<IRequestProcessor>(
r => r.Process(input.First))
.Return(input.Second)
.Repeat.Once()
.WhenCalled(arg => lastProcessRequestsThrewException = false),
input => requestProcessor.Stub<IRequestProcessor>(
r => r.Process(input.First))
.Return(input.Second)
.Repeat.Once()
.WhenCalled(arg =>
{
lastProcessRequestsThrewException = true;
throw new Exception();
})
}));
}
}
private static bool LastRequestThrewAnException()
{
return lastProcessRequestsThrewException;
}
private static bool LastRequestDidNotThrowAnException()
{
return !lastProcessRequestsThrewException;
}
#endregion
#region Transitions
class ProcessRequestsAsynchronously : MetaTransition<ProcessRequestsAsynchronouslyInput, ProcessRequestsAsyncCompletedArgs>
{
public ProcessRequestsAsynchronously()
{
Generator = new ProcessRequestsAsynchronouslyInputGenerator();
Execute =
input =>
{
lastProcessRequestInput = input;
lastProcessRequestsThrewException = false;
lastProcessRequestsAsyncCompletedArgs = null;
input.Stub(input.RequestResponsePair);
asyncRequestProcessor.ProcessRequestsAsync(input.RequestResponsePair.First, args => lastProcessRequestsAsyncCompletedArgs = args);
// this uglyness is only here because of the async stuff
int counter = 0;
while (lastProcessRequestsAsyncCompletedArgs == null)
{
if (++counter == 5)
{
throw new InvalidOperationException("time out… the callback should’ve been called already");
}
Thread.Sleep(10);
}
return lastProcessRequestsAsyncCompletedArgs;
};
}
}
class GetResults : MetaTransition<ProcessRequestsAsyncCompletedArgs, Response[]>
{
public GetResults()
{
Precondition = () => lastProcessRequestsAsyncCompletedArgs != null;
GenerateInput = () => lastProcessRequestsAsyncCompletedArgs;
Execute = input => input.Result;
}
}
class GetError : MetaTransition<ProcessRequestsAsyncCompletedArgs, Exception>
{
public GetError()
{
Precondition = () => lastProcessRequestsAsyncCompletedArgs != null;
GenerateInput = () => lastProcessRequestsAsyncCompletedArgs;
Execute = input => input.Error;
}
}
#endregion
[SpecFor(typeof(ProcessRequestsAsynchronously))]
public Spec ProcessRequestsAsyncCompletedShouldNotTimeout(ProcessRequestsAsynchronouslyInput input, ProcessRequestsAsyncCompletedArgs output)
{
return new Spec(); //throws InvalidOperationException if it does timeout
}
[SpecFor(typeof(GetResults))]
public Spec GetResultsIfAllGoesWell(ProcessRequestsAsyncCompletedArgs input, Response[] output)
{
return new Spec(() => Ensure.Equal(lastProcessRequestInput.RequestResponsePair.Second, output))
.If(LastRequestDidNotThrowAnException);
}
[SpecFor(typeof(GetResults))]
public Spec GetResultsIfSomethingGoesWrong(ProcessRequestsAsyncCompletedArgs input, Response[] output)
{
return new Spec().Throws<TargetInvocationException>()
.If(LastRequestThrewAnException);
}
[SpecFor(typeof(GetError))]
public Spec GetErrorIfAllGoesWell(ProcessRequestsAsyncCompletedArgs input, Exception output)
{
return new Spec(() => Ensure.Null(output))
.If(LastRequestDidNotThrowAnException);
}
[SpecFor(typeof(GetError))]
public Spec GetErrorIfSomethingGoesWrong(ProcessRequestsAsyncCompletedArgs input, Exception output)
{
return new Spec(() => Ensure.NotNull(output))
.If(LastRequestThrewAnException);
}
}
}
December 7th, 2009 at 9:03 pm
I think both versions are butt ugly… although I can’t really give you a nice alternative.
If I really had to choose, I’d go for version 1 since the first line in version 2 makes me want to shut my system down and run away as fast as possible.
December 8th, 2009 at 1:55 am
I agree, both versions are ugly:
Bad formatting, but that could be a blog limitation.
Long names and declarations, who has the time to read
ProcessRequestsAsyncCompletedArgs processRequestsAsyncCompletedArgs = null;
when this will do:
ProcessRequestsAsyncCompletedArgs completedArgs = null
And all that Spec-stuff? Too much magic for my taste. I don’t understand any of those tests / specifications.
December 8th, 2009 at 7:04 am
well, it’s not really about which version looks better or less ugly… it’s about which version can you (sort of) understand without someone explaining everything to you?
December 8th, 2009 at 9:25 am
beautiful code is amongst other things easily understandable
ugly code, in most cases, isn’t
I haven’t invested a lot of time in figuring out both versions, but if I had to do so I have a feeling I’d go for version 1…
…although I also have a feeling that would be the wrong one in your opinion
December 8th, 2009 at 9:26 am
i used to think Objective C had an ugly syntax
until i learned to understand it
December 9th, 2009 at 9:32 am
allright… i’ll bite
on second thought maybe I do prefer the 2nd version
the specs are clearer
the in-line MetaTransition classes are still ugly though
maybe it would help if you renamed them to GetResultsTransition and GetErrorTransition
December 9th, 2009 at 12:36 pm
I had been thinking about using factorymethods instead of classes to model the transitions, like so:
public IMetaTransition GetError()
{
return new MetaTransition
{
Precondition = () => lastProcessRequestsAsyncCompletedArgs != null,
GenerateInput = () => lastProcessRequestsAsyncCompletedArgs,
Execute = input => input.Error
}
}
}
Which clears things up a little.
I couldn’t get this to work because there is no way to reference the method in the attribute parameter.
Except ofcourse if I used a magic string.