Thought inducing, as ever, but I don't like it :-). I would say that a null catalog is an invalid state for the Controller (and would even have asserts). This trick is effectively using invalid state isn't it?
I very much like the smell leading to the identification of an opportunity to separate concerns, and I understand the pushback unfortunately. As programmers we tend to optimise for code size of all things!, but yes, in this example I would split them.
1 reply
Yes, a null Catalog seems indeed like an invalid state for the Controller, and that's what leads me to wonder why a significant path through the Controller doesn't need the Catalog! That's the tension that leads me to wonder if something about the design might be wrong.
Simply saying "I won't allow this invalid state to happen" ignores the underlying risk. It contributes to a situation in the future in which we clearly want to separate behavior B from class C, but can't, because B happens to be tangled up with seemingly-unrelated fields F1, F2, F3... and we have to perform delicate surgery or write a bunch of tests to gain confidence that we can safely pull B apart from F1, F2, F3...
...or we could spend a few minutes now when there's just F1 (catalog), thinking about whether we have enough confidence to extract B (handle empty barcode) from C (controller). What would give us that confidence? Should we explore it or wait for more evidence by building more, related behavior?
So what don't you like, again? :)
1 reply
I think we are in violent agreement :-) - I wasn't justifying catalog being in the Controller, merely pointing out that _if_ it was valid state I would have asserted it.
1 reply
Yes, I can see how leading with that point makes more of it then I meant. I simply meant that I would have come to the same conclusion thinking "good (self) discussion, more important things here I come" but then it would have niggled and niggled until I fixed it and split the collaborator :-).
The main reason I guess is because I just know that every time I revisit that code it would nag at me (3.00am, still not sleeping, what's bugging me? Oh yeah...). I also would push back against that code when pairing/reviewing other's code so .....
In other words, 100% with you except for the ending, which I think is little more than stylistic differences/little more than splitting hairs.
The most important take away (far more important than whether it is split or not) is learning to identify the smell and then scrabbling away uncovering what it is that is making that smell.
1 reply
I think I understand; thanks. If seeing "null" there would bother you, then you should find some other way to express the risk without feeling compelled to "fix it" before you know it's a problem. You might prefer a comment, for example. I don't mind. If you and I worked in the same code base, I could tolerate a comment, although I feel fine leaving the null as it is.
Some people hate public fields. I don't. I know that when I notice a public field, I'll think about how to make it non-public, but it doesn't fill me with bile the way it does to others and used to do to me.
They're just conventions. Use the conventions that fit you. This convention fits me.