MostHated
asked on
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.
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!
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
}
ASKER CERTIFIED SOLUTION
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Just because someone may be able to write a one liner, doesn't mean it will be clearer.
ASKER
@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.
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.
SOLUTION
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
ASKER
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!
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!
Hi again MH,
A few suggestions on the micro level if you want to continue with bash...
Intead of this style of indentation:
Because the "if..." is not inside the "read", and the "fi" should be at the same level as the "if".
And instead of this:
And instead of this:
Or if you prefer a one-liner...
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).
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:if test "$NEWUSER" = ""; then
echo "$0: sorry, username cannot be blank" >&2
exit 1;
fi
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 "$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=$NEWUSE R/" ~/store_installer/lib/conf ig.cfg
sed -i -e"s/^pass=.*/pass=$NEWPAS S/" ~/store_installer/lib/conf ig.cfg
sed -i -e"s/^domain=.*/domain=$NE WDOMAIN/" ~/store_installer/lib/conf ig.cfg
sed -i -e"s/^email=.*/email=$NEWE MAIL/" ~/store_installer/lib/conf ig.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.sed -i -e"s/^pass=.*/pass=$NEWPAS
sed -i -e"s/^domain=.*/domain=$NE
sed -i -e"s/^email=.*/email=$NEWE
CONFIGFILE="~/store_instal ler/lib/co nfig.cfg"
Then later...
sed -i "s/^user=.*/user=$NEWUSER/ " $CONFIGFILE
sed -i "s/^pass=.*/pass=$NEWPASS/ " $CONFIGFILE
sed -i "s/^domain=.*/domain=$NEWD OMAIN/" $CONFIGFILE
sed -i "s/^email=.*/email=$NEWEMA IL/" $CONFIGFILE
# Note the '-e' switch is not required.sed -i "s/^pass=.*/pass=$NEWPASS/
sed -i "s/^domain=.*/domain=$NEWD
sed -i "s/^email=.*/email=$NEWEMA
Or if you prefer a one-liner...
sed -i "s/^user=.*/user=$NEWUSER/ ; s/^pass=.*/pass=$NEWPASS/; s/^domain=.*/domain=$NEWDO MAIN/; s/^email=.*/email=$NEWEMAI L/" $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).
ASKER
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.
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
}
Again, thanks for all the help and input you have provided!
Hi MH,
This:
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.
If you just want less 'text' then you'll lose on readability.