Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I'm looking for feedback on improving the structure of this code. Also, I could not find the command line executable for Chrome.

General feedback is also requested.

#
#
#
# Divider - configures bash, git, grunt, sublime and chrome
#
#
#


user1=foo

config_bash() {
    rm ~/.bash_profile
    ln -s ~/root/config/bash/login.sh ~/.bash_profile
    source ~/.bash_profile
    echo "Bash configured."
}
config_git() {
    local a="$1"
    if [ $# -eq 0 ]
    then
        a="default"
    fi
    if [ $# -eq 1 ]
    then
        git config --global user.name "$a"
        git config --global user.email "$a@$a.com"    
    fi
    git config --global push.default matching
    git config --global core.editor "subl"

    git remote add godaddy $user1@foo.com:~/root.git
    git remote add heroku https://git.heroku.com/frozen-dusk-2587.git
    echo "Git configured."   
}
config_grunt() {
    sudo npm install -g grunt-cli
    mkdir ~/root_install/grunt
    ln -s ~/root/config/grunt/package.json ~/root_install/grunt/package.json
    ln -s ~/root/config/grunt/Gruntfile.js ~/root_install/grunt/Gruntfile.js
    cd ~/root_install/grunt
    npm install
    cd ~/root
    echo "Grunt configured."
} 
config_sublime_2() {
    ln -s /Applications/Sublime\ Text\ 2.app/Contents/SharedSupport/bin/subl /usr/local/bin/subl
    rm  -rf ~/Library/Application\ Support/Sublime\ Text\ 2/Packages/User
    ln -s ~/root/config/sublime ~/Library/Application\ Support/Sublime\ Text\ 2/Packages/User
    echo "Sublime configured."
}
reset_sublime_2() {
    rm -rf ~/Library/Application\ Support/Sublime\ Text\ 2
}
config_chrome(){
    chmod 0444 ~/Library/Application\ Support/Google/Chrome/Default/History
    echo "Chrome configured."
}
config_all() {
    config_bash
    config_git
    config_grunt
    config_sublime_2
    config_chrome
    list
}
list(){
    local a=$(which bash) b=$(which git) c=$(which grunt) d=$(which subl)
    local g=$(which node)  i=$(which heroku)
    echo "****"    
    echo "Your bash executble is here: $a."    
    echo "Your git executble is here: $b."    
    echo "Your grunt executble is here: $c."
    echo "Your sublime executble is here: $d."
    echo "Your node executble is here: $g."
    echo "Your heroku executble is here: $i."
    echo "****"
}
share|improve this question
1  
I rolled back to the version before your refactoring. It is our policy to not edit the code after answers were received, as they may be invalidated by the changes. You may post your updated code as a new question to get further reviews. – janos Mar 9 at 19:01
    
1  
@janos' comment still applies, please don't update your question with updated code. – Quill Mar 10 at 22:30
    
Her is the update per code 1.2.1 section 8 subsection alpha - codereview.stackexchange.com/questions/122605/… – cade galt 0 Mar 11 at 21:33
up vote 12 down vote accepted

Simplify

Since $# cannot be both 0 and 1 at the same time, it would be better to join the two if conditions here:

local a="$1"
if [ $# -eq 0 ]
then
    a="default"
fi
if [ $# -eq 1 ]
then
    git config --global user.name "$a"
    git config --global user.email "$a@$a.com"    
fi

But, actually, what if there are 2 or more parameters instead of 0 and 1? It can happen by a programming mistake. It would be more robust to rewrite this by checking the values of the parameters, rather than their total count:

if [ "$1" ]
then
    git config --global user.name "$1"
    git config --global user.email "$1@$1.com"    
fi

Notice that I eliminated the a local variable, as it was pointless (a="default" set but never actually used).

Avoid changing directories in scripts

This is a bit fragile:

cd ~/root_install/grunt
npm install
cd ~/root

A cd command may fail, for example if the directory was not yet created. In general it's not a good idea to change directories inside a script, as the script might perform actions in unexpected directories when things go wrong.

A good solution in such situations is to wrap commands within (...), like this:

(cd ~/root_install/grunt; npm install)

Directory changes executed inside (...) will not affect the environment outside.

You can learn more about this syntax in the Compound Commands section in man bash:

   (list) list  is  executed in a subshell environment (see COMMAND EXECU-
          TION ENVIRONMENT below).  Variable assignments and builtin  com-
          mands  that  affect  the  shell's  environment  do not remain in
          effect after the command completes.  The return  status  is  the
          exit status of list.

Robustness

Note that if you re-run the script a 2nd time, some commands will fail, for example these:

mkdir ~/root_install/grunt
ln -s ~/root/config/grunt/package.json ~/root_install/grunt/package.json
ln -s ~/root/config/grunt/Gruntfile.js ~/root_install/grunt/Gruntfile.js

If the directory and the symlinks already exist, the script will print error messages for these lines. You can avoid that by adding some flags:

mkdir -p ~/root_install/grunt
ln -sf ~/root/config/grunt/package.json ~/root_install/grunt/package.json
ln -sf ~/root/config/grunt/Gruntfile.js ~/root_install/grunt/Gruntfile.js

Naming

These variable names are not very helpful:

local a=$(which bash) b=$(which git) c=$(which grunt) d=$(which subl)
local g=$(which node)  i=$(which heroku)

Better name these after the commands they represent:

local bash=$(which bash) git=$(which git)  # and so on ...
share|improve this answer
    
As I mentioned above, after you assign a="default", the variable is never used again. It's a pointless assignment – janos Mar 9 at 5:46
    
First a is assigned to $1. If there are no arguments, then a is assigned to default, but then a is not used again. If there is one argument, then a is used by git config commands. Like I said, the value default never gets used. – janos Mar 9 at 19:04
    
that was a logic error I see thanks for pointing it out, but for correct working code I need 3 cases ( 0, 1, and more than 1 ) and a variable. – cade galt 0 Mar 9 at 19:34
4  
@cadegalt0 nobody on this site gets paid to participate. – Phrancis Mar 10 at 22:50
1  
Votes are notoriously difficult to predict. Just posting better code doesn't guarantee more upvotes. If you want to get more upvotes, read the how-to-ask page in the help center very carefully, and also look at other successful posts in the relevant tags. In general, what seems to gather more upvotes is serious hard work on the quality of a post. – janos Mar 11 at 21:41

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.