LinuxQuestions.org
Register a domain and help support LQ
Go Back   LinuxQuestions.org > Forums > Non-*NIX Forums > Programming
User Name
Password
Programming This forum is for all programming questions.
The question does not have to be directly related to Linux and any language is fair game.

Notices

Reply
 
Search this Thread
Old 06-19-2007, 02:42 PM   #1
dave201
LQ Newbie
 
Registered: Oct 2005
Location: Camelot
Distribution: cent, RH core, slax, slackware... *buntu...
Posts: 28

Rep: Reputation: 15
code critique? a better way?


This is a "snippit" of a program I wrote. The program adds/deletes lines from a pipe delimited file. The code listed here is the part thats doing the deleting, and is designated with the -r. The logic is flow is as such: read the field number, Ask if its correct, then see if the field exists in the file. If it doesn't, report and error, and exit. If it does have the field, spit out some info on it, and delete. A sample fieldlist is provided just for clarification. It also needs to delete multiple lines at once.

I'm looking for any and all comments on ways to accomplish what I've done in less lines of code, or perhaps with a bit more style. I'm also looking to pick up a different language... perl perhaps. Any advice is appreciated.
Code:
FIELDLISTFILE="/home/dave/fieldlist.txt"

# <Sample fieldlist.txt> #
111|alpha|
222|bravo|
333|charlie|
444|gamma|
555|delta|
555|dupel|
# </Sample fieldlist.txt> #

if [ ${1} = "-r" ]; then
checked=0
   while [ $checked =  0 ];
     do
     clear
     echo -ne "\nField number of line to remove: "
     read -n3 field
     echo -e "\n\nYou have selected line "$field"  for removal."
     sleep 1
     echo -en "\nIs this correct?"
     read -es -n1 -p "(y/n)" keypress
     if [ $keypress = "y" ]; then
         clear
         grep $field $FIELDLISTFILE > /dev/null
         if [ $? = 0 ]; then
         grep -n $field $FIELDLISTFILE > temp.tmp
         while read line
             do
                 lineCut=`echo $line |cut -d\: -f1`
                 fieldName=`echo $line |cut -d\| -f2`
                 echo -e '\E[30;42m'"\n\nRemoved field ""$fieldName"", field number ""$field"" from the fieldlist.\033[0m"  
                 sed -i  ${lineCut}d ${FIELDLISTFILE}
                 sleep 2
                 clear
         done < temp.tmp
         rm -f temp.tmp
         checked=1
         else
             clear
             echo -e '\E[31;40m'"\n\nFIELD NOT FOUND, EXITING.\033[0m"
             sleep 3    
         exit 1
         fi
     fi
   done
tput sgr0
fi
 
Old 06-20-2007, 06:32 AM   #2
chrism01
Guru
 
Registered: Aug 2004
Location: Sydney
Distribution: Centos 6.5, Centos 5.10
Posts: 16,289

Rep: Reputation: 2034Reputation: 2034Reputation: 2034Reputation: 2034Reputation: 2034Reputation: 2034Reputation: 2034Reputation: 2034Reputation: 2034Reputation: 2034Reputation: 2034
In shell, equality tests for numbers uses the symbol " -eq "
eg
if [[ $? -eq 0 ]]
then
your code
fi

String tests are

if [[ $str == "match" ]]
then
your code
fi

assignment
str=match

See http://rute.2038bug.com/index.html.gz
 
Old 06-20-2007, 08:26 AM   #3
bigearsbilly
Senior Member
 
Registered: Mar 2004
Location: england
Distribution: FreeBSD, Debian, Mint, Puppy
Posts: 3,290

Rep: Reputation: 174Reputation: 174
lose the cut
you can split a line with read using multiple variables,
according to the value of $IFS
usually on whitespace

and why the temp file?
just pipe straight in


like:
Code:
        
        grep -n $field $FIELDLISTFILE |

        while read lineCut fieldName rest
             do
                 # blah!

        done
 
Old 06-20-2007, 01:23 PM   #4
dave201
LQ Newbie
 
Registered: Oct 2005
Location: Camelot
Distribution: cent, RH core, slax, slackware... *buntu...
Posts: 28

Original Poster
Rep: Reputation: 15
thanks! that cut about 10% off the programs runtime! woohoo!
 
Old 06-21-2007, 01:57 AM   #5
bigearsbilly
Senior Member
 
Registered: Mar 2004
Location: england
Distribution: FreeBSD, Debian, Mint, Puppy
Posts: 3,290

Rep: Reputation: 174Reputation: 174
it's a useful tool.
you can use IFS to set the split char, if you just want to grab the
output not keep persistent variables it's safer to do in a subshell,
so you don't clobber IFS for other stuff, a'la:
Code:
# code here
(
IFS=:
while read name the_rest 
 do 
    echo $name 
 done < /etc/passwd
) | pipe_to_this
try this on the command line:
(IFS=: && ls -1d $PATH)
 
Old 06-21-2007, 11:17 AM   #6
dave201
LQ Newbie
 
Registered: Oct 2005
Location: Camelot
Distribution: cent, RH core, slax, slackware... *buntu...
Posts: 28

Original Poster
Rep: Reputation: 15
Yup, I understand subshells. I wouldn't mind posting some of my other code for your review... do you mind?
 
Old 06-22-2007, 03:07 AM   #7
bigearsbilly
Senior Member
 
Registered: Mar 2004
Location: england
Distribution: FreeBSD, Debian, Mint, Puppy
Posts: 3,290

Rep: Reputation: 174Reputation: 174
certainly not, that's what the forum is for
I've done a fair amount of scripting in my time

though I'm actually a ksh lover!

here's a good one,
#!/bin/bash -eu
#!/bin/ksh -eu


I use a fair bit,
-e causes any error to exit the script immediately
-u causes any unbound variable to error, which is very useful for catching silly typos.

set -x
set -u
are the same thing but after the script starts

works in ksh anyway,
 
Old 06-22-2007, 12:00 PM   #8
dave201
LQ Newbie
 
Registered: Oct 2005
Location: Camelot
Distribution: cent, RH core, slax, slackware... *buntu...
Posts: 28

Original Poster
Rep: Reputation: 15
ahh, those are great to know. I'll post some code soon... I'm actually writing a bunch of stuff in KSH... but having major issues cron'ing these scripts.

I can't seem to get the darn thing to run correctly. Parts of the code work, but other parts just completely don't run. I know the script is working because I have a small logger function at the end... which does write to a log.

I think it has something to do with cron using SH for everything, but I'm perplexed at a way to work around it.

My crontab looks like...

0,10,20,30,40,50 * * * * /usr/bin/ksh /home/dave/myScript.sh > /dev/null
 
Old 06-25-2007, 04:32 AM   #9
bigearsbilly
Senior Member
 
Registered: Mar 2004
Location: england
Distribution: FreeBSD, Debian, Mint, Puppy
Posts: 3,290

Rep: Reputation: 174Reputation: 174
well, as long as the script has a #!/bin/ksh it will be fine.
the thing with cron is you start with a limited environment and
no default tty, for instance the $PATH variable is very limited.
you won't have all the stuff like $LOGNAME either.

you can of course source your .profile in your script

you can always test with a set -x also.

you can do limited cron job testing using batch though this inherits the current environment,
eg:

echo script | batch

Last edited by bigearsbilly; 06-25-2007 at 04:34 AM.
 
  


Reply


Thread Tools Search this Thread
Search this Thread:

Advanced Search

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is Off
HTML code is Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
Iptables Critique Centinul Linux - Security 2 08-09-2005 10:03 AM
New iptables configuration critique gizza23 Linux - Networking 11 08-06-2005 10:05 PM
Partitioning Critique Wanted Skazi Slackware 7 08-11-2004 02:20 PM
Tieing up loose ends on Web Page (Critique needed) johnp General 3 05-13-2004 11:03 PM
can experienced java users critique this? megaspaz Programming 8 01-24-2003 12:43 AM


All times are GMT -5. The time now is 03:22 PM.

Main Menu
My LQ
Write for LQ
LinuxQuestions.org is looking for people interested in writing Editorials, Articles, Reviews, and more. If you'd like to contribute content, let us know.
Main Menu
Syndicate
RSS1  Latest Threads
RSS1  LQ News
Twitter: @linuxquestions
identi.ca: @linuxquestions
Facebook: linuxquestions Google+: linuxquestions
Open Source Consulting | Domain Registration