1
\$\begingroup\$

Here is the story.

We have the BankTerminalSettings class.

It has many properties:

public class BankTerminalSettings {
    public bool IsEnabled { get; set; }
    public string IPAddress {get; set;}
    public ushort TcpPort {get; set; }
    public List<string> Zombies {get; set;}

    //and many other properties
}

And we have a class which takes BankTerminalSettings as a parameter of it's constructor.

public class BankTerminal {
    public BankTerminal(BankTerminalSettings terminal, int clientId) {
        ValidateBankTerminalSettings(terminal);

        terminalSettings = terminal;
        this.clientId = clientId;
    }
}

The thing is that the BankTerminal uses only three of all those properties in that domain config class named BankTerminalSettings. It's a temptation just to pass the whole instace of the BankTerminalSettings, but the caller can incidentally change those properties which are relevant for the BankTerminal. That can cause very subtle bugs.

What would you recommend? To replicate the BankTerminalSettings before passing it's instance? To replicate the BankTerminalSettings in the constructor of the BankTerminal? To pass only those parameters which are truely needed by the BankTerminal?

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Could you expand your code to list the many other properties? It will likely be vital in deciding how to tackle your issue. Additionally posting more of the code of BankTerminal (so we can see how BankTerminalSettings is used) would help too. \$\endgroup\$
    – Pimgd
    Feb 13, 2015 at 14:43
  • 3
    \$\begingroup\$ It's not that this isn't an interesting question. It is. I just don't feel like there's enough context to give a meaningful review. \$\endgroup\$
    – RubberDuck
    Feb 13, 2015 at 18:24

3 Answers 3

1
\$\begingroup\$

It depends on whether you want to handle config updates or not. In your case I guess you just want to work with data at constructing point. There are few options to do this:

  1. Make copy of your settings before they are passed and use BankTerminalSettings inside the BankTerminal (as was considered). But personally I dont like the idea there are different BankTerminalSettings passing around.
  2. Better approach would be just to inialize your internal fields with config settings, i.e. to copy settings of BankTerminalSettings to BankTerminal

Also I'm worried with the fact BankTerminalSettings contain more properties than is needed for BankTerminal. Maybe BankTerminalSettings should get another name? Anyway, I would create interface IBankTerminalSettings that contain only properties needed by BankTerminal and pass it to BankTerminal constructor

\$\endgroup\$
1
\$\begingroup\$

If you don't want users to modify a BankTerminalSettings instance you pass into them, then pass in a clone instead. If users are not supposed to modify a BankTerminalSettings instance you pass into them, then create an immutable interface without setters, and pass an ImmutableBankTerminalSettings instead. That way there will be no confusion, no inadvertent changes, enforced at compile time.

Keep in mind that even without a setter, the Zombies list will still be mutable. You will need to take extra precautions to prevent modifications, either by using an unmodifiable list (don't know if such exists in C#), or using a defensive copy of the list.

Lastly, if the BankTerminal doesn't need all the fields that are in BankTerminalSettings, then:

  1. Why is BankTerminalSettings called that way? (misleading)
  2. Consider passing to BankTerminal only the fields it actually needs, rather than leaking the entire object
\$\endgroup\$
0
\$\begingroup\$

One additional point: If you already have a *Settings class, then ValidateBankTerminalSettings shouldn't be in BankTerminal, but every single instance of the settings should already take care of its invariant if that isn't coupled to the BankTerminal object, namely being valid, or at least have a validate method to use.

\$\endgroup\$

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