-2
\$\begingroup\$

I have the following Ruby method.

def get_status(creds)
  client = create_client(creds)
  status = client.account_status
  client.close_session
  status
end

Usually I optimize this kind of code by using tap or yield_self. I have come up with a way to use these, but it doesn't seem like the nicest way to optimize it.

def get_status(creds)
  create_client(creds).yeild_self do |client|
    [client, client.account_status]
  end.yield_self do |client, status|
    client.close_session
    status
  end
end

Is my new solution better than the original solution?

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Define better and optimization please. Your review request lacks context. \$\endgroup\$ Aug 30, 2020 at 11:20
  • \$\begingroup\$ I did encourage you to read the guide, and not to post it as-is. Should I have been more clear? \$\endgroup\$
    – Mast
    Aug 30, 2020 at 11:51

1 Answer 1

2
\$\begingroup\$

First of all I also agree with the questions asked that you should define first why you want to optimize this method? With 4 lines of code I think this method is still readable and also your suggested refactoring would more obfuscate this method.

Anyway, here are some suggestions to improve the readability a little bit.

This is just a small refactoring but I think it makes it more clear that you create a 'session' and makes it more reusable. Basically it is just extracting a with_session method.

def get_status(creds)
  with_session(creds) do |session|
    session.account_status
  end
end

def with_session(creds)
  client = create_client(creds)
  result = yield(client)
  client.close_session
  result
end

Now the next step could be to adapt the client class (or extract a session class) and move this method in there instead.

class Session
  def initialize(credentials)
    @credentials = credentials
  end

  def execute
    client = create_client(credentials)
    result = yield(client)
    client.close_session
    result
  end

  private

  attr_reader :credentials
end

def get_status(creds)
  Session.new(creds).execute do |session|
    session.account_status
  end
end

In the end your method did not change much but giving it proper names, splitting concerns and moving it to the right place improved in my opinion the code.

\$\endgroup\$

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