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.

As a beginner in Ruby, I am trying to optimize this script that is a client side pre-commit hook for git.

Was hoping I could get a review to ensure I am following all ruby idioms, and to ensure I have the most elegant solution.

#!/usr/bin/env ruby
#
# Pre-commit hook created to help ensure dns changes are being 
# committed to the correct branch.
#

# Hash of zone files to look for when a commit is attempted.
zone_files = { 
  development: ‘some_zone_file1’,
  prod: ‘some_zone_file2’
}

# Execute git diff to determine files includes as part of commit.
out_data = %x{git diff --cached --name-only}
pri_branch = "master"

# Iterate through the zone files, and check if any are found in the output. 
# Only one needs to be found. No need to find all.
zone_files.keys.each do |key| 
  if out_data.include?zone_files[key]
    current_branch = %x{git branch}.match(/\* (.+?)\n/m)
    if current_branch[1].include?pri_branch
      break
    else
      puts "Warning: You have triggered a git pre-commit hook."
      puts "Warning: Your current branch is: #{current_branch[1]}."
      puts "Warning: For dns, ensure you commit on branch: #{pri_branch}"
      puts "Warning: Include '-n' to bypass this check."
      puts "Exiting..."
      exit 1
    end
  end
end
share|improve this question

3 Answers 3

up vote 8 down vote accepted
  1. Use fewer abbreviations. It's certainly not bad in this case, but small stuff like production instead of prod (unless perhaps it's a naming convention for your production environment/server), and primary_branch (I guess?) instead of pri_branch. Or, at any rate, be consistent: You have development spelled out, but you shorten production?

  2. Use parenthenses or at the very least spaces in method calls - and include? is just a method call. So this expression out_data.include?zone_files[key] looks weird right now.

  3. When iterating a hash, the block will be passed the elements as [key, value] arrays. If you add a bit of array destructuring, you get key and value separately.

    zone_files.each do |environment, file|
      # ...
    end
    
  4. But zone_files doesn't actually need to be a hash. Of course you can use one, and it may be worth it to label to two zone files. But it could just be an array. And you might want to make it constant while you're at it.

  5. Get the current branch once. Right now it's being parsed for each of the zone files, since it's inside the loop.

  6. And check the branch once - if it's the pri_branch then you can skip everything else.

  7. You can use a heredoc for the warning

Here's my take

# Array of zone files to look for when a commit is attempted.
ZONE_FILES = %w(some_zone_file1 some_zone_file2).freeze

# The branch we're allowed to commit zone files on
PRIMARY_BRANCH = "master".freeze

# get the current branch
current_branch = %x{git branch}.match(/\* (.+?)\n/m)[1]

# stop here if we're committing to the primary branch
exit(0) if current_branch == PRIMARY_BRANCH

# execute git diff to determine files includes as part of commit.
commit_files = %x{git diff --cached --name-only}

# find out if any of the zone files are among the files
# being committed
committing_zone_file = ZONE_FILES.detect { |zone_file| commit_files.include?(zone_file) }

# and if we're committing a zone file, complain to the user
if committing_zone_file
  puts <<-EOT
Error: You have triggered a git pre-commit hook.
Your current branch is: #{current_branch}.
For DNS, ensure you commit on branch: #{PRIMARY_BRANCH}
Include '-n' to bypass this check.
Exiting...
  EOT

  exit(1)
end
share|improve this answer
    
As always, good advice. The only thing I am not sure about is the profusion of comments: good code should be declarative, and declarative code needs nearly no comments (maybe some comments about the "why", few about the "what"/"how"). –  tokland Jul 9 at 18:50
    
@tokland Oh, I agree. I chose to add a bunch of comments in part because OP claims to be a "beginner in Ruby", so I figured the verbosity might be appreciated. Also, some comments are just holdovers from the original code. By the way, apologies for repeating some of your advice; didn't see your answer until I'd typed most of mine :P –  Flambino Jul 9 at 19:00
    
Thanks so much for taking the time to write this out. I appreciate it. –  fr00z Jul 9 at 19:15
    
Flambino, no problem about repeating advice, I've always thought answers should be complete. –  tokland Jul 9 at 20:12

Some notes:

  • zone_files.keys.each and then zone_files[key]. You should iterate the pair: zone_files.each do |key, zone|

  • current_branch = %x{git branch}.match(/\* (.+?)\n/m). Isn't this inmutable? then better move it before the loop.

  • current_branch[1].include?pri_branch. That's pretty unreadable, put those parentheses: current_branch[1].include?(pri_branch)

Otherwise, it looks pretty good to me.

share|improve this answer
    
Great tidbits. Thank you so much for responding. –  fr00z Jul 9 at 19:14

For readability, I suggest using parentheses when calling .include?(…).

Saying "Warning… Warning… Warning" on every line is a bit excessive, I think. A warning that prevents an operation from completing is not a warning — it's an error. The last line of the message would be more clearly phrased as "Run 'git commit -n' to bypass this check."

In general, though, I think the script looks fine.

share|improve this answer
    
Thank you for your feedback. –  fr00z Jul 9 at 19:15
    
I'm definitely going to use your message.. Much more clear as you stated.. thank you! –  fr00z Jul 9 at 19:27

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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