What is the best way to split up large methods where each subtask depends on previous tasks?

Question

According to this question and just about every style guide I've ever read, large methods are bad. However, suppose I have a method that does the following:

1. Receive a JSON String from some service and store it as a Dictionary
2. For various parameters in the Dictionary, creates connections to other services, such as using AWS keys to open connections to AWS services
3. Downloads, Authenticates, or Manipulates data based on values in the Dictionary and new data received from step 2.
4. Creates a file on the local directory and writes various values to it, dependent on step 2 and 3.
5. Pushes the file from step 4 to another location

Clearly, this should not all be one large method. However, I'm not sure how to split this up into smaller methods without passing around a ton of parameters. For example, the first method is simple because I only need to tell the program one thing, and only one thing is returned. However, starting at step 3, I will either have to pass the entire Dictionary to the new method, or make the Dictionary an instance or global variable.

So, in short, here is my question. When breaking up a large method like this, what is the better alternative: should I make variables which need to be used by multiple methods instance/global variables (whichever is appropriate for this particular task), or should I pass create new methods which take many parameters?

To me, it seems like the first option is much worse, because it seems to break encapsulation: each method created in this way now has some strange dependency on a global or instance variable which is outside of its scope. However, the second method feels really messy.

Answer 1

There's nothing wrong with passing a very large and complex parameter, as long as the structure and content of that parameter is well-defined and easy to understand. What you want to avoid is parameters that are easy to mix up, having parameters whose purpose is not clear, or depending too much on instance state rather than parameters. Even having lots of parameters is okay if you find an intuitive way of grouping them.

In fact, if I'm reading your question right, it sounds like you already know how to break up this task in a relatively simple way that only requires pure functions. Since Javascript is my main language, I'm imagining the code would look something like this:

var jsonStr = fetchJSONStringFromService();
var dict = JSON.parse(jsonStr);

var serviceConnections = createServiceConnections(dict);

var processedData = doVariousDataProcessingThings(dict);

var dataToWrite = serializeStuff(serviceConnections, processedData);
var fs = require('fs');
fs.writeFile('file/path', dataToWrite);

pushFileToOtherServer('file/path');

It's possible you left out some details that would make the real code significantly more complicated than this, but I'm assuming this is in the right ballpark. As you can see, it's already very easy to follow; the only thing missing is documentation of what structure the various objects being passed around are expected to have.

To answer your question directly, this sounds like a situation where you're better off aiming for stateless, pure, referentially transparent functions that only manipulate POJSOs, network connections and files. Even if some of the methods take more parameters than I've guessed above. That way each step will be straightforward to unit test (once you make all the required mocks), and very easy to change, move around or replace in the future without breaking the other steps. If any of the functions in your real code seem to take "too many" parameters, there's probably a way to break them into smaller functions or combine the parameters into meaningful objects until you end up with functions that are easy to understand.

Since your question implies this dict object is fairly large and complex, a possible improvement on top of the above would be to add a step where you break up the dict into smaller dictionaries or other data structures. In particular, I presume there's a subset of information in dict that needs to be used by createServiceConnections(), so it would be ideal to pass only that subset rather than the whole thing.

For completeness, I'm not saying that objects with instance variables are never useful. In fact, in the sample code above, it's likely that serviceConnections and fs are very stateful objects, which is fine because they represent inherently stateful things, unlike a blob of data.

Answer 2

You are concerned with how to structure a single function, therefore whatever solution you come up with, it should not affect the structure of the class.

What this means is that you should definitely not introduce instance variables; passing all necessary parameters to methods is the way to go.

As for global variables, you should not have even considered them, nor brought them up in this question. There is no application of global variables which does not result in evil, and there is no evil which does not get worsened by the introduction of global variables. And as a matter of fact, even instance variables are nothing but global variables within the scope of a class, (the class being thought of as the globe in this case,) so it is best to never have any instance variables that could have been avoided.

I do not see why you regard passing parameters as an onerous task. You speak of having to "pass the entire Dictionary to the new method"; well, it is not like you are going to be transferring it in truckloads; you just declare the method to accept the dictionary as a parameter, and then later when you invoke that method, you make sure to pass the dictionary as a parameter. It is not a big deal, really.

All that having been said, let me also add that the maxim which proclaims that functions should be as small as possible should be taken with a grain of salt. The ultimate goal is to reduce the complexity of the code, and all these lofty principles, guidelines, and words of wisdom have nothing but one end goal: to help reduce complexity. So, keep in mind that it is entirely possible that a single long method consisting of a few sequentially executed chunks sharing some local state may represent a smaller amount of complexity than a small method which sequentially invokes a multitude of other methods which serve no purpose other than to break the code into smaller chunks, simply because the work done is the same, the code which does it is the same, and we now have the additional burden of packaging and passing parameters around, un-packaging results, etc. And of course, if this mechanism requires the introduction of additional entities, like global variables, instance variables, or new classes to hold temporary results, then it is by definition a solution of greater complexity than the single long method approach.

Answer 3

My view is little different. The first step in the workflow is the key to move ahead with the remaining steps. I believe, if your first step results in any exception or an empty resultset then you would not go for the remaining steps. And attempt to have a reTry mechanism in between the steps.

You should create separate classes to perform various actions required in the entire workflow. This would keep the code clean & easier to troubleshoot also. This would promote loose coupling among the workflow steps and easier to make independent modifications in the future requirements.

• Receive a JSON String from some service and store it as a Dictionary.

  ---- Make a class which would initiate the connection to the external service with the required set of parameters. Lets say we name the class as PreRequisiteExternalInfoClass. Fetch the result of the call appropriately. If the call results in success, then only perform the next step. You can add a reTry mechanism to the connection process for extern service, if it results in failure. And log the data into the db for analysis and troubleshooting. If your application is multi-threaded based, then replace the simple Dictionary object with ConcurrentDictionary obj (.NET framework).

• For various parameters in the Dictionary, creates connections to other services, such as using AWS keys to open connections to AWS services.

  ----Create a new class which can take care of the connections to the AWS services. Probably think of making a singleton object for the connection object. Let's say we name the class as AWS_ServiceClass.

• Downloads, Authenticates, or Manipulates data based on values in the Dictionary and new data received from step 2.

  ---- Add new methods into the AWS_ServiceClass to perform downloads, authentication, manipulating datasets, etc...

• Creates a file on the local directory and writes various values to it, dependent on step 2 and 3.

  ---- Create a new static class say ManageFiles to have static methods like fileCreation, fileMove, fileCopy etc.. And pass appropriate parameter values to these methods.

• Pushes the file from step 4 to another location

  ---- If there are large number of files for the movement to another location, then probably take the help of Robocopy utility at the OS level.

Answer 4

Your class already does too much but I will not address that here.

Regarding the specific question:

should I make variables which need to be used by multiple methods instance/global variables (whichever is appropriate for this particular task), or should I pass create new methods which take many parameters?

Having too many parameters passed between methods is an indicator of low cohesiveness of the class. In the case of a static methods belonging to an utility class, that maybe OK but not otherwise.

If the method really belongs to that class then upon splitting it into separate methods, resulting methods should access the state of the class instead of passing lots of parameters between them. Keep the number of parameters to one or two at most.

The ideal number of arguments for a function is zero (niladic). Next comes one (monadic), followed closely by two (dyadic). Three arguments (triadic) should be avoided where possible. More than three (polyadic) requires very special justification—and then shouldn’t be used anyway.

Clean Code, A Handbook of Agile Software Craftsmanship -- Robert C. Martin