2
\$\begingroup\$

I am trying to write a method, that works with three types of arguments, but requires only one of them.

def convert(arg_a: 1, arg_b: 2, arg_c: 'foo')
end

Please note, that both: arg_a, and arg_b are the same type (let's say Numeric), so using one mandatory argument, and then making decision based on the input type won't work here.

At this point my code looks like this:

def convert(arg_a: nil, arg_b: nil, arg_c: nil)
  if arg_b.nil? && arg_c.nil? && arg_a
    # do something with arg_a
  elsif arg_a.nil? && arg_c.nil? && arg_b
    # do something with arg_b
  elsif arg_a.nil? && arg_b.nil? && arg_c
    # do something with arg_c
  else
    raise ArgumentError
end

In my opinion this code smells a little, and can be improved. Any thoughts?

\$\endgroup\$

migration rejected from stackoverflow.com Apr 16 at 15:54

This question came from our site for professional and enthusiast programmers. Votes, comments, and answers are locked due to the question being closed here, but it may be eligible for editing and reopening on the site where it originated.

put on hold as off-topic by 200_success, Graipher, Vogel612 Apr 16 at 15:54

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – 200_success, Graipher, Vogel612
If this question can be reworded to fit the rules in the help center, please edit the question.

4
\$\begingroup\$

There are lots of ways of improving this; at a high level, I'd say it's possible the method itself should be broken up into multiple methods with distinct names, because a method that accepts three different inputs and does three different things with them probably doesn't have a single responsibility.

That not withstanding, you can clean this method up by separating the argument validation from the rest of the logic. There are lots of ways of doing this, but if you just need exactly one non-nil argument, you can use something along these lines:

def convert(arg_a: nil, arg_b: nil, arg_c: nil)
  raise ArgumentError unless [arg_a, arg_b, arg_c].compact.one?

  if arg_a
    # do something with arg_a
  elsif arg_b
    # do something with arg_b
  elsif arg_c
    # do something with arg_c
  end
end
\$\endgroup\$
  • \$\begingroup\$ The solution proposed by you does makes the code much clearer - I think this is the answer I was looking for. This convert method is only for argument validation - based on it other methods are called. \$\endgroup\$ – ciejj Apr 15 at 17:30
2
\$\begingroup\$

From what I can tell, your implementation only makes use of one of the three arguments, and only really expects (or allows) a single argument at a time.

i.e., with your current implementation, this is what an error-free call-site looks like:

convert(arg_a: 1)
convert(arg_b: 2)
convert(arg_c: 'foo')

If the method were called with two or more arguments (any of them), it would raise an ArgumentError, so really, this method can only be called with a single argument.

Given that you're already using keyword arguments with a default value of nil, I cannot see how this is any better than simply writing three different methods that handle the three values. Therefore, something like...

def convert_arg_a(a)
  # Handle a...
end

def convert_arg_b(b)
  # Handle b...
end

def convert_arg_c(c)
  # Handle c...
end

...should be able to do exactly what is possible with the implementation you've described, with none of the branching.

New contributor
Hari Gopal is a new contributor to this site. Take care in asking for clarification, commenting, and answering. Check out our Code of Conduct.
\$\endgroup\$

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