2
\$\begingroup\$

My Resolver.resolve class returns a hash of objects and I would like to return a specific object if it exists. Is there a more concise way to write this?

def resolve_url(url)
    # Resolver library now returns a hash of objects
    # Using this helper method to preserve the old functionality of returning a single object
    # Order of old return types is Lesson -> Series -> User
    objects = Resolver.resolve(url, :only => [User, Lesson, Series])
    if objects[Lesson]
      objects[Lesson]
    elsif objects[Series]
      objects[Series]
    elsif objects[User]
      objects[User]
    else
      nil
    end
  end
end
\$\endgroup\$
1
\$\begingroup\$

I might as well post this then:

def resolve_url(url)
    # Resolver library now returns a hash of objects
    # Using this helper method to preserve the old functionality of returning a single object
    # Order of old return types is Lesson -> Series -> User
    only = [Lesson, Series, User]
    objects = Resolver.resolve(url, :only => only)
    objects.values_at(*only).compact[0]
  end
end

So first get only the necessary fields (reusing the list from the resolve call), remove nils with compact and finally return the first value; if it's empty you still get nil back.

I've also removed my comment since you've updated your post. FWIW you could also use something like objects[Lesson] or objects[Series] or Objects[User] instead.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ You compute all three values, objects[Lesson], objects[Series] and objects[User], even though if may only be necessary to compute one or two, which could be an efficiency issue. \$\endgroup\$ – Cary Swoveland Dec 7 '14 at 5:25
  • \$\begingroup\$ Yep, if that's an issue a separate method would be in order. \$\endgroup\$ – ferada Dec 7 '14 at 11:21
1
\$\begingroup\$

You could refactor your if block into something like this. But I don't think this is a good idea. I'll explain why below.

object =
  objects[Lesson] || objects[Series] || objects[User] || nil

As pointed out in the comments, I would use a name other than objects and object.

Your example code is lacking context, so what I'm about to say may or may not be applicable, but...

  1. Your resolve_url method has two hard coded dependencies. The first is the name of the class Resolver. Should Resolver's name ever change, or be replaced, you must make changes to resolve_url.

  2. This is more subtle, but it's a dependency nevertheless. The order of arguments that Resolve accepts is hard coded into resolve_url. Should those change, those changes will cascade down.

Finally, should you add more classes to User, Series, and Lessons, you must modify your method, and add the necessary conditionals.

So ask yourself if this code really belongs in the resolve_url method. My two cents.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

You could also write it like this:

def resolve_url(url)
    objects = Resolver.resolve(url, :only => [User, Lesson, Series])
    [Lesson, Series, User].reduce(nil) { |v,e| v || objects[e] } 
end

As soon as objects[e] is non-nil, and the memo v is set to that value, v will remain unchanged for all subsequent values of e (and object[e] will not be computed for any of those values of e) and v will be returned by reduce.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.