How to design not to duplicate procedural workflows between test and implementation?

[Grzegorz Gałęzowski]

Hi!

In one of the talks about mocking roles not object state in 2008, Nat and Steve mentioned, that it is a smell when a test code mimics implementation code.

What he means it that when we have a class that has a methods like:

public void doAction()
{
  InputRecord inputRecord  = readHelper.read();
  validationHelper.Validate(inputRecord);
  OutputRecord outputRecord = convertHelper.convert(inputRecord);
  repository.save(outputRecord);
}

Steve mentioned something like (quoting from memory) "when there is no decision or no iteration logic, the code is procedural, not object oriented and the tests show that".

As I am myself guilty of writing many such "flow" tests, I'd like to learn better on how to avoid them and how to change them to something else.

A (significantly changed, so the logic may be flawed) example from one of my previous projects is about setting up a data replication between two machines. The logic looks like this:

public class ReplicationOfXmlConfig : Replication
{
  //...

  public void setUp(ReplicationParameters parameters)
  {
    redundantMachine.send(parameters);
    localMachine.save(parameters);
    redundantMachine.sendLocalXmlConfiguration();
    redundantMachine.applyXmlConfigurationAtRuntime();
    localMachine.applyXmlConfigurationAtRuntime();
  }
}

My questions are:
1. Is every method that implements a flow with cyclomatic complexity of 1 is a smell?
2. If a "smell" is something that is probably a problem, do you know examples where such situations are OK?
3. How can we refactor this? I know each set of calls can be refactored into an iteration over a "Step" object collection - is that the right way to go? If so, is it always necessary? How would OO version of the above procedure look like?

Anyway, as you can see, I have some issues getting my head around it and I'd appreciate your help.

best regards,
Grzegorz Gałęzowski

[Mateusz Łoskot]

On 3 February 2014 12:11, Grzegorz Gałęzowski

<grzesiek....@gmail.com> wrote:
> My questions are:
> 1. Is every method that implements a flow with cyclomatic complexity of 1 is
> a smell?

I'm curious, what is the definition of "a flow" here?

Do you mean a naturally 1-cyclomatic-complex routine with single
entry/exit point, no loops, no conditional statements, no try-catch
blocks?

Best regards,
--
Mateusz Łoskot, http://mateusz.loskot.net

[Grzegorz Gałęzowski]

On Monday, February 3, 2014 3:23:16 PM UTC+1, Mateusz Łoskot wrote:

I'm curious, what is the definition of "a flow" here?

Do you mean a naturally 1-cyclomatic-complex routine with single
entry/exit point, no loops, no conditional statements, no try-catch
blocks?

Yes, precisely. I am not too attached the description of this case by "cyclomatic complexity", rather I mean what Steve was mentioning in the talk around 36:30.

In the presentation, Steve says there that one possibility is refactoring into a handler that is passed through a set of converters, but I am having a hard time figuring out the more general rule on how to approach such situation based on one simple example.

Best regards,
Grzegorz Gałęzowski

[Mateusz Łoskot]

On 3 February 2014 15:16, Grzegorz Gałęzowski

<grzesiek....@gmail.com> wrote:
> On Monday, February 3, 2014 3:23:16 PM UTC+1, Mateusz Łoskot wrote:
>>
>> I'm curious, what is the definition of "a flow" here?
>>
>> Do you mean a naturally 1-cyclomatic-complex routine with single
>> entry/exit point, no loops, no conditional statements, no try-catch
>> blocks?
>>
>
> Yes, precisely.

Good I'm on the right track myself.

> I am not too attached the description of this case by
> "cyclomatic complexity", rather I mean what Steve was mentioning in the talk
> around 36:30.

I'll check it when I have a moment.

> In the presentation, Steve says there that one possibility is refactoring
> into a handler that is passed through a set of converters, but I am having a
> hard time figuring out the more general rule on how to approach such
> situation based on one simple example.

I'll wait for (and learn from) what others have to say about this.

[Steve Freeman]

It's been a while, but...

1) not every cylomatic complexity of 1 is a smell. I was just trying to point out that it might be worth taking another look

2) I don't have hard rules (apart from not singletons). If you look at the pattern format, you'll see that it includes conflicting forces.

3) This could, in theory, be refactored into a list of action objects, although I'm not sure that would be better. 

I'm curious about this code, is it a cut down example because it seems to be missing all sorts of error handling and recovery? For an example, an alternative might (or might not) look like

try:

  otherMachine.updateWith(parameters)

  localMachine.updateWith(parameters)

catch RemoteFailure:

  localMachine.rollback(parameters)

Would something like that have been appropriate?

S

[Steve Freeman]

sorry, that last one was in response to a request for comment on

https://groups.google.com/forum/#!topic/growing-object-oriented-software/tijxbyDsxLM

S

[Grzegorz Gałęzowski]

Steve, thanks for the answer! I thought about it since the last time and got to a conclusion that this might not necessarily be a bad thing to have this kind of design, but is certainly something to look at critically.

As for the error recovery you mention, in this particular case (as I remember correctly from the code), the error handling was done by the entity that triggered the procedure (don't remember the exact reasons, sorry...). Also, some of the steps had rollback mechanism implemented, but the rollback was inherent to a single mechanism rather than it being a "step" or "action" (e.g. saving configuration file was triggered from some other places than just the replications setup procedure and in all of those places, we needed to guarantee the rollback, so the rollback was triggered internally and additionally, exception was reported, e.g. to display it on GUI), while some of the steps did not require rollback, because they did not leave machine in the "corrupted" state.

Having said that, there were other places in the application, where I used a pattern similar to the one you mention - I had a series of "steps"/"actions" that I went through in a loop and each had rollback()/undo() method. 

The reason I asked this question is because when I moved from "doing structural design with objects" to "doing something more OO style" then many (not all of course) of my conditionals went from the logic itself to either the structure, i.e. the the way pieces are assembled together at the application entry point or to factories. Further down the way, trying to follow "tell don't ask" heuristic led to decisions being made locally where the information is available instead of as a part of higher level workflows. This left me with a considerable amount of "cyclomatic complexity 1" workflows and there wasn't a need for every such workflow to have its own error handling. I noticed that I tend to tolerate such "series of calls" as long as the number of dependencies is relatively small and I seem to break it into "actions"/"steps" collections as soon as I observe I am pushing too much objects through the constructor.

Also, I am rather talking about workflows such as:

a.x();

b.y();

c.z();

where a, b and c are not closely related to each other (i.e. are different subsystems), than something like this:

v1 = a.x();

v2 = b.y(v1);

v3 = c.z(v2);

return v3;

I know it's very hard to reason without looking into the actual code and its context, but does what I described sound as something familiarly good/evil or is too much dependent on the context to draw any general conclusions?

I'd be happy to know your thoughts!

grzesiek

[Steve Freeman]

On 20 Apr 2014, at 23:19, Grzegorz Gałęzowski wrote:
> Steve, thanks for the answer! I thought about it since the last time and got to a conclusion that this might not necessarily be a bad thing to have this kind of design, but is certainly something to look at critically.

exactly

> [...]

> Having said that, there were other places in the application, where I used a pattern similar to the one you mention - I had a series of "steps"/"actions" that I went through in a loop and each had rollback()/undo() method.
>
> The reason I asked this question is because when I moved from "doing structural design with objects" to "doing something more OO style" then many (not all of course) of my conditionals went from the logic itself to either the structure, i.e. the the way pieces are assembled together at the application entry point or to factories. Further down the way, trying to follow "tell don't ask" heuristic led to decisions being made locally where the information is available instead of as a part of higher level workflows. This left me with a considerable amount of "cyclomatic complexity 1" workflows and there wasn't a need for every such workflow to have its own error handling. I noticed that I tend to tolerate such "series of calls" as long as the number of dependencies is relatively small and I seem to break it into "actions"/"steps" collections as soon as I observe I am pushing too much objects through the constructor.
>

> [...]

> I know it's very hard to reason without looking into the actual code and its context, but does what I described sound as something familiarly good/evil or is too much dependent on the context to draw any general conclusions?

There are always trade-offs. Chaining step objects with similar structure means that you can build behaviour from very simple and obvious parts, but it also means that you need to understand how they fit together. Some people like this more than others. For me, the trick is to become very sensitive to duplication and see where that takes me. Not very scientific, I know...

S