refactor guardian class for clarity & correctness (PR #862)

introduce NullUser to avoid type-checking DRY up code reduce number of multiple returns remove some redundant/impossible logic branches add pending test for possible bug

GitHub

You’ve signed the CLA, mattvanhorn. Thank you! This pull request is ready for review.

I like the idea of anonymous? and logged_on? (maybe authenticated?) - if I get some time, I’ll look at adding that. The pattern I saw all over the place was “Do I have a user and this other object? (sometimes a kind of user, sometimes another object)” So I came up with have_both?. Ideally, I’d like to make everything a lot more object oriented, so that all these objects knew how to talk to each other without all the parameter passing, but I was pretty satisfied with this as a first step. (@avdi also suggested renaming NullUser to GuestUser, and I like that as well.)

I spent a bit of time thinking about the NullUser thing, naming wise I think AnonymousUser is a way better name.

One trap here is that there is no real interface defined, so @user.bla is not necessarily a safe call if we introduce this Null object. I worry that this can lead to some nasty bugs down the line if a test is forgotton, for example:

def authorized? 
  @user.present?
end
---

def can_skip?
  authorized? && @user.skippy? 
end

vs:

def can_skip?
  @user.skippy? 
end

The issue the NullObject has is that now you need to remember to bolt on the .skippy? method to the AnonymousUser, this is a bit error prone.


I wonder what @eviltrout thinks of all of this, Guardian is his creation so he will process this PR.

(also needs a rebase, I made a few changes today)

That’s kind of the fundamental problem(?) of coding in any dynamically-typed OO language. We don’t have interfaces a la Java or C#, so we have to be mindful.

More constructively: this would be a good place to add a shared spec for “user-like” objects, which is included into the specs for both GuestUser/AnonymousUser and User. Any addition to the public interface of user-like objects should start with an addition to this shared spec.

Funny aside: I used AnonymousUser until I was chatting with Josh and James one day and they ran GuestUser by me and I liked it a lot better. Matter of taste, I’m sure.

Pretty hard to argue against this pull request. It makes each check a lot shorter and cleaner and I’d be delighted to replace my old code with it.

  • I agree that AnonymousUser is a better name but I’d be okay accepting this and renaming it later.
  • Not a fan of has_both either, I think we could rename it or change it to something more clear.
  • Adding methods is a concern, but I think they would always break tests with no method exceptions? Maybe that won’t be the clearest error but the user implementing the guardian should hopefully figure it out.

Either way, awesome work.

I’ll rename the user class today, and push something later this morning, when I introduce anonymous/authenticated

As far as the NullObject pattern - it is possible in a lot of cases to make smart use of method_missing, but really, @avdi’s idea of having a common set of tests is a good one if the class becomes public. (It should behave_like_a_user)

Right now, I’ve isolated the GuestUser to just the Guardian class itself, and changed the accessor, so collaborators always get either a real User, or nil, much like before, except that they never have to worry about getting “bob” or a FIxnum or anything else.

Happy with these changes, just awaiting a rebase, our code base has diverged a bit, there is a merge conflict

note regarding Guest vs Anonymous, I want to keep consistent throughout the project, there are a bunch of spots we call anonymous users … hmm … anonymous users. I would really like to avoid having two terms for the same thing. It does not really matter we can merge this in and change, its not blocking the PR.

I am open to having a BaseUser type contract especially if this is a contract we would like to consume elsewhere.

Thanks heaps

Anonymous is the way to go, I think.

OK, I changed the name, and rebased, taking the changed logic from upstream side. Need to bundle and run tests, and then will push changes.

I don’t think there’s a need for a BaseUser yet. I prefer to let these sorts of things be driven by necessity, and just make it easy to rearrange the code when it finally is needed, by following good SOLID principles.

ack! I fail at Git. Closing - will redo soon.