Ensure that a directory reflects a specified Git commit

Question

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...

• Simon Forsberg: What happened to operation_1? I agree with the above comment though, the more context you can add the better, really.
• Brad Werth: @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.
• Simon Forsberg: 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.
• Simon Forsberg: 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.
• Brad Werth: @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!

Answer 1

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

• Brad Werth: 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.
• Brad Werth: Just out of curiosity, which of the three solutions do you consider to be more idiomatic/readable?
• George Koehler: 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.

Answer 2

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

• Brad Werth: 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!
• Simon Forsberg: 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.
• Stefan Pochmann: 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.)
• Brad Werth: @StefanPochmann, you're right, good catch! Updated answer, thanks.