How to encapsulate private fields that only apply to a few methods

Question

I'm working on modeling a business domain object in a class and am wondering what would be the best way to properly encapsulate private fields that only apply to a few methods.

When I started, my code originally looked like this:

public class DiscountEngine
{
    public Cart As Cart { get; set;}
    public Discount As Discount { get; set;}

    public void ApplySKUGroupDiscountToCart()
    {
        ...
    }
}

However, ApplySKUGroupDiscountToCart() was starting to get ugly, so I decided to refactor the code into smaller private methods that get called from ApplySKUGroupDiscountToCart(). I started by passing in lots of local variables into the helper method, but then decided to pull out variables common to both routines and make them private modular variables. The new code looks like this:

public class DiscountEngine
{
    public Cart As Cart { get; set;}
    public Discount As Discount { get; set;}

    private int _SKUGroupItemDiscountsApplied = 0
    private int _SKUGroupTotalDiscounts = 0
    private int _SKUGroupID = 0

    public void ApplySKUGroupDiscountToCart()
    {
        ...
    }

    private void ApplyDiscountToSingleCartItem(ref CartItem cartI, 
                                               ref DiscountItem discountI)
    {
        ...
    }
}

On the one hand, the three private integer fields are useful for allowing the related methods to share common variables without needing to pass them back and forth as parameters. However, these variables are only applicable to these related methods and any other methods I might add would have no need to see them.

Is there a way to encapsulate the private fields and their related methods while still remaining a part of the DiscountEngine class? Is there a better way altogether of dealing with this problem?

Answer 1

Normally, making a class field private implies "I have enough discipline to ensure that this field is only used in an appropriate manner inside this class". If your class is too big for you to say that with confidence, then maybe the class is trying to do too many different things, and should be split up (see SRP).

Anyway, enough of the theory :-). If you want to stick with one class then you could always encapsulate those three fields into a private nested class, e.g.

public class DiscountEngine
{
    public Cart As Cart { get; set;}
    public Discount As Discount { get; set;}

    private class SKUGroup
    {
        public int ItemDiscountsApplied = 0
        public int TotalDiscounts = 0
        public int ID = 0
    }

    public void ApplySKUGroupDiscountToCart()
    {
        ...
    }

    private void ApplyDiscountToSingleCartItem(ref CartItem cartI, 
                                               ref DiscountItem discountI)
    {
        ...
    }
}

That gives you a bit more freedom to pass instances of the class around your code as method parameters.

You could take this a step further, and move any private methods that act on the SKU data into the nested class as well.

Answer 2

First things first, you very likely don't need to pass the parameters to ApplyDiscountToSingleCartItem as ref. Short version: unless you're actually assigning a value to the variable that you want to be visible to the calling code, you don't need ref. Modifying variable and property values on them will be visible to the calling code without passing them as ref.

Second, there is no way to scope a variable in between instance and local, which is what you're asking. The only way to accomplish this would be to refactor this functionality into another class (likely a nested private class).

Don't, however, use instance variables as a way to pass data between functions. If the data becomes "stale" after the function is called, then it should be a parameter, not an instance variable.