29

To simplify the interface, is it better to just not have the getBalance() method? Passing 0 to the charge(float c); will give the same result:

public class Client {
    private float bal;
    float getBalance() { return bal; }
    float charge(float c) {
        bal -= c;
        return bal;
    }
}

Maybe make a note in javadoc? Or, just leave it to the class user to figure out how to get the balance?

3
  • 26
    I'm generously assuming this is example, illustrative code and that you aren't using floating point math to store monetary values. Otherwise I'd have to get all preachy. :-)
    – corsiKa
    Feb 5, 2016 at 22:58
  • 1
    @corsiKa nah. That is just an example. But, yes, I would have used float to represent money in a real class... Thanks for reminding me about the perils of floating point numbers.
    – david.t
    Feb 6, 2016 at 1:26
  • 10
    Some languages distinguish between mutators and accessors to good effect. It would be awful annoying to be unable to get the balance of an instance that's only accessible as a constant!
    – JDługosz
    Feb 6, 2016 at 10:57

6 Answers 6

89

You seem to suggest that the complexity of an interface is measured by the number of elements it has (methods, in this case). Many would argue that having to remember that the charge method can be used to return the balance of a Client adds much more complexity than having the extra element of the getBalance method. Making things more explicit is much simpler, especially to the point where it leaves no ambiguity, regardless of the higher number of elements in the interface.

Besides, calling charge(0) violates the principle of least astonishment, also known as the WTFs per minute metric (from Clean Code, image below), making it hard for new members of the team (or current ones, after a while away from the code) until they understand that the call is actually used to get the balance. Think of how other readers would react:

enter image description here

Also, the signature of the charge method goes against the guidelines of doing one and only one thing and command-query separation, because it causes the object to change its state while also returning a new value.

All in all, I believe that the simplest interface in this case would be:

public class Client {
  private float bal;
  float getBalance() { return bal; }
  void charge(float c) { bal -= c; }
}
12
  • 25
    The point about command-query separation is extremely important, IMO. Imagine you're a new developer on this project. Looking through the code, it is immediately clear that getBalance() returns some value and does not modify anything. On the other hand, charge(0) looks like it probably modifies something... maybe it sends a PropertyChanged event? And what is it returning, the previous value or the new value? Now you have to look it up in the docs and waste brainpower, which could have been avoided using a clearer method.
    – Mage Xy
    Feb 5, 2016 at 19:46
  • 19
    Also, using charge(0) as the way of getting the balance because it happens to have no effect now weds you to this implementation detail; I can easily imagine a future requirement where tracking the number of charges becomes relevant, at which point having your code base littered with charges that aren't really charges becomes a pain point. You should provide interface elements that mean the things your clients need to do, not ones that merely happen to do the things your clients need to do.
    – Ben
    Feb 5, 2016 at 23:21
  • 12
    The CQS admonition is not necessarily appropriate. For a charge() method, if that method is atomic in a concurrent system, it may be appropriate to return the new or old value. Feb 6, 2016 at 0:47
  • 3
    I found your two citations for CQRS extremely unconvincing. For example, I generally like Meyer's ideas, but looking at the return type of a function to tell if it mutates something is arbitrary and unrobust. Many concurrent or immutable data structures require mutation + returns. How would you append to a String without violating CQRS? Do you have any better citations?
    – user949300
    Feb 6, 2016 at 7:27
  • 6
    Almost no code conforms to Command Query Separation. It's not idiomatic in any popular object-oriented languages whatsoever, and is violated by the built-in APIs of every language I've ever used. To describe it as a "best practice" thus seems bizarre; can you name even one software project, ever, that actually follows this pattern?
    – Mark Amery
    Feb 6, 2016 at 19:58
28

IMO, replacing getBalance() with charge(0) across your application isn't a simplification. Yes it is fewer lines, but it obfuscates the meaning of the charge() method, which could potentially cause headaches down the line when you or someone else needs to revisit this code.

Although they might give the same result, getting the balance of an account is not the same as a charge of zero, so it would probably be best to seperate your concerns. For example, if you ever needed to modify charge() to log whenever there is an account transaction, you now have a problem and would need to separate the functionality anyway.

13

It's important to remember that your code should be self-documenting. When I call charge(x), I expect x to be charged. Information about balance is secondary. What's more, I may not know how charge() is implemented when I call it and I definitely won't know how it's implemented tomorrow. For example, consider this potential future update to charge():

float charge(float c) {
    lockDownUserAccountUntilChargeClears();
    bal -= c;
    Unlock();
    return bal;
}

All of a sudden using charge() to get the balance doesn't look so good.

1
  • Yes! A good point that charge is far more heavy-weight. Feb 6, 2016 at 17:24
2

Using charge(0); to get the balance is a bad idea: one day, someone might add some code there to log the charges being made without realising the other use of the function, and then every time someone gets the balance it will be logged as a charge. (There are ways around this such as a conditional statement that says something like:

if (c > 0) {
    // make and log charge
}
return bal;

but these rely on the programmer knowing to implement them, which he won't do if it isn't immediately obvious that they are necessary.

In short: don't rely on either your users or your programmer successors realising that charge(0); is the correct way to get the balance, because unless there's documentation that they are guaranteed to not miss then frankly that looks like the most frightening way of getting the balance possible.

0

I know there are plenty of answers, but another reason against charge(0) is that a simple typo of charge(9) will cause your customer's balance to diminish every time you want to get their balance. If you have good unit testing you might be able to mitigate that risk, but if you fail to be diligent on every call to charge you could have this mishap.

-2

I'd like to mention a particular case where it would make sense to have fewer, more multipurpose, methods: if there is a lot of polymorphism, that is, many implementations of this interface; especially if those implementations are in separately developed code that can't be updated in sync (the interface is defined by a library).

In that case, simplifying the job of writing each implementation is much more valuable than the clarity of usage of it, because the former avoids contract violation bugs (the two methods being inconsistent with each other), whereas the latter only hurts readability, which can be recovered by a helper function or superclass method defining getBalance in terms of charge.

(This is a design pattern, which I do not recall a specific name for: defining a complex caller-friendly interface in terms of an minimal implementor-friendly one. In Classic Mac OS the minimal interface for drawing operations was called the "bottleneck" but this does not seem to be a popular term.)

If this is not the case (there are few implementations or exactly one) then separating the methods for clarity, and to allow the simple addition of behavior relevant to nonzero charges to charge(), makes sense.

5
  • 1
    Indeed, I've had cases where the more minimal interface was chosen to make complete coverage testing easier. But this isn't necessarily exposed to the user of the class. E.g. insert, append, and delete can all be simple wrappers of a general replace which has all control paths well-studied and tested.
    – JDługosz
    Feb 6, 2016 at 11:02
  • I've seen such an API. The Lua 5.1+ allocator uses it. You provide one function, and based on the parameters it passes, you decide whether it is trying to allocate new memory, deallocate memory, or reallocate memory from old memory. It is painful to write such functions. I would much rather that Lua had just given you two functions, one for allocation and one for deallocation. Feb 7, 2016 at 17:44
  • @NicolBolas: And none for reallocation? Anyway, that one has the added "benefit" of nearly being the same as realloc, sometimes, on some systems. Feb 7, 2016 at 18:58
  • @Deduplicator: allocation vs. reallocation is... OK. It's not great to put them in the same function, but it's not nearly as bad as putting deallocation in the same one. It's also an easy distinction to make. Feb 7, 2016 at 19:16
  • I would say that conflating deallocation with (re)allocation is an especially bad example of this type of interface. (Deallocation has a very different contract: it does not result in a valid pointer.)
    – Kevin Reid
    Feb 7, 2016 at 21:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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