Does it really run? Seriously? Because there seem to be too many broken structures for me to really believe that.
Example:
Code:
cat jan.txt | while read uname password gname fullname; do
#Reads /etc/passwd for Username
egrep -w "^$uname:" /etc/passwd >/dev/null 2>&1
#If Username is found then error reports
if [ $? == 0 ]
then
echo "User Already Exists : Error adding user with username $uname;$gname;$fullname" >> Errors1.log
else
#Reads /etc/group for Groupname
egrep -w "^$gname" /etc/group
#If Groupname is found then nothing
if [ $? == 0 ]; then
echo ""
else
#If Groupname not found then creates new group and reports
groupadd "$gname"
echo "Group Not Found: New Group $gname was created" >> Successes1.log
done < $1
The
while..done loop here is being fed
stdin from two sources at once, the pipe from
jan.txt and the redirection from
$1. Several of the
if statements, here and elsewhere, don't seem to line up with proper closing marks either.
Then there's another "
done" dangling at the end of the script that doesn't seem to match any actual loop.
Debugging things like this would be a lot easier if you applied better formatting. Don't just indent by two spaces, use a whole tab stop. And don't do it haphazardly, consistently indent
every loop and test. Make sure that every single level of subcommands has its own clear indentation step.
Add some more blank lines between logical sections too. Leave plenty of blank space between the loops and groups of commands, so you can see at a glance where the change-ups occur.
Watch what happens when I try to apply consistent formatting to the above code:
Code:
cat jan.txt | while read uname password gname fullname; do
#Reads /etc/passwd for Username
egrep -w "^$uname:" /etc/passwd >/dev/null 2>&1
#If Username is found then error reports
if [ $? == 0 ]; then
echo "User Already Exists : Error adding user with username $uname;$gname;$fullname" >> Errors1.log
else
#Reads /etc/group for Groupname
egrep -w "^$gname" /etc/group
#If Groupname is found then nothing
if [ $? == 0 ]; then
echo ""
else
#If Groupname not found then creates new group and reports
groupadd "$gname"
echo "Group Not Found: New Group $gname was created" >> Successes1.log
done < $1
Notice how easily we can see now that neither of the
if statements has proper closings (at least not inside this code block, where they should be). That, or the "
done" line is in the wrong place.
Finally, you should try to be consistent with your commands. For example, sometimes you use
[..] tests, and sometimes
[[..]] tests. Try to stick to the latter when using
bash. (And if it's an integer comparison, switch to using
((..)) instead.)
Other than that though, I have to say that your
in-line syntax is generally pretty good. Everything is quoted properly, everything is commented, and there's no glaring ugliness. You just need to take more care at the macro level.
