4
\$\begingroup\$

I wrote my first ruby script, by this tutorial. It creates a local and remote git repo.

I have been working for about 3 years with PHP, and somehow I find that my ruby script looks incredibly ugly.

  • Is it correct to use a class for that?
  • What can I make better?
  • Are there any security gaps in it?
  • Is the usage in for true and false in the empty_name? and folder_exists? methods correct?
#!/usr/bin/env ruby

require 'fileutils'
#require 'colorize'

class Repo

  LOCAL_PATH = 'C:\xampp\htdocs'
  REMOTE_PATH = 'C:\Users\Windows\Desktop\Dropbox\repos'
  UPSTREAM_NAME = 'dropbox'

  def initialize
      print 'Name: '
      @name = gets.chomp

      if ! folder_exists?

          create_local_repo
          create_files_and_commit
          create_remote_repo

      end
  end

  private

  # Check if name is emtpy.
  def empty_name?
      if @name.empty?
          print "You need to pass a name: "
          @name = gets.chomp
          empty_name?

          true
      end

      false
  end

  # Check if folder exists on local and remote path.
  def folder_exists?

      empty_name?

      if File.exists? "#{LOCAL_PATH}/#{@name}"
          print "Directory `#{@name}` exists in #{LOCAL_PATH}. Pass a new name: "
          @name = gets.chomp

          folder_exists?
      elsif File.exists? "#{REMOTE_PATH}/#{@name}.git"
          print "Directory `#{@name}.git` exists in #{REMOTE_PATH}. Pass a new name: "
          @name = gets.chomp

          folder_exists?
      end

      false
  end

  # Create local folder and initialize git.
  def create_local_repo
      Dir.chdir "#{LOCAL_PATH}" do `git init #{@name}` end
  end

  # Create .gitignore and commit.
  def create_files_and_commit
      Dir.chdir "#{LOCAL_PATH}/#{@name}" do
          File.open ".gitignore", 'w+' do |f| 
              f.write ".idea\n/vendor\n/node_modules\n.Thumbs.db\n.DS_Store"
          end

          `git add .`
          `git commit -m "initialize commit"`
      end
  end

  # Create empty remote repo.
  def create_remote_repo
      `git init --bare #{REMOTE_PATH}/#{@name}.git`

      Dir.chdir "#{LOCAL_PATH}/#{@name}" do
          `git remote add #{UPSTREAM_NAME} #{REMOTE_PATH}/#{@name}.git`
          `git push -u #{UPSTREAM_NAME} master`
      end
  end

end

Repo.new
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

The structure is kinda confusing. empty_name? does a lot more than just tell you if the name's empty, and so does folder_exists?. Meanwhile, the constructor is the first place you ask for a name - but it's not checked there. create_remote_repo doesn't just create a repo, it also sets it as upstream for the local one. Which, in turn, means that create_local_repo must be called before create_remote_repo, but that isn't made explicit in the code.

In all, you methods just seem to be doing more, or doing something different than what they claim.

There's also a straight-up bug: empty_name? will never return true. If the string's empty it'll just call itself repeatedly until you finally give in and enter a string. At that point, it'll return all the way back through the stack, and... return false in the end. The true line does nothing by itself.
folder_exists? does something similar; it'll also always return false.

You class is also called Repo, though it doesn't actually model a repository. A name like RepoGenerator would be more accurate, though I'm on the fence about whether a class is even necessary in this case. You basically just call Repo.new and it does everything and the script exits; there's little need to keep state.

I'd also avoid the hardcoded file path constants. They make the script unusable for anyone but you - and perhaps even for yourself if you switch computer.

However, regardless of implementation, I find the function of this script somewhat more suspicious.

Your remote repo isn't actually "remote". It's just as local as your other one, it just happens to get copied by Dropbox. And since it's copied from computer to computer there's no guarantee it's in sync. If you have 2 computers, you basically end up with 2 "remote" repos, and 2 local ones.

Moreover, you don't want Dropbox messing with git's own files. If something gets corrupted by Dropbox, your "remote" repo may break. Or worse: It breaks silently. You then pull from that repo, and now you've broken your local repo too.

You might as well simply place your "local" repo in Dropbox, and call it a day. If you want a proper remote repo, use GitHub, Bitbucket, or Beanstalk - or set up your own server.

Secondly, you could just set up a ~/.gitignore_global file on your computer(s) to make sure files like .DS_Store etc. are always ignored. No need for a local .gitignore in every repo that just lists the same files every time.

If you do want to list some files, use a heredoc:

ignore_list = <<-LIST
# System files
.Thumbs.db
.DS_Store

# IDE files
.idea

# Generated/downloaded files
/vendor
/node_modules

LIST

Other stuff:

  • You don't handle special characters (e.g. whitespace) in your paths. A name like "my awesome repo" would probably pass all your checks, but fail when it has to actually create a repo. Take a look at the Shellwords module and do something like: %x{ git init #{name.shellescape} }

  • In fact, if a command fails, your script will probably just keep going. For instance, let's say that git init fails: Your script will happily continue with git add . (which fails) and git commit (which fails too). Kinda pointless.
    In other words, if you run a shell command, you'll probably want to check its return value (e.g. $?.success? - $? gets set when you execute a shell command). You can also consider combining a few commands and let the shell handle things, e.g. git add . && git commit -m 'Initial commit'

  • In git parlance, the first commit is the initial commit. Not "initialize commit" - in fact, that doesn't really make much sense.

To sum up, and to answer your questions directly:

  1. Is it correct to use a class for that?

You can, although it's probably not called for in this case. Your class, as it is right now, is just a collection of methods. You can't really use a Repo instance for anything. A module would be fit the bill better, though for a simple script like this, you might as well just write some methods.

  1. What can I make better?

Well, see above :)
Basically, refactor it so your methods only do what they say, and only say what they do; don't repeat yourself; etc. etc..

  1. Are there any security gaps in it?

"Security gaps" is debatable. I doubt this script will somehow get you hacked, or something. There are safety gaps though, such as not escaping strings before using them in commands, and not checking that previous commands actually worked.
And there's the idea of have two local repos, where one just happens to get copied by Dropbox, which is sort of odd.

  1. Is the usage in for true and false in the empty_name? and folder_exists? methods correct?

Well, no, not quite. As mentioned, those methods can't actually return anything but false every time. ?-methods are basically yes/no questions, but yours do a bunch of stuff - seemingly to avoid answering "yes". Just make them "honest", and just have them say true or false.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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