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
171 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
[X]
Welcome to Experts Exchange

Add your voice to the tech community where 5M+ people just like you are talking about what matters.

  • Help others & share knowledge
  • Earn cash & points
  • Learn & ask questions
  • 3
  • 2
  • 2
  • +2
9 Comments
 
LVL 79

Accepted Solution

by:
arnold earned 500 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 30

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
VIDEO: THE CONCERTO CLOUD FOR HEALTHCARE

Modern healthcare requires a modern cloud. View this brief video to understand how the Concerto Cloud for Healthcare can help your organization.

 
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 79

Assisted Solution

by:arnold
arnold earned 500 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: 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.

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

Every server (virtual or physical) needs a console: and the console can be provided through hardware directly connected, software for remote connections, local connections, through a KVM, etc. This document explains the different types of consol…
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…
In this fourth video of the Xpdf series, we discuss and demonstrate the PDFinfo utility, which retrieves the contents of a PDF's Info Dictionary, as well as some other information, including the page count. We show how to isolate the page count in a…
Video by: Mark
This lesson goes over how to construct ordered and unordered lists and how to create hyperlinks.

617 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