Also, I dislike how MissingBasket is not a subclass of basket. And why not use Basket.where(id: id).first_or_initialize
On 23 February 2016 at 13:46, Graham Ashton <[email protected]> wrote: > Hi Robert. Firstly, I really like the approach of avoiding nil checks – > good choice for an article. > > While reading your example code I got a bit confused though. I wouldn’t > expect the `#line_items` method to ever return `nil`, as I’d expect it to > always return a collection (ActiveRecord is so prevalent in Rails that I > just assume it). > > So if there was a basket, but there weren’t any line items in it, I’d > expect to get an empty array. > > Later on, when you introduce `MissingBasket` I get the feeling that > substituting it for a `nil` basket might have been what you actually > intended. > > In case my description is hard to follow, I’m saying I think you probably > meant to write something like this: > > <% if @basket.present? %> > <% @basket.line_items.each do |item| %> > <p><%= item %></p> > <% end %> > <% end %> > > I think there’d be less potential for people to get tripped up by this if > you changed the example code so that `@basket` is `nil`, rather than having > `Basket#line_items` returning `nil`. > > Also, whether you need a unit test for the `if` is a slightly contentious > one (I suspect the more testing you do the less important such tests seem), > so (in order to prevent the reader from getting distracted by it) I’d > probably remove that paragraph if I was you. I don’t think you need it to > reinforce the benefits of the Null object pattern anyway – it’s undeniably > cleaner to just avoid the conditional. > > Cheers, > Graham > > > On Tue, 23 Feb 2016, at 11:47 AM, Robert Whittaker wrote: > > In an attempt to get them to share more, I have asked my team at On the > Beach to submit a blog post every month about something they have learned. > Today, I submitted my first article! > > > > http://purinkle.co.uk/post/139843782736/on-the-if > > > > Let me know your thoughts. Here are some helpful links just in case you > want to share. > > > > * https://twitter.com/purinkle/status/702084436234477568 > > * https://www.facebook.com/purinkle/posts/10100204829015603 > > * https://plus.google.com/115653446145456682786/posts/MoKipM4pr1W > > * https://www.reddit.com/r/ruby/comments/476406/on_the_if/ > > * https://www.reddit.com/r/programming/comments/47650b/on_the_if/ > > * https://news.ycombinator.com/item?id=11158011 > > > > -- > > You received this message because you are subscribed to the Google > Groups "North West Ruby User Group (NWRUG)" group. > > To unsubscribe from this group and stop receiving emails from it, send > an email to [email protected]. > > To post to this group, send an email to [email protected]. > > Visit this group at https://groups.google.com/group/nwrug-members. > > For more options, visit https://groups.google.com/d/optout. > > -- > You received this message because you are subscribed to the Google Groups > "North West Ruby User Group (NWRUG)" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > To post to this group, send an email to [email protected]. > Visit this group at https://groups.google.com/group/nwrug-members. > For more options, visit https://groups.google.com/d/optout. > -- You received this message because you are subscribed to the Google Groups "North West Ruby User Group (NWRUG)" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send an email to [email protected]. Visit this group at https://groups.google.com/group/nwrug-members. For more options, visit https://groups.google.com/d/optout.
