Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a lot of code that looks like this:

value = TopicLinkClick.create_from(new_params)
return value unless value.nil?

# do something else

I find this code is not so good because creating a temporary variable is troublesome and disruptive of my workflow, and it takes time to read and understand a large block of code that is meaningless.

Is there a better way for me to do this?

share|improve this question

closed as off-topic by 200_success Aug 12 at 2:51

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

  • "Questions must involve real code that you own or maintain. Questions seeking an explanation of someone else's code are off-topic. Pseudocode, hypothetical code, or stub code should be replaced by a concrete example." – 200_success
If this question can be reworded to fit the rules in the help center, please edit the question.

    
There's a giant # do something else placeholder — could you fill that in? Otherwise, this is a purely generic question, which would be off-topic for Code Review. –  200_success Aug 12 at 2:51
    
@200_success thanks for your comment. It may seem that huge part should be the one, but I only interest in two lines above, since it appears a lot in my code. And naming that meaningless variable name seem to my problem, and if I can resolve these 2 lines, it will help me a lot in my workflow. –  nXqd Aug 12 at 3:51

2 Answers 2

Naming temporary variable is troublesome because [...] sometimes it costs time to read and understand a large block of code meanwhile it doesn't give any meaning.

Well, then give it some meaning. The name value doesn't describe much of anything. Instead, you could do something like

def some_method
  new_click = TopicLinkClick.create_from(new_params)
  return new_click unless new_click.nil?
  # do something else
end

You can shorten it a bit, but I wouldn't recommend it, as it's not quite as clear in my opinion

def some_method
  return new_click if new_click = TopicLinkClick.create_from(new_params)
  # do something else
end

Your other options are of course to not return early, but use a good old conditional instead

def some_method
  new_click = TopicLinkClick.create_from(new_params)
  unless new_click
    # do something else
  end
end

Alternatively, if you've got your code factored really nicely into methods, you can do

def some_method
  TopicLinkClick.create_from(new_params) || some_other_method(...)
end

Or, if you need a bit more logic, you can use a begin..end block, but again it's not as nice in my opinion.

def some_method
  TopicLinkClick.create_from(new_params) || begin
    # do something else
  end
end

Overall, though, your current approach is fine. Just use more descriptive names.

If you have a method that's simply a shortcut for calling either method A or method B, then I'd recommend using the method_a || method_b trick. Otherwise, like @schism, I'd recommend against doing too much on one line (as in code blocks 2 and 5)

share|improve this answer

Personally I don't think having one extra line per early return is much clutter. It doesn't seem like it'd take long to understand either.

That said, if you really wanted to, you could reduce it to one line. You'll still have to assign a variable though.

if !(value = TopicLinkClick.create_from(new_params)).nil? then return value
share|improve this answer
3  
Btw, congrats on the 2K! –  syb0rg Aug 12 at 1:14

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