Another Design Principle
There are a couple of demos that I commonly use when I teach TDD. One is a Stack. It may seem like a rinky-dink sort of a problem because, well, it's a stack and it's hard to do any real programming without understanding what a stack does and how to write one. But actually, that is the thing that is so nice about it. Because everyone understands stack, it's easier to concentrate on the TDD process itself.For all their simplicity, typical stack implementations present a large number of tradeoffs and things to discuss. Let me fast-forward and show you one of them.
In one path through the stack problem, you can end up with a partial pop() implementation that looks like this:
public void pop() {
if (top == -1)
throw new StackUnderflowException();
top--;
}
(Note that there's a top() implementation floating around that does the useful work of returning the topmost element)
When I get to this point, I usually mention that we can eliminate a little bit of duplication. I point back to another method I've written that looks like this:
public boolean isEmpty() {
return top == -1;
}
It sure is duplication, isn't it?
So, off we go.
public void pop() {
if (isEmpty())
throw new StackUnderflowException();
top--;
}
Aaaaahhh! Relaxation. We eliminated duplication. All is well.
But is it? I have to admit that I feel rather dirty when I do this, and it's taken a while to understand why. Here's my best explanation of it so far.
Classes present interfaces with their public methods. When a method is marked public, it's easy to assume that it's an entry point for an object. When an object uses it's own public methods, there's an opportunity for misunderstanding.
I'm sitting in an airport right now, so I can't pull up a real example, but here's a hypothetical. Suppose that we have some invariant that we'd like to preserve for an object. We go to each public method and make sure that it preserves the invariant. This could involve some checking. If we do the checking in each public method, and public methods call other public methods, we could be doing that work redundantly.
Now, having said this, I know there are many cases where it causes no problems at all. But, I suspect that there is a lot of well-factored code out there that just falls in line with this idea implicitly. I also notice that this sort of thing is recognized at higher levels in some systems also. For instance, it would be pretty sad if the facade of a subsystem was used prolifically in the subsystem itself.
If there is a principle here, I think that is a bit loose. I don't like the duplication we would have if we left the top == -1 check in both places in the Stack code, but I think there is something to this.
What do you think?
!commentForm
I think you're onto something. The original code has "top == -1" and "top--" near each other where a reader can see the clear relationship between the two. The refactored code has less duplication but the relationship between "isEmpty()" and "top--" is not at all clear. So, in this unusual case we can't have both DRY and expressive code.
So, why can't top manage it's own state better?
I recently read Prefactoring by Ken Pugh, and that's called a lot of primitive type use to my attention. I think this might be an interesting example.
Suppose that, instead of top being an int, top was a Position. (That doesn't sound quite right, but this is just off the top of my head.)
class Position
{
public bool IsEmpty[?]() {...}
public void Decrement() {...} // throws an exception
}
If you combine this with your new principle, then you can get rid of the duplication *and* the call of a public method.
Hmm, this may not work as well as I've thought. Now the Position seems to want to support both the IsEmpty[?] and Decrement methods, so a choice still needs to be made between duplication and calling a public method. It would clean up the Stack class but still leave the choice in the Position class. Somehow it seems like a smaller problem there, though. But I'm honestly still not sure which I would prefer to write.
So, why can't top manage it's own state better?
I recently read Prefactoring by Ken Pugh, and that's called a lot of primitive type use to my attention. I think this might be an interesting example.
Suppose that, instead of top being an int, top was a Position. (That doesn't sound quite right, but this is just off the top of my head.)
class Position
{
public bool IsEmpty[?]() {...}
public void Decrement() {...} // throws an exception
}
If you combine this with your new principle, then you can get rid of the duplication *and* the call of a public method.
Hmm, this may not work as well as I've thought. Now the Position seems to want to support both the IsEmpty[?] and Decrement methods, so a choice still needs to be made between duplication and calling a public method. It would clean up the Stack class but still leave the choice in the Position class. Somehow it seems like a smaller problem there, though. But I'm honestly still not sure which I would prefer to write.
Umm, my decrement operators seem to have been interpreted as turning on and off strikethrough. Sorry about that.
- I cleaned them up for you. You can avoid that using the literal widget - David Chelimsky
Do we really need method for empty check? If I don't ask emptiness of the stack, I don't need isEmpty method. So problem will be disappeared.
PP said "Tell don't Ask". I can remember someone said "Try don't Ask". ;-)
PP said "Tell don't Ask". I can remember someone said "Try don't Ask". ;-)
I think this is the situation where AOP can help.
An alternative that avoids the"use of public methods" problem is to use a state pattern. Much of the Stack's behaviour revolves around "Empty" and "Non-Empty", so create two StackState[?] classes to handle this behaviour.
I've come across a problem like this when testing using mocks. Imagine you're doing an MVP implementation and you want to specify that every time your presenter receives any updates from the view that it, in turn, updates the view:
How do you specify/verify that UpdateView() gets called? You could have a mock view and verify that it receives messages to update fields 1, 2, 3. But then when you change UpdateView() to also update field 4, you have to change all of those tests. OK, so maybe in your test class you have a separate method that your test methods call, so all of the verification happens once, but what I REALLY want to specify is that it updates the view - not that it updates it correctly (which is tested in other methods that test directly the ability to update the view).
I've handled this in two different ways. One is to use a "partial mock" where you subclass the class under test and override the UpdateView() method to record whether it receives the message. The other way is to pull UpdateView() out to a ViewUpdater class with UpdateView(domainObject) on it. This approach makes for more little classes (ravioli), but reduces the confusion you talk about that seems to be the result of some form of spaghetti. Angel hair? Linguine? All ideas are welcome here...
public class Presenter
{
public void Field1Changed(value)
{
domainObject.Field1 = value;
UpdateView();
}
public void Field2Changed(value)
{
domainObject.Field2 = value;
UpdateView();
}
public void UpdateView()
{
view.Field1 = domainObject.Field1;
view.Field2 = domainObject.Field2;
view.Field3 = domainObject.Field3;
}
}
How do you specify/verify that UpdateView() gets called? You could have a mock view and verify that it receives messages to update fields 1, 2, 3. But then when you change UpdateView() to also update field 4, you have to change all of those tests. OK, so maybe in your test class you have a separate method that your test methods call, so all of the verification happens once, but what I REALLY want to specify is that it updates the view - not that it updates it correctly (which is tested in other methods that test directly the ability to update the view).
I've handled this in two different ways. One is to use a "partial mock" where you subclass the class under test and override the UpdateView() method to record whether it receives the message. The other way is to pull UpdateView() out to a ViewUpdater class with UpdateView(domainObject) on it. This approach makes for more little classes (ravioli), but reduces the confusion you talk about that seems to be the result of some form of spaghetti. Angel hair? Linguine? All ideas are welcome here...
I don't like the duplication either, but I suspect a good solution might be to encapsulate it into a private method, like so:
private boolean empty() {
return top == -1;
}
Then you've got:
public boolean isEmpty() {
return empty();
}
public void pop() {
if (empty()) {
throw new StackUnderflowException();
top--;
}
I'd have a hard time defending this code under duplication removal, but it's real easy to defend on the basis of improved clarity.
John Roth
private boolean empty() {
return top == -1;
}
Then you've got:
public boolean isEmpty() {
return empty();
}
public void pop() {
if (empty()) {
throw new StackUnderflowException();
top--;
}
I'd have a hard time defending this code under duplication removal, but it's real easy to defend on the basis of improved clarity.
John Roth
John, yeah I thought of that but it feels like overkill to me if isEmpty doesn't do something more. It also feels like another form of duplication. This is a really tricky proto-principle. I think I would stop short of delegating from isEmpty to empty, but I would be very happy if a class didn't have to use its own public interface at all, and it seems that a lot of classes are like that. It's puzzling.
Dhui, yes, I agree. AOP seems like a bandaid for this sort of thing. I wonder whether a more direct language mechanism would help, maybe another using another keyword to warn people that we are using a public internally: public internal isEmpty() { ... } (apologies to everyone using .NET ;-> )
Truth is, I've thought about this proto-principle for years on and off, and I've pretty much settled on "if it ain't broke, don't fix it." I do think it's funny that a lot of code ends up doing it implicitly though, and that cases like the one David do come up at times, but it seems like many of the cures are worse than the disease, or at least don't need to be applied all the time.
Dhui, yes, I agree. AOP seems like a bandaid for this sort of thing. I wonder whether a more direct language mechanism would help, maybe another using another keyword to warn people that we are using a public internally: public internal isEmpty() { ... } (apologies to everyone using .NET ;-> )
Truth is, I've thought about this proto-principle for years on and off, and I've pretty much settled on "if it ain't broke, don't fix it." I do think it's funny that a lot of code ends up doing it implicitly though, and that cases like the one David do come up at times, but it seems like many of the cures are worse than the disease, or at least don't need to be applied all the time.
The Single Responsibility Principle states that a module should only have one reason to change, right? So let's say that the meaning of IsEmpty[?]() changes (I realize that's unlikely, but just play along...). In John's implementation you only have to change the IsEmpty[?] method. Pop need not change. If Pop() is relying on IsEmpty[?]() then Pop() must change when its spec changes AND when IsEmpty[?]()'s spec changes.
The thing that makes me feel uneasy here is that the new method seems
to be balancing between viewing the stack in terms of abstract operations
and its implentation.
I think I feel slightly more comfortable with the following implementation:
public void pop() {
if (isEmpty())
throw new StackUnderflowException[?]();
removeTopElement();
}
private void removeTopElement() {
// Precondition: the stack is not empty
top--;
}
Although arguably the check and exception are an explicit
precondition anyway. Any thoughts on this?
to be balancing between viewing the stack in terms of abstract operations
and its implentation.
I think I feel slightly more comfortable with the following implementation:
public void pop() {
if (isEmpty())
throw new StackUnderflowException[?]();
removeTopElement();
}
private void removeTopElement() {
// Precondition: the stack is not empty
top--;
}
Although arguably the check and exception are an explicit
precondition anyway. Any thoughts on this?
We (univerity of hamburg) use the stack example alot in teaching and from my innocent point of view isEmpty() is a precondition of pop.
public void pop() {
assert isEmpty(): "precondition violation: isEmpty()";
top--;
}
Methods used in precondition need to be public because a client has to be able to check the precondition (isEmpty()) before he calls pop(). Non-public proconditions do not help much, because the client could never be sure about fulfilling the preconditions.
From my point of view exceptions should only be used to indicate that something exceptional has happened. For instance the network is down, the database is down etc. Anything within the logic of the program, that can be found out by simply calling a method - such as isEmpty() - should not be propagated by an exception.
I hope this does not sound dogmatic, which is probably due to my lack of english practice. We teach this approach to our students, but we work with it in real world projects as well and it works fine.
public void pop() {
assert isEmpty(): "precondition violation: isEmpty()";
top--;
}
Methods used in precondition need to be public because a client has to be able to check the precondition (isEmpty()) before he calls pop(). Non-public proconditions do not help much, because the client could never be sure about fulfilling the preconditions.
From my point of view exceptions should only be used to indicate that something exceptional has happened. For instance the network is down, the database is down etc. Anything within the logic of the program, that can be found out by simply calling a method - such as isEmpty() - should not be propagated by an exception.
I hope this does not sound dogmatic, which is probably due to my lack of english practice. We teach this approach to our students, but we work with it in real world projects as well and it works fine.
Interesting. I had this exact conversation with a peer about a week ago. The situation I ran into was that of testing. Typically, we write a test case for each public method on a class. If public methods call others, you aren't able to test effectively. Your unit tests are forced to test more than just a single unit. I also found the code very difficult to understand, which is possibly a symptom of what the method invocation scheme actually led to. Here's some general guidelines, each related in subtle ways:
Public methods should only call private methods on the same class or public methods on another class.
Private methods should be used to emphasize functional cohesion within a class.
Public methods should be used to present the class interface.
If you find the need to test a private method independently, refactor it to a public method on another class.
Never allow a bi-directional compile-time relationship between two classes where each invokes a public method on the other.
Public methods should only call private methods on the same class or public methods on another class.
Private methods should be used to emphasize functional cohesion within a class.
Public methods should be used to present the class interface.
If you find the need to test a private method independently, refactor it to a public method on another class.
Never allow a bi-directional compile-time relationship between two classes where each invokes a public method on the other.
..and I thought I was the only one feeling uneasy about public methods calling public methods...
I got to admit, my gut reaction is usually to refactor the core logic into a private method and have the two public methods call it, like John suggested above.
Yes, the public method that just calls the private one feels a bit silly all of a sudden, but I guess that I see it as the lesser evil, when compared to either the duplicated code, or the public-calling-public issue.
Any more thoughts on this topic ?
I'm very curious about alternatives.. for instance, the assertion suggested by Carola.
I think this is a question hard to dismiss. Doing TDD, I often end up making some method public to make it easier to test it, so I end up in this public-calls-public issue more often than one could expect.. and it always feels smelly...
I got to admit, my gut reaction is usually to refactor the core logic into a private method and have the two public methods call it, like John suggested above.
Yes, the public method that just calls the private one feels a bit silly all of a sudden, but I guess that I see it as the lesser evil, when compared to either the duplicated code, or the public-calling-public issue.
Any more thoughts on this topic ?
I'm very curious about alternatives.. for instance, the assertion suggested by Carola.
I think this is a question hard to dismiss. Doing TDD, I often end up making some method public to make it easier to test it, so I end up in this public-calls-public issue more often than one could expect.. and it always feels smelly...
Add Child Page to AnotherDesignPrinciple