5

I have a process which must do 8 steps, in a particular order. One of the step involves sending an email, another one going on a distant FTP server, another one querying a database, and so on. Now to be able to unit test these 8 steps, each of them is a method in a distinct class. So far so good.

Now the problem I'm facing is that the 'parent' class that calls the steps now receives in its constructor 8 classes, which is clearly a code smell here. You could tell me: hey, just create one class with the first 4 steps, and another one with the 4 others, and the parent will simply receive two classes in it's constructor. In my mind, this also feels wrong - I have the impression that I'm adding complexity and not really solving the problem.

Is there a design pattern or another technique to use when many steps must be called by a class?

  • It does sound like there is a design problem here and it is unusual to need so many external dependencies in a single atomic process but without more detail it's really difficult to suggest what the solution might be. – Ant P Nov 12 '19 at 16:07
  • 1
    If those 8 classes are only used in 1 function each, move them to the function as a parameter instead of the class creation. For example instead of new MyClass(EmailService emailService, ...) do myClass.SendEmail(EmailService emailService). – Shelby115 Nov 12 '19 at 16:09
  • 1
    @Shelby115: That does mean you can't guarantee that the 8 steps are executed in batch though. One could call them in any arbitrary order, or only a subset of them. – Flater Nov 12 '19 at 16:36
  • Not posting an answer to supplant Eric King's already correct answer, but consider that the "constructor bloat" argument mostly applies to cases where developers manually call the constructor (because it's a pain to work with). If you use a DI framework, constructor bloat is less of a problem. It can be a red flag for code smells, but if you've vetted that the code does what it needs to (which you have), then it's not an actual issue. Not every red flag is an actual issue. – Flater Nov 12 '19 at 16:39
  • 2
    Here's some unsolicited advice. You said: One of the step involves sending an email, another one going on a distant FTP server, another one querying a database, and so on. Now to be able to unit test these 8 steps, each of them is a method in a distinct class. If that's what those classes do, then skip the Unit Tests and write just one good integration test that does everything in a test environment and bombs if any step fails. Unit tests do not add value to code that depends on external stuff (like FTP, emails, dbases, etc). – Graham Nov 12 '19 at 16:40
9

If the class is truly simplified to the point that it's just providing a controlled sequence to the operations, and the complexity of each operation is managed by the dependent classes, then I don't think there is a problem here to solve.

If the sequence of operations for this process is truly 8 steps, then the constructor with 8 dependencies adequately exposes the complexity of the process.

If anything should be considered for refactoring, it should be the process itself. Can it be simplified? Can it take fewer steps? If not, then it's as simple/complex as it needs to be and the coordinating class' constructor should reflect that.

Trying to hide the necessary complexity through arbitrary step-groupings would be a worse code smell than a large constructor.

  • 3
    Good reply. Sometimes, the "system gotta do what the system gotta do." – HardCode Nov 12 '19 at 16:19
  • Agree. Often, we attack complex processes out of habit, but if there's a screen with a button that is clicked once, which invokes 20 sub-actions, then ultimately there's one controlling class/function that understands how to kick off those 20 actions. There is nothing wrong with that, its our job to make sure the controlling class/function can easily explain how its kicking off those sub-functions in a succinct manner. – Graham Nov 12 '19 at 16:35
  • Yes, in this case, it is actually a simple coordinating class... – user1861857 Nov 12 '19 at 16:59
3

Further to Eric King's answer which is a good warning.

If you have to sequence over a series of operations, consider if they are similar enough that you could give them a common interface that could be invoked - such that you could treat them as a collection of abstract tasks.

Another thing to consider is if you might need to change or switch out any of the parts - when you want to change it - it might become more obvious how you would want to rearrange things.

Your solution is the same 'shape' as the problem which is often a good sign.

0

Is there any interaction between the 8 classes? If not, then one possibility would be for your 8 classes to all implement an interface with the single function doTheThing() (hilarious example function name, please don't use it in real code), then you could pass them within an IList instead of as eight separate parameters. Your "parent" class then just needs to iterate through this list, calling doTheThing() for each member.

  • 1
    Disagree. Adding a common interface function to unrelated classes with no commonality in their implementation of that interface is pointless and adds no value compared to just invoking each of the 8 classes/functions explicitly. – Graham Nov 12 '19 at 16:36
  • 1
    Disagree with your disagree. They're not unrelated - they represent the sub-steps that need to be performed as part of the overall process. And since when did classes implementing an interface need to have a commonality in how they implement the functions? The original question was asking how to deal with the problem of having lots of function parameters, and I gave a possible solution. – Pete Nov 12 '19 at 17:00
  • Not a commonality with how they implement the function (my first comment was probably not worded the best), but there should be commonality in that they perform a similar function in different ways. Take a look at the interfaces inside the .NET framework, things like "IDisposable". Implementations of that interface have a common method to dispose of whatever resources they've allocated. The overall 'action' is the same, but the details are different per implementation. – Graham Nov 12 '19 at 17:48
  • 4
    Absolutely no need to implement an interface when the language supports first class functions, such as delegates in C#. That IList can just be a list of Action's. No need to give them all the same name then. – David Arno Nov 13 '19 at 13:14
-3

The concept you are looking for here is 'dependency injection' and the related concept 'inversion of control'.

A good overview of the subject is here:

https://martinfowler.com/articles/injection.html

-4

The thing that you are doing wrong is doing "steps" in the first place. That indicates that you did a procedural decomposition and packed "steps that need to be done" in some arbitrary classes. You know, Services, Managers, things like that.

The problem with procedural decomposition is that it is ill suited for abstractions. You can't really do much except group procedures together and you sooner or later arrive at the point you are now.

The cure for this is object-orientation, although admittedly this is not as easy as it sounds. Objects are supposed to be autonomous things (not a group of procedures) that "encapsulate" some logic and/or knowledge. Because they do that they can be used for creating suitable abstractions, which ultimately (as a side-effect) solves your dependency problem.

So in a nutshell you have to stop thinking in terms of "steps", and have to start thinking in terms of "things".

  • 2
    Except for that even with the object oriented approach, somewhere some code has to be written that lists those 8 steps in order. Wrapping everything up such that the call-site is just one line and perfect looking OOP does nothing to reduce the actual complexity of the code. If a sequence of 8 tasks needs to fire, in order, in response to a button click, timer, or some other singular event, then 'procedural' code is unavoidable and perfectly appropriate. – Graham Nov 12 '19 at 21:04
  • @Graham It is difficult to explain how to think in an object-oriented way. I've tried for years now and I'm failing most of the time. Just saying "8 tasks need to fire" already demonstrates the wrong mindset. Sure, physically that needs to happen, but you need to see beyond that. We're not saying I need to fill the AX register anymore, right? We created abstraction over that. The same is true for OOP. It basically creates abstractions over procedural/imperative programming. – Robert Bräutigam Nov 13 '19 at 9:58
  • I don't think that analogy fits here. We filled the AX register in the past solely because we wanted to achieve some business outcome. Thus, it is fine to abstract that specific instruction behind a framework. In this case, the thing I want to code to do (send an email, FTP a file) is the actual business outcome, thus abstracting it behind anything else is no longer a guarantee of an improvement. Abstractions are great when they help us hide details that we don't really care about (the assembly registers and such), but that's not what we're dealing with here, in this example. – Graham Nov 13 '19 at 21:50
  • This post is too abstract and doesn't really answer the question. It can be improved by providing a specific solution to the problem. – Rumen Georgiev Nov 14 '19 at 8:58
  • @RumenGeorgiev You're right. Unfortunately there is not enough details in the question to propose a design. – Robert Bräutigam Nov 14 '19 at 11:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.