How to optimize Ruby method

Question

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?

Can you give a clear, unambiguous, precise, objectively measurable definition of what you mean by "better"? What is and isn't "better" is generally based on subjective opinions, which makes questions about "better" off-topic unless "better" is precisely defined. Same applies to "optimized". There are many things to optimize: memory usage, runtime, throughput, latency, cold-cache startup time, warm-cache startup time, … Also, if your code works and you are asking for improvements, then Code Review is generally better equipped for that than Stack Overflow. Make sure to study their help center first. — Jörg W Mittag
– Jörg W Mittag, Commented Aug 30, 2020 at 10:01
If you move/repost this on Code Review, make sure you pick a descriptive title and add a description of what the code actually does. Stack Overflow focuses on short MCVEs, Code Review requires context. See the guide. — Mast
– Mast, Commented Aug 30, 2020 at 10:56
IMHO the first version is much better than the second. It is shorter, easier to read, and easier to understand. I see no reason to change it. — spickermann
– spickermann, Commented Aug 30, 2020 at 15:38
Consider creating a separate class with a finalizer that closes the session. Then simply return the account_status, and when the object goes out of scope it will automatically call close_session. — Matt
– Matt, Commented Aug 30, 2020 at 18:27
The tricky thing with a finalizer is that you can't access the object itself in the finalizer, since it may have already been destroyed, but you do have access to its object_id. If you can save everything you need about an object in order to close_session on it into a class hash variable while it is instantiated, then you can retrieve it based on the object_id in the finalizer. If Cary or anyone else wants to do this, please give the credit to them instead. Personally I would stick to your first version. — Matt
– Matt, Commented Aug 31, 2020 at 13:32

Cary Swoveland · Accepted Answer · 2020-08-31 15:40:09Z

3

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.

edited Aug 31, 2020 at 15:40

answered Aug 30, 2020 at 18:29

Cary Swoveland

111k6 gold badges69 silver badges105 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Amadan Over a year ago

I disagree. Change client.close_session to client.close_session if client or client&.close_session, and I'll prefer this version over others - if a connection is opened, it should be closed, and there are only two ways to guarantee it: ensure, or a finalizer (and if client cleans up in a finalizer, then #close_session is noise).

Amadan Over a year ago

I just saw @Matt's comment. Yes, finalizer is superior. But a) it is notoriously tricky to get right, and b) the client class might not be under your control, and maybe you don't want to make a wrapper, or monkeypatch it.

Amadan Over a year ago

See my example for finalizers in question comments - creating a proc inside an instance method captures the instance into the closure, even if you don't use it, causing a memory leak. "Notoriously tricky". :) The article you linked suggests the same technique I used: create the proc inside a class method.

max pleaner · Accepted Answer · 2020-08-30 16:59:45Z

Let's list the goal of the function:

Open connection
Read value (and return it)
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.

spickermann · Accepted Answer · 2020-08-31 04:10:47Z

1

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

answered Aug 31, 2020 at 4:10

spickermann

108k9 gold badges115 silver badges147 bronze badges

Collectives™ on Stack Overflow

How to optimize Ruby method

3 Answers 3

3 Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

3 Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related