Should one create shareable private class member or keep variable in method scope to pass it as a second method argument?

Question

Recently I had to refactor some legacy code. As in most cases, I had to split big parts of code into smaller, cleaner and readable functions. I ended with many functions, that had multiple, weird parameters. Let me show an example:

public void SendOrder(Order order, XmlOrder xmlOrder, Insider insider)
{
    string customerCode = CustomerServices
                 .Get(insider.CustomerId)
                 .Code;

    OutputData outputData = CreateOutputData(order, xmlOrder, customerCode);
    CreateReservations(order, customerCode, outputData);
    PlaceOrder(order, xmlOrder, outputData, customerCode);
}
...
private void CreateReservations(Order order, string customerCode, OutputData outputData)
{
    ...
    try
    {
        ReservationServices.AddReservation(reservation);
    }
    catch (BusinessException ex)
    {
        Logger.Log(ex);
        outpuData.Status = Statuses.BusnessError;
        throw;
    }
}

(That is just demonstration code, not real one)

The problem is I had to pass for example outputData to other functions just to change it's status if an exception happens or pass customerCode to multiple functions. Class is responsible of sending multiple, not connected messages to WebService, so when I was creating it, I didn't plan to put variables connected to given order as a class state. Is it a good practice to exclude such variables from function and make them class members? Are there any guidelines for situations like this? What are your practices and solutions for such problems?

Answer 1

Should one create shareable private class member or keep variable in method scope to pass it as a second method argument?

There is a third option, which is to formally address logically separate abstractions by introducing another class.

Class is responsible of sending multiple, not connected messages to WebService, so when I was creating it, I didn't plan to put variables connected to given order as a class state.

It looks to me like you are dealing with several abstractions here.

Sometimes we use multiple parameters instead of creating a formal abstraction. For example, we have an x coordinate and a y coordinate, and use those as parameters in several places. They are meant to be taken as a pair — they are thus an informal abstraction. To make the a formal abstraction we can create a Point class that binds the x and y together. Now where ever we had passed the two variables, we pass the single abstraction. A single abstraction is better than the variable pairs in that the client using these has fewer things to worry about, and the Point is more type safe and less error prone that the individual coordinate values (which might have been ints, for example). For example, one could accidentally swap x and y; one could use x with the wrong y when dealing with multiple points.

Generally speaking, it would be a mistake to include the fields of a logically different abstraction as members of some other abstraction. While it is hard to see from the code in your example, what the class in question is doing, it doesn't look like a good home just for passing parameters to these other functions (methods?).

When you're deciding whether to add fields to an existing class vs. create create another class, here is one way to think about it. If they are logically separate abstractions then the should have separate classes. If they are one and the same abstraction, then add the fields to the class. Check the lifetime of the individual fields. If they all have the same lifetime (and all the same lifetime as object (the class's instances)), that is consistent with being a single logical abstraction. However, if some of the fields are uninitialized after the others are properly initialized (e.g. in the constructor or an init method), and the other fields are only initialized and valid during certain operation sequences, then they belong to a logically separate abstraction, which indicates separate classes.

In your example, order and xmlorder possibly form a logically separate abstraction for you to receive as parameter in the first method.

Further, outputData could be upgraded (either by adding fields to it or by creating a wrapper — use the above lifetime analysis to determine) to capture (order and xmlorder) and outputData and customerCode.

Thus, your code could look like this:

public void SendOrder(OrderBundle orderBundle, Insider insider)
{
    string customerCode = CustomerServices
                 .Get(insider.CustomerId)
                 .Code;

    OutputDataBundle outputBundle = CreateOutputData(orderBundle, customerCode);
    outputBundle.CreateReservations();
    outputBunlde.PlaceOrder();
}

(Quality type & variable names notwithstanding.)

Answer 2

Class members should never be temporary storage data for some procedure, that would be noisy and confusing to anyone trying to understand the nature of the class. Of course if you randomly cut up a long function just to replace it with a number of functions with a limited number of lines, you will run into trouble with variable scope. A local variable cannot be in the scope of multiple functions.

So before cutting up a function "because it is too long", try to understand what it actually does logically and start from there. Each step may yield a result and does not have to know about stuff the other steps need to know about. Recognize and isolate those parts that are independent of the main logic. Those are the ones you want separate functions for.