1
\$\begingroup\$

I have a method that looks like the method below, and I can't help but shake the feeling that it can be improved. It doesn't really "read" well, and it seems like checkout could be called only once, somehow.

def ensure_correctness
  if exist?
    unless correct_version?
      delete!
      checkout
    end
  else
    checkout
  end
end

def checkout
  run_command(['git', 'clone', '--depth', '1', url, path])
end

def exist?
  Dir.exist?(path)
end

def delete!
  FileUtils.rm_rf(path)
end

def correct_version?
  commit_sha_short == desired_commit_sha_short
end

The best I could come up with is moving the code from failing correct_version? to a new method (which probably isn't a terrible idea, but I wonder if I can do better).

I feel like I'm missing something really obvious here...

\$\endgroup\$
5
  • 1
    \$\begingroup\$ What happened to operation_1? I agree with the above comment though, the more context you can add the better, really. \$\endgroup\$ Dec 1 '17 at 20:08
  • \$\begingroup\$ @SimonForsberg operation_1 was purely for notification, I'm not sure it was terribly useful, so I eliminated it to hopefully make the question easier to understand. Thanks for your help. \$\endgroup\$
    – Brad Werth
    Dec 1 '17 at 20:10
  • \$\begingroup\$ You have two conditions and two (or three including the removed notification) actions. Make a truth-table and see which actions should run when, and then you might easier see what the best way to structure your logic is. \$\endgroup\$ Dec 1 '17 at 20:12
  • \$\begingroup\$ Just to be clear: My suggestion about a truth-table was a suggestion for the future for similar problems, not meant as a direct answer to your question. If you undelete your question it will be reopened now. Thanks for listening to our feedback about your question. \$\endgroup\$ Dec 1 '17 at 20:22
  • \$\begingroup\$ @SimonForsberg Holy cow! It turns out it did pay to have the real methods in place for an ideal solution. It would still be nice if there were a generic solution to the originally posted problem, it seems to me that there is a more elegant way to express that logic lurking just below the surface... Anyway, thanks to you and 200_success for pushing me to improve the question as I think it ended up resulting in a better solution. Have a great weekend! \$\endgroup\$
    – Brad Werth
    Dec 2 '17 at 0:26
2
\$\begingroup\$

The two calls to checkout look well to me. They are simple method calls to self with no arguments, and I can easily see that both calls are the same. You avoid making two identical calls to run_command(['git', 'clone', ...]).

The "something really obvious" might be a return:

def ensure_correctness
  if exist?
    return if correct_version?
    delete!
  end
  checkout
end
\$\endgroup\$
3
  • \$\begingroup\$ Yes! Thank you for addressing the more abstract problem! I believe you are correct about the "something really obvious", but I wonder if there is a way to achieve the same thing in a different arrangement without the return... If I can't think of any, I'm going to mark this as accepted, as it most closely satisfies my original curiosity. \$\endgroup\$
    – Brad Werth
    Dec 2 '17 at 0:51
  • \$\begingroup\$ Just out of curiosity, which of the three solutions do you consider to be more idiomatic/readable? \$\endgroup\$
    – Brad Werth
    Dec 2 '17 at 0:53
  • 1
    \$\begingroup\$ Your correct_version? || delete! && checkout confused me because of operator precedence. I first thought of Unix shell (sh), where || and && have same precedence. Then I remembered that Ruby does && before ||. The code might be more readable to people who don't know sh. \$\endgroup\$ Dec 2 '17 at 1:12
1
\$\begingroup\$

While I envisioned just moving the instructions in ensure_correctness around a little, to find an efficient solution to a pattern a see repeated with some frequency, it turns out it was better to rearrange everything a little. I think the final solution is more attractive, even if it does potentially call delete! unnecessarily (it doesn't matter).

def ensure_correctness
  unless correct_version?
    delete!
    checkout
  end
end

def checkout
  run_command(['git', 'clone', '--depth', '1', url, path])
end

def exist?
  Dir.exist?(path)
end

def delete!
  FileUtils.rm_rf(path)
end

def correct_version?
  exist? && commit_sha_short == desired_commit_sha_short
end
\$\endgroup\$
4
  • \$\begingroup\$ I think ultimately this solution works better for the application than the accepted answer, both in terms of readability and robustness. The adjustment has the side-effect of making the correct_version method technically more correct, which could be useful if consumed from elsewhere. I really appreciate everyone that took a look at this question - thanks! \$\endgroup\$
    – Brad Werth
    Dec 2 '17 at 0:51
  • \$\begingroup\$ Personally I would prefer parenthesis around delete! && checkout, but that might just be because I am not used to Ruby's operator precedence and that I prefer to avoid combining && and || on the same line. \$\endgroup\$ Dec 2 '17 at 1:22
  • \$\begingroup\$ That "less confusing" way is wrong, should use unless instead of if. And you rely on FileUtils.rm_rf returning something "true" but the documentation doesn't specify any return value. Is that safe? (Also, if someone changed delete! so it returns something "false", that would break it as well. And since delete! isn't documented at all, one might feel free to make such a change, not knowing that it breaks something.) \$\endgroup\$ Dec 3 '17 at 12:16
  • \$\begingroup\$ @StefanPochmann, you're right, good catch! Updated answer, thanks. \$\endgroup\$
    – Brad Werth
    Dec 3 '17 at 15:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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