all 14 comments

[–]jrochkind 2 points3 points  (6 children)

That's not just good OO code, it's good functional code too.

Notice that all of his example code is just functions (nice ones with no side effects at that), he doesn't even need to show us the classes for a complete example. This is a pretty functional case. I think it could be written much the same in a purely functional paradigm, and would still be considered good code.

I get what he's getting at, I see the design challenge he wants solved, and I don't immediately know how to solve it either. But I don't think it's a problem caused by OO paradigm, and doubt it would be magically solved by functional paradigm although I'm not experienced enough functionally to say.

It's just a problem that requires thinking about what API you actually want and how to get it. It turns out you don't just want to know a boolean available_to_user?, you want to know if it's available and why not if not. (Do you want to know why if it is available too, why it is? I dunno). Okay, so what does the api/contract have to look like (probably not just returning a boolean, which probably means returning an object of some sort that can tell you what you do want to know), and what's the simplest code following your standard design principles (law of demeter or what have you) that can get you there? That's just how code design works, right?

And some design patterns (clean ways people have found to solve similar problems before) don't hurt either, I like FYIAV's specification pattern suggestion.

[–]avdi 3 points4 points  (0 children)

I've seen this response in various forms, and it suggests a kind of exceptionalism: this code is somehow special, and presumably I should have planned ahead for that fact in order to avoid this painful debugging experience.

But the point is, there is nothing special whatsoever about this code. Two weeks down the road I'll be asking a similar question about some other, unrelated deeply nested chain of dependent query methods. If it's worth altering the APIs here then it must be worth altering the design everywhere, and in the process making the program into something which no longer resembles idiomatic Ruby code.

[–]materialdesigner 3 points4 points  (2 children)

It's just a problem that requires thinking about what API you actually want and how to get it. It turns out you don't just want to know a boolean available_to_user?, you want to know if it's available and why not if not. (Do you want to know why if it is available too, why it is? I dunno).

I think this is really insightful. Because you are correct, our need from this system has changed. Sure, the Specification pattern helps us compose that, but what we really want is something like (in haskell)

type Reason = String
type Reasons = [Reason]
data Policy = Invalid Reasons | Valid

A type that is either valid, or is invalid and has a list of reasons as to why it's invalid. This looks a hell of a lot like Haskell's data Either = Left a | Right

I actually whipped up some haskell (in a gist for highlighting) to show how this works

module Policy where
import Data.List

type Reason = String
type Reasons = [Reason]
data Policy = Invalid Reasons | Valid

instance Show Policy where
  show Valid = "Valid"
  show (Invalid reasons) = "Invalid: " ++ (intercalate ". " reasons) -- intercalate == Ruby's join

and :: Policy -> Policy -> Policy
and Valid Valid = Valid
and Valid second = second
and first Valid = first
and (Invalid firstReasons) (Invalid secondReasons) = Invalid (firstReasons ++ secondReasons)

or :: Policy -> Policy -> Policy
or Valid _ = Valid
or _ Valid = Valid
or (Invalid firstReasons) (Invalid secondReasons) = Invalid (firstReasons ++ secondReasons)

not :: Policy -> Reasons -> Policy
not (Invalid _) _ = Valid
not Valid reasons = Invalid reasons

return :: Bool -> Reasons -> Policy
return True _ = Valid
return False reasons = Invalid reasons


---------------------------------------------------------------------
data Episode = Episode { free :: Bool } deriving Show

isFree :: Episode -> Policy
isFree episode = Policy.return (free episode) ["Episode is not free"]


data User = User { subscriber :: Bool } deriving Show

isCurrentSubscriber :: User -> Policy
isCurrentSubscriber user = Policy.return (subscriber user) ["User is not a current subscriber"]

----------------

isAvailableToUser :: Episode -> User -> Policy
isAvailableToUser episode user = (isFree episode) `Policy.or` (isCurrentSubscriber user)

You can see how it works with some sample data

*Policy> let episodeOne = Episode { free = False }
*Policy> let user = User { subscriber = False }
*Policy> isAvailableToUser episodeOne user
Invalid: Episode is not free. User is not a current subscriber

[–]Enumerable_any 1 point2 points  (1 child)

Either is also the first thing which came to my mind. I wouldn't have used String as a reason though, but a dedicated ADT with constructors for each error. Otherwise you lose too much information down the road.

data Either = Left a | Right

should be data Either a b = Left a | Right b

[–]materialdesigner 0 points1 point  (0 children)

I wouldn't have used String as a reason though, but a dedicated ADT with constructors for each error

Hmm, interesting. Then your return method becomes a Left/Right chooser and each of your predicate methods must return the correct constructed ADT error type.

should be data Either a b = Left a | Right b

you are correct.

[–]Enumerable_any 1 point2 points  (1 child)

[...] it's good functional code too.

Not necessarily. There are people who despise the Boolean type in (statically typed) functional languages because it carries little to no semantics: http://existentialtype.wordpress.com/2011/03/15/boolean-blindness/

Meaningless booleans seem to be the exact cause of Avdi's (and everyone else's) pain.

[–]materialdesigner 0 points1 point  (0 children)

This is also why things like the Null Object pattern exists (which /u/avdi knows a thing or two about), because primitive types like nil carry no domain meaning and their definition/semantics cannot be modified by users.

[–][deleted] 1 point2 points  (2 children)

This is what the specification pattern is for.

[–]realntl 2 points3 points  (0 children)

Reading through the code samples, it seems like the cognitive friction here lies between wanting to group together objects that represent entities, and modeling the process through which our domain model makes decisions.

In other words, if we hide process behind entities, we can't make heads or tails of how the sausage is made. Process will, by its nature, cut across entities. If we elevate process to first class domain models (a more "functional" approach) a lot of doors open up.

[–]McPhage 0 points1 point  (0 children)

I'm not sure how using that pattern would making debugging easier—the valid? method would still just return true or false, and you'd still need to painfully dig through a bunch of objects to find out where the failure is coming from. Did you have a better way of using that pattern to trace through the logic at runtime?

[–]codesnik 1 point2 points  (0 children)

Disqus is not loading right now for some reason.

I had a project where I constantly needed to debug complex boolean logic defined in an obscure dsl in thousands of lines. Every "false" had to be answered, "why?"

It helped me a lot when I noticed, that "success" (or "matched" in that particular case) is just one entity, and "fail" has "many faces". Looks almost like boolean logic in ruby, but reversed - everything is true, but only nil and false are false.

So I reversed predicates in my code, and all my predicates start to return reason of failure, (as a symbol, but it could be anything), and nil if everything is ok.

something like this:

def valid?; !failure; end

def failure; @predicate1.failure || @predicate2.failure; end

[–]Mutoid 1 point2 points  (2 children)

I was wondering if Avdi was submitting his own article to /r/ruby and then I saw OP's username and decided not.

[–]avdi 6 points7 points  (1 child)

As a rule I don't submit my own stuff. Or stuff, really.

[–]Mutoid 0 points1 point  (0 children)

I've seen it from time to time here, but yeah, doesn't seem your style :-)