Improve company productivity with a Business Account.Sign Up

x
?
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
Medium Priority
?
247 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 82

Accepted Solution

by:
arnold earned 2000 total points
ID: 41802619
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 38

Expert Comment

by:Gerwin Jansen, EE MVE
ID: 41803315
The code is certainly not ugly, it's pretty structured.

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

Expert Comment

by:serialband
ID: 41803320
Just because someone may be able to write a one liner, doesn't mean it will be clearer.
0
Free Tool: Subnet Calculator

The subnet calculator helps you design networks by taking an IP address and network mask and returning information such as network, broadcast address, and host range.

One of a set of tools we're offering as a way of saying thank you for being a part of the community.

 
LVL 1

Author Comment

by:MostHated
ID: 41803332
@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
 
LVL 82

Assisted Solution

by:arnold
arnold earned 2000 total points
ID: 41803352
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
ID: 41803444
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 12

Expert Comment

by:tel2
ID: 41804035
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
ID: 41804877
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 12

Expert Comment

by:tel2
ID: 41808525
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

Free Tool: IP Lookup

Get more info about an IP address or domain name, such as organization, abuse contacts and geolocation.

One of a set of tools we are providing to everyone as a way of saying thank you for being a part of the community.

Question has a verified solution.

Are you are experiencing a similar issue? Get a personalized answer when you ask a related question.

Have a better answer? Share it in a comment.

Join & Write a Comment

The Windows functions GetTickCount and timeGetTime retrieve the number of milliseconds since the system was started. However, the value is stored in a DWORD, which means that it wraps around to zero every 49.7 days. This article shows how to solve t…
This installment of Make It Better gives Media Temple customers the latest news, plugins, and tutorials to make their Grid shared hosting experience that much smoother.
Learn how to match and substitute tagged data using PHP regular expressions. Demonstrated on Windows 7, but also applies to other operating systems. Demonstrated technique applies to PHP (all versions) and Firefox, but very similar techniques will w…
In a previous video, we went over how to export a DynamoDB table into Amazon S3.  In this video, we show how to load the export from S3 into a DynamoDB table.

584 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