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?

gnat
  • 21,442
  • 29
  • 112
  • 288
david.t
  • 437
  • 4
  • 7
  • 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 05 '16 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 06 '16 at 01: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 06 '16 at 10:57

6 Answers6

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; }
}
MichelHenrich
  • 6,225
  • 1
  • 27
  • 29
  • 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 05 '16 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 05 '16 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. – Dietrich Epp Feb 06 '16 at 00:47
  • Another example is the `Array#pop` method that both removes the last element of an array and returns the removed element. – John Dvorak Feb 06 '16 at 06:19
  • 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 06 '16 at 07:27
  • 1
    If the object is multi-threaded, it may be important to note the value as of *this* particular change. – JDługosz Feb 06 '16 at 10:58
  • 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 06 '16 at 19:58
  • Thanks everyone, i rephrased some of my original answer and added a few more arguments to it based on your comments. – MichelHenrich Feb 07 '16 at 13:12
  • 1
    Sorry, but returning whatever useful info you *have* to calculate *anyway* does not violate "Do one thing, and do it right". Arguably, the more expensive that info might be to get, the more returning it when you have it anyway is *the right thing*. Besides, nearly all APIs do it, so it's common and expected. – Deduplicator Feb 07 '16 at 13:17
  • I would go for boolean charge. Returning false if something blocked the charge from taking place. – Taemyr Feb 07 '16 at 19:09
  • 3
    @MarkAmery I feel it's very dangerous to suggest a lowest common denominator approach to code design. It's natural that more modern code design should be superior to that of older code because we should be learning from past mistakes. Built-in APIs are not a great example because they often contain many poor design decisions which can't be fully undone for reasons of backwards compatibility. These APIs, however, can and often do introduce new (and superior) design choices which become the recommended practice. – TheInnerLight Feb 07 '16 at 20:29
  • @TheInnerLight Sure, the patterns used by built-in APIs are not *necessarily* the pinnacle of good design - that's why the reference to them in my comment was one among many. That doesn't change the fact that effectively nobody ever has used CQS on a real project, and there's little reason they'd ever want to. – Mark Amery Feb 07 '16 at 21:13
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.

Matt Dalzell
  • 381
  • 2
  • 5
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.

Peter Mortensen
  • 1,050
  • 2
  • 12
  • 14
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.

micheal65536
  • 209
  • 1
  • 3
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.

Shaz
  • 2,614
  • 1
  • 12
  • 14
-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.

Kevin Reid
  • 156
  • 7
  • 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 06 '16 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. – Nicol Bolas Feb 07 '16 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. – Deduplicator Feb 07 '16 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. – Nicol Bolas Feb 07 '16 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 07 '16 at 21:52