Does sending parameters around inside a class suggest that I should refactor to a new class?

Question

I have noticed a pattern when I refactor my code for readability. When I refactor a method into several smaller methods, I often have to introduce parameters which sometimes (too often) makes the code less readable than before. I am wondering if this indicates that I should refactor to a different class, instead of just smaller methods.

I would think this is a very common situation, but I don't recall reading anywhere how to handle it.

---

Her is a simple example of this from a made-up scenario.

Before refactoring

public class ATM
{
    private double cashBalance = 5000;

    public double WithDraw(CreditCard card, string pin, double amount)
    {
        if(card.Pin != pin)
            throw new Exception("Wrong pin");
        if(balance < amount)
            throw new Exception("ATM out of money");
        if(card.Balance < amount)
            throw new Exception("Credit card out of money");
        cashBalance -= amount;
        return amount;
    }
}

Right off the bat, this isn't too bad, but I want to remove the logic from my conditionals and give it proper names.

First refactoring

public class ATM
{
    private double cashBalance = 5000;

    public double WithDraw(CreditCard card, string pin, double amount)
    {
        Validate(card, pin, amount);
        cashBalance -= amount;
        return amount;
    }

    private void Validate(CreditCard card, string pin, double amount)
    {
        if(!VerifyPin(card, pin))
            throw new Exception("Wrong pin");
        if(!HasEnoughCashBalance(amount))
            throw new Exception("ATM out of money");
        if(!CardHasEnoughMoney(card, amount))
            throw new Exception("Credit card out of money");
    }

    private bool VerifyPin(CreditCard card, string pin)
    {
        return card.Pin == pin;
    }

    private bool HasEnoughCashBalance(double amount)
    {
        return cashBalance >= amount;
    }

    private bool CardHasEnoughMoney(CreditCard card, double amount)
    {
        return card.Balance >= amount;
    }
}

I separate validation to its own method, it then further refactors each conditional to its own method given it a more descriptive name. (note I havn't spend the time to come up with actually good names) This however, has introduced a lot of parameters, and while the WithDraw method is more readable, the validation part isn't really.

This is the situation a notice again and again while refactoring methods into smaller methods.

Second refactoring

public class ATM
{
    private double cashBalance = 5000;
    private CreditCard card;
    private string pin;
    private double amount;

    public double WithDraw(CreditCard card, string pin, double amount)
    {
        this.card = card;
        this.pin = pin;
        this.amount = amount;
        Validate();
        cashBalance -= amount;
        return amount;
    }

    private void Validate()
    {
        if(!VerifyPin())
            throw new Exception("Wrong pin");
        if(!HasEnoughCashBalance())
            throw new Exception("ATM out of money");
        if(!CardHasEnoughMoney())
            throw new Exception("Credit card out of money");
    }

    private bool VerifyPin()
    {
        return card.Pin == pin;
    }

    private bool HasEnoughCashBalance()
    {
        return cashBalance >= amount;
    }

    private bool CardHasEnoughMoney()
    {
        return card.Balance >= amount;
    }
}

I sometimes end up with this fix, but I'm not a big fan of it. It feels borderline risky, or at the least doesn't feel cohesive.

Third refactoring

public class ATM
{
    private double cashBalance = 5000;

    public double WithDraw(CreditCard card, string pin, double amount)
    {
        var validator = new ATMValidator(cashBalance, card, pin, amount);
        validator.Validate();
        cashBalance -= amount;
        return amount;
    }
}

public class ATMValidator
{
    private double amount;
    private CreditCard card;
    private string pin;
    private double cashBalance;

    public ATMValidator(double cashBalance, CreditCard card, string pin, double amount)
    {
        this.cashBalance = cashBalance;
        this.card = card;
        this.pin = pin;
        this.amount = amount;
    }

    public void Validate()
    {
        if(!VerifyPin())
            throw new Exception("Wrong pin");
        if(!HasEnoughCashBalance())
            throw new Exception("ATM out of money");
        if(!CardHasEnoughMoney())
            throw new Exception("Credit card out of money");
    }

    private bool VerifyPin()
    {
        return card.Pin == pin;
    }

    private bool HasEnoughCashBalance()
    {
        return cashBalance >= amount;
    }

    private bool CardHasEnoughMoney()
    {
        return card.Balance >= amount;
    }
}

And this is where I end up, having a separate class. In this example it seems quite reasonable, and I would make an argument that withdrawal and validation are two separate responsibilities.

---

What I really want to know though, is if this pattern of sending parameters around inside a class is indicative of separate responsibilities. And am I handling it correctly. Is this perhaps just pure coincidence?

Thank you for taking the time to read all this.

Sidenote: Without refactoring I might've never noticed the separate responsibility.

Answer 1

Your second refactoring is definitely wrong. The addition of card, pin and amount variables to the ATM class are conceptually wrong. Those parameters don't belong to the ATM, they belong to a specific transaction.

The design should model the real-world problem. Keep the cashBalance in the ATM class, and try putting card, pin and amount variables in an ATMTransaction class. You'll end up with something that's a mix of your first and third refactorings, and will hopefully make more sense.

Note that you have to have two validation functions - one that validates the transaction itself (to check the card PIN and that it has enough money) and another to validate the transaction against the ATM (to check that the ATM has enough cash to cover the transaction amount).

In answer to your original question, the answer is "it depends". Sometimes you have to pass parameters around, sometimes it's indicative of a problem with the design. There's no simple rule that you can apply that will always be right, and if you try to go down this route, you're going to create more problems than you solve. However, if you find that you have a little group of parameters that you are repeatedly passing together to functions (eg card, pin, and amount in your example above) then that does indicate that there needs to be a new level of abstraction that encapsulates them (ie the ATMTransaction class that I propose).

Answer 2

I would argue that none of your examples really separate the responsibilities involved in completing a withdrawal. In all your refactorings you are shuffling the Validate-method around, which takes all the variables as arguments. Also, in your last example, both classes "know about" all the variables, which can be an indication (at least in this example) that this is a false separation.

To separate responsibilities, consider validation of the PIN the responsibility of the Card (as an entity that only needs to know the entered PIN and the card number), check that the customer has the funds to make the withdrawal the responsibility of an Account (that only needs to know the amount and which holds the balance), and check that there's enough cash to dispense the responsibility of the ATM (that only needs to know the amount and which holds the cash supply).

So to answer your question: yes, the passing around of (the same) parameters inside a class might suggest that the class could be split to separate responsibilities.