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