This is a companion discussion topic for the original entry at https://thoughtbot.com/upcase/videos/tell-don-t-ask
Good stuff!
Could you guys give tips on how to spec/test NullObjects?
I test them just like other objects. A NullObject has specific things itâs supposed to return when you send it certain messages, so I write tests for those things.
@alexbush Iâve sometimes written tests to check that a null objectâs interface matches the interface of the object it stands in for. This can help with the problem Joe and Ben talked about in the video, where interfaces get out of sync.
There are details in this blog post: Testing Null Objects
@benorenstein Does it really make sense to test a NullObject, apart from making sure it complies with the interface of the object it should stand in for?
I tend to think of a NullObject as something too trivial to test (unless when itâs not). Do you test it for coverage sake, for documentation?
Where is the place that we usually put the conditional to return null object if the object to be accessed is not presence? Should AR model know this knowledge to return null object when its attribute missing value? In the example of NullAddress, I feel it is used for removing view presentation logics only.
address || NullAddress.new
Not just view logic. You can take advantage of it anywhere an address is used, on the views, controlles, services, other models, etc⌠You get the benefit of not having to check if it is nil anymore.
Sure, why not? Itâs just another object that responds to messages. I like to test my objects.
@zamith @benorenstein sometimes I test my null objects, but I only best them to compile with the interface of the object that it stands for. In addition, I test full behaviors of special case objects. I like the ideas of differentiate between special case objects and null objects. Null objects usually just compile to the interface and do nothing like NullLogger, but on the other hand Special case objects compile to their interface and can also have some behaviors Like GuestUser. So basically, null object pattern is just a specialize form of special case pattern.
The thing is that I tend to weight the value of having a test on roughly these aspects:
- Design feedback
- Regression testing
- Documentation
- Overall confidence and joy it gives me
And most Null objects, given that they are really simple and have no logic in them, will at most fulfil the 2nd and 3rd, IMO. I do understand why you would test them, but still I would always question myself how much value is it really adding, mostly according to these parameters.
Thanks for the feedback, btw.
Thatâs fair. I tend to just stick to just always stick to a TDD-based process unless the circumstances are particularly weird.
For the first example in Tell, Donât Ask which relates to showing a welcome message to the user, better code puts view related code into the model object. As a general principle I try to keep the model objects as simple thin database wrappers as opposed to them being junk drawers for random view code. The alternative to the implementation in the video is to have a helper object that is responsible for building the welcome message for a given user however I canât think of a clever way of not violating Tell Donât Ask and keeping the logic out of the model.
module UserHelper
def welcome_message(user)
(user.admin?) ? "Welcome Admin!" : "Welcome User!"
end
end
In your experience is it more important to obey Tell Donât Ask or keeping models only as thin database wrappers and keeping extra logic out of them altogether.
It feels to me that the example is really a view concern rather than a model concern and the logic should not be added to the model itself even though it is technically violating Tell Donât Ask.
What you are doing here is fine. Tell, donât ask is more about donât ask about the internals of an object and then reach into that object to do something. That is in fact why this shouldnât on the user model. You shouldnât have to âdoâ something to the user model to accomplish this, you just need to query itâs role.
Right. The motivation for my question was that Iâve seen a few systems where the User model becomes a junk draw with a lot of code that relates to messages show in the view and it becomes messy over time. I see youâre point though, in my implementation I am asking for the role of the user but unlike the example in the video I am not reaching into the user to get any of its internals. However what If I were to modify the example to be the following:
module UserHelper
def welcome_message(user)
if user.admin?
"Welcome #{user.display_name}, you have phenomenal cosmic powers!"
else
"Welcome #{user.display_name}!"
end
end
end
I think this makes it closer to the example in the video where I am asking a question about the user and then reaching into the object to learn about its state in order to craft the welcome message.
Then I would reach into my bag of tricks and pull out Polymorphism over Conditionals:
module AdminWelcomeMessage
extend self
def message(user)
"Welcome #{user.display_name}, you have phenomenal cosmic powers!"
end
alias_method :call, message
end
module UserWelcomeMessage
extend self
def message(user)
"Welcome #{user.display_name}!"
end
alias_method :call, message
end
module UserHelper
def welcome_message(user)
user.admin? ? AdminWelcomeMessage.(user) : UserWelcomeMessage.(user)
end
end
Whatâs the purpose of the alias_method? Personally I like the semantics of calling AdminWelcomeMessage.message(user) or UserWelcomeMessage.message(user). Perhaps thatâs just a coding style thing.
The main idea here is pulling the code that accesses the user state into a separate class that has semantic meaning instead of inlining it into a branch of the conditional. Moving it into separate modules doesnât fix the tell donât ask violation though it just provide semantic grouping around it. It does provide a cleaner way to generate view text based on model state without having to put it in the model itself.
@Dreamr Should your code example demonstrate the use of polymorphism instead of conditionals? You still have exactly the same conditional @ajorgensen had in his code which confuses me a lot. If you want to achieve true polymorphism you need to put welcome_message
on the user object and have two different user classes User
and Admin
. UserPresenter
and AdminPresenter
would work as well.
@timhabermaas thatâs one way of doing it but in one of my follow ups I said:
The motivation for my question was that Iâve seen a few systems where the User model becomes a junk draw with a lot of code that relates to messages show in the view and it becomes messy over time.
I think the important lesson here is that our job as programmers is to manager trade-offs and that these rules are more like guidelines than hard and fast rules. Itâs damn near impossible to always follow all of them. Gary Bernhardt has a Destroy All Software screencast about conflicting principles.
Personally I would rather violate tell donât ask in this case rather than stick the view concern on the user model where it definitely doesnât belong. For this example I think SRP trumps Tell Donât Ask. Polymorphism would work but I donât think its the best solution even thought it gets rid of the Tell Donât Ask violation.
I agree, donât put welcome_message
on User
. I was just utterly confused by Dreamrâs âbag of tricksâ response and tried to suggest a straight-forward way of using polymorphism without changing any of the existing implementation.
We can at least minimize these violations by pushing the decision further up:
a) Violate SRP, obey TDA:
class User
def presenter
admin? ? AdminPresenter.new(self) : UserPresenter.new(self)
end
end
b) Obey SRP, violate TDA:
class PresenterBuilder
def wrap(record)
return record unless record.is_a? User
record.admin? ? AdminPresenter.new(record) : UserPresenter.new(record)
end
end
I prefer one of these solutions to the one discussed in this thread, because they make sure user.admin?
isnât repeated several times. So if you later decide to implement two subclasses instead of a boolean flag you only have to go to one place.
\edit: Bad example. You could always quickfix it by doing this:
class Admin
def admin?
true
end
end
I like the idea of removing the admin?
call from multiple places. If you were to ever have multiple types of admin (super, etc) then youâd have to re-do all that logic in so many different views. Gems like cancan can also help with this a little by moving that sort of logic into a central place. Personally Iâm more of a fan of your b) example.