CODEWITHSUNDEEP
ruby
megas
megas

Reputation: 21791

How to optimize Ruby method

I have a 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 tap or yield_self, but here I can't find a nice way to optimize it.

The only solution I have come up:

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

But it doesn't better than the original solution, is it?

Upvotes: 1

Views: 189

Answers (3)

Cary Swoveland
Cary Swoveland

Reputation: 110685

One could write the following.

class Client
  def account_status
    "Overdrawn!"
  end
  def close_session
    puts "It's closed"
  end
end

def create_client(creds)
  Client.new
end    

def get_status(creds)
  begin
    client = create_client(creds)
    client.account_status
  ensure
    client.close_session if client
  end
end

get_status("Anything")
It's closed
  #=> "Overdrawn!"

Do I prefer this to #1 in the question? No.

Do I prefer this to #2 in the question? Yes!

Do I prefer this to @max's answer? No.

I understand a finalizer could be created using the class method ObjectSpace::define_finalizer.

class Client
  def initialize
    ObjectSpace.define_finalizer(self, proc { puts "It's finalized!" })
  end

  def account_status
    "Overdrawn!"
  end
end

def create_client(creds)
  Client.new
end    

def get_status(creds)
  create_client(creds).account_status
end

get_status("Anything")
  #=> "Overdrawn!" 
exit
It's finalized!

One must be careful when creating finalizers, as explained Here. I understand a technique sometimes used is to have finalizer's proc reference class-level objects. See, for example, this article, @Amadan's comments below and @Matt's comments on the question. I am not advocating the use of a finalizer. I merely thought readers unfamiliar with finalizers (as I was before writing this) would find this useful.

Upvotes: 3

spickermann
spickermann

Reputation: 106882

I like your first version because it is short, easy to read, and easy to understand. I would not change it.

Nevertheless, an alternative version using tap might look like this:

def get_status(creds)
  client = create_client(creds)
  client.account_status.tap { client.close_session }
end

Upvotes: 1

max pleaner
max pleaner

Reputation: 26768

Let's list the goal of the function:

  1. Open connection
  2. Read value (and return it)
  3. Close connection

I would consider this a "temporary connection", and that leads me to think it could be refactored to a separate method.

Reasoning: The get_status method is concerned with getting the status from a connection - it doesn't have to handle the details of actually closing/opening the connection itself.

def open_temporary_connection(creds, &block)
  client = create_client(creds)
  result = block.call(client)
  client.close_session
  result
end

def get_status(creds)
  open_temporary_connection(creds, &:account_status)
end

Also, I should mention, I think yield_self is a bit of a trap. Unless you're dead set on making all of your code into a single expression, it makes the code look awkward without offering a lot of benefit.

Upvotes: 2

Related Questions