Solved

How can I clean up this bash code? It is extremely ugly and I know I am doing it in probably the worst possible way.

Posted on 2016-09-16
9
86 Views
Last Modified: 2016-09-21
Hello all,
    I am newish to bash and I have been researching and looking at hundreds of examples for a large personal project (especially thanks to my help from folks on here), and I have most of the code pretty trimmed down and clean, using a file for config, another for functions, etc, but not quite sure what to do with what I have below. I know bash has ways of doing multiple things in a single line, but I just do not know enough about it to do it. The code works just fine, I get all my output to what I need, just if someone could show me some quick examples of how I can take this code and clean it up, I would greatly appreciate it!
# Get user input for needed variables
function getuserinput(){
read -p "Please specify the new username: " NEWUSER
	if test "$NEWUSER" = ""; then
        echo "$0: sorry, username cannot be blank" >&2
        exit 1;
fi
	if test $? -eq 0; then
        echo 'Username set' >&2
	else
        echo 'Change attempt failed' >&2
        exit 1
fi
read -s -p "Please specify the new password: " NEWPASS
echo ""
	if test "$NEWPASS" = ""; then
        echo "$0: sorry, password cannot be blank" >&2
        exit 1;
fi
	if test $? -eq 0; then
        echo 'password set' >&2
	else
        echo 'change attempt failed' >&2
        exit 1
fi
read -p "Please specify your domain: " NEWDOMAIN
	if test "$NEWDOMAIN" = ""; then
        echo "$0: sorry, domain cannot be blank" >&2
        exit 1;
fi
	if test $? -eq 0; then
        echo 'domain set' >&2
	else
        echo 'change attempt failed' >&2
        exit 1
fi
read -p "Please specify your email address: " NEWEMAIL
	if test "$NEWEMAIL" = ""; then
        echo "$0: sorry, email cannot be blank" >&2
        exit 1;
fi
	if test $? -eq 0; then
        echo 'email set' >&2
	else
        echo 'change attempt failed' >&2
        exit 1
fi
sed -i -e"s/^user=.*/user=$NEWUSER/" ~/store_installer/lib/config.cfg
sed -i -e"s/^pass=.*/pass=$NEWPASS/" ~/store_installer/lib/config.cfg
sed -i -e"s/^domain=.*/domain=$NEWDOMAIN/" ~/store_installer/lib/config.cfg
sed -i -e"s/^email=.*/email=$NEWEMAIL/" ~/store_installer/lib/config.cfg
echo "Updated configuration options" >&2
~/store_installer/lib/singleinstall.cfg
}

Open in new window

0
Comment
Question by:MostHated
  • 3
  • 2
  • 2
  • +2
9 Comments
 
LVL 76

Accepted Solution

by:
arnold earned 500 total points
Comment Utility
To what end?
To "clean" things up often one analyzes what needs to be done and improve the logic/flow of the program.

I.e. Should the current step 4 function better if it was done as step 2 or combined with step 1.

M4 I think /use of a template
With data about a user/info in a db
Would be ... If you have to do this frequently.
The process adds/removes user to db. Then calls a process that has the base template of each of the files you referenced with each  user readied .....
0
 
LVL 37

Expert Comment

by:Gerwin Jansen
Comment Utility
The code is certainly not ugly, it's pretty structured.

If you just want less 'text' then you'll lose on readability.
0
 
LVL 27

Expert Comment

by:serialband
Comment Utility
Just because someone may be able to write a one liner, doesn't mean it will be clearer.
0
 
LVL 1

Author Comment

by:MostHated
Comment Utility
@arnold I suppose I should have thought about that. That is a great idea. The plan is to *hopefully* use this a lot (this is just a small part of my overall installer script). My pals and I are working on a store project that we are hoping to offer to customers one day. The code is not really meant to be readable by an end user, the installation script is just to help automate things for us, as we will be hosting it. So it is more for deployment purposes.

Having those things write to a DB would be a much better idea than trying to maintain it on individual VM's. I will have to see what bash's DB writing capabilities are, if there are any, if not I am sure I can integrate another script / language to handle that part of it.

That being said, I suppose I should clarify what I meant by "clean it up", I was just hoping to simplify it from a programmatic point of view. I feel that the overall functionality could be reduced to just a few lines of code, instead of the way I have done it. I would rather have it that way if I can figure out how to do it.  It is not so much about the look or formatting, but more getting more things done per line of code instead of having an entire code block to do the same thing.
0
Highfive + Dolby Voice = No More Audio Complaints!

Poor audio quality is one of the top reasons people don’t use video conferencing. Get the crispest, clearest audio powered by Dolby Voice in every meeting. Highfive and Dolby Voice deliver the best video conferencing and audio experience for every meeting and every room.

 
LVL 76

Assisted Solution

by:arnold
arnold earned 500 total points
Comment Utility
The simplification if no user interaction, an option is to convert the script to getting data from running the command without interactive prompts.
Command "newuser" "password" "newdomain" "email address"
Your existing would check that you received 4 arguments.
You then have one function, that will ask user for the missing item.

If -z "$1"; then
            Newuser=Getinput "Newuser"
   Else
         Newuser=$1
Fi

The simplification dealing with getting a value response for each needed data point without exiting if the provided info is not correct.

........

Text related config files, perl, Python script has better interaction to databases.

Bash to MySQL or PostgreSQL
Response =$(Echo "select * from database where username=$newuser" | MySQL -u --password=password databasename )

..............
0
 
LVL 1

Author Comment

by:MostHated
Comment Utility
There will be a local mongodb for parse-server, the store, etc, but then we can / should make a central server for user account information. I should make a web interface that will interact with mongodb for central management (Assuming bash can interact with mongodb? Probably no reason that it cannot if mongodb accepts command line things, which I expect it does, I will research that) so that I can then create accounts from the web interface, store them in the DB and then just populate the data automatically based on just an account number, or perhaps the hostname of the VM could be the domain name of the client upon creation of it, and then pull based on that, which would make for even less interaction necessary.

There would really then be no reason for all this code, I would only need to then pull info from the DB, and then use sed to input it in to the store / local parse-server configs. That would certainly help minimize things. Thanks for the suggestions and helping stimulate some good ideas!
0
 
LVL 11

Expert Comment

by:tel2
Comment Utility
Hi again MH,

A few suggestions on the micro level if you want to continue with bash...

Intead of this style of indentation:
read -p "Please specify the new username: " NEWUSER
      if test "$NEWUSER" = ""; then
        echo "$0: sorry, username cannot be blank" >&2
        exit 1;
fi
I'd do this:
read -p "Please specify the new username: " NEWUSER
if test "$NEWUSER" = ""; then
        echo "$0: sorry, username cannot be blank" >&2
        exit 1;
fi

Because the "if..." is not inside the "read", and the "fi" should be at the same level as the "if".

And instead of this:
echo "$0: sorry, username cannot be blank" >&2
if you want to make a bell sound, you could do this:
echo -e "$0: sorry, username cannot be blank\07" >&2

And instead of this:
sed -i -e"s/^user=.*/user=$NEWUSER/" ~/store_installer/lib/config.cfg
sed -i -e"s/^pass=.*/pass=$NEWPASS/" ~/store_installer/lib/config.cfg
sed -i -e"s/^domain=.*/domain=$NEWDOMAIN/" ~/store_installer/lib/config.cfg
sed -i -e"s/^email=.*/email=$NEWEMAIL/" ~/store_installer/lib/config.cfg
Traditionally people put the following line near the top of the script, so there's one place to update file references if they ever need to change.
CONFIGFILE="~/store_installer/lib/config.cfg"
Then later...
sed -i "s/^user=.*/user=$NEWUSER/" $CONFIGFILE
sed -i "s/^pass=.*/pass=$NEWPASS/" $CONFIGFILE
sed -i "s/^domain=.*/domain=$NEWDOMAIN/" $CONFIGFILE
sed -i "s/^email=.*/email=$NEWEMAIL/" $CONFIGFILE
# Note the '-e' switch is not required.

Or if you prefer a one-liner...
sed -i "s/^user=.*/user=$NEWUSER/; s/^pass=.*/pass=$NEWPASS/; s/^domain=.*/domain=$NEWDOMAIN/; s/^email=.*/email=$NEWEMAIL/" $CONFIGFILE

But if those are the only 4 things in the config.cfg file, then you don't need 'sed' at all.  Just write to the file with '>' (for the 1st item) and '>>' (for the rest).
1
 
LVL 1

Author Comment

by:MostHated
Comment Utility
Thank you yet again @tel2 for the great reply. I had indeed done some of the things you suggested in other parts of my code (such as using a variable for the path), not sure why it slipped my mind doing it during this code segment.

Also, thank you for the last bit about consolidating to a single line of code, that was exactly what I was hoping to get out of this post. As for what you said about the config file, I believe I am going to go another route as per my post just before yours, and instead pull the information from a DB instead of taking it during the beginning of the install. This will allow for more automation as well when dealing with the digitalocean "dropets" (VM's), and I can then utilize their API to spin up a new VM and pass the information from the DB directly to it and theoretically need no interaction from myself or my colleagues. If we put a payment gateway / account creation process in front of things, we could technically allow for a new customer to create their own account, start up their own store, and manage everything them self.

The info you provided will definitely help me condense my code, as well as format it a bit better. As I mentioned in the previous thread that we were working in, I definitely am no professional programmer, so all the input is definitely appreciated. After I initially created this post I had came across a nice way to create parse session keys, and had used the variable method of declaring the paths to things as seen below.

installdir=/home/store/

function configapps(){
appstring=$(date +%s | sha256sum | base64 | head -c 12 ;)
sed -i "s|{ session_key }|$appstring|g" $installdir/store-admin/app/config.js
sed -i "s|{ parse_url }|$newdomain/api|g" $installdir/store-admin/app/config.js
}

Open in new window

Again, thanks for all the help and input you have provided!
0
 
LVL 11

Expert Comment

by:tel2
Comment Utility
Hi MH,

This:
appstring=$(date +%s | sha256sum | base64 | head -c 12 ;)
can be shortened to this:
appstring=$(date +%s | sha256sum | base64 | head -c 12)
because the ' ;' does nothing here.
0

Featured Post

Threat Intelligence Starter Resources

Integrating threat intelligence can be challenging, and not all companies are ready. These resources can help you build awareness and prepare for defense.

Join & Write a Comment

Suggested Solutions

Title # Comments Views Activity
Need help editing script 3 50
Parse DNS log 3 33
RoboCopy to Changing External Drives 2 28
bash script question (chmod) 10 40
It’s 2016. Password authentication should be dead — or at least close to dying. But, unfortunately, it has not traversed Quagga stage yet. Using password authentication is like laundering hotel guest linens with a washboard — it’s Passé.
Active Directory replication delay is the cause to many problems.  Here is a super easy script to force Active Directory replication to all sites with by using an elevated PowerShell command prompt, and a tool to verify your changes.
The viewer will learn how to dynamically set the form action using jQuery.
The viewer will learn how to look for a specific file type in a local or remote server directory using PHP.

771 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

11 Experts available now in Live!

Get 1:1 Help Now