Hi Agnello, On Sunday 19 December 2010 11:17:48 Agnello George wrote: > Hi > > I have written a small script to input customer data in to a flat file, > i would like some guidance on how i could write this script better , i > would also like to know if i have optimally used HoH properly .
It's great that you're seeking input. > > --------------------------------------------------------------------------- > -------------- [r...@localhost scripts]# cat data_insert.pl > #!/usr/bin/perl -w You shouldn't use "-w". Use "use warnings;" instead (and you already have that). > # this is a test script trying to play around with perl > # A. ask for name , age, address, emailID phone number and put data in > flat file if email iD exist then exit ; > use warnings; > use strict; That's good. > use Data::Dumper ; > > my $txt = '/scripts/data.txt'; I suggest you rename this variable into something more descriptive. > > my %read_data = &get_data; Don't prefix subroutine calls with ampersand: http://perl-begin.org/tutorials/bad-elements/#ampersand-in-subroutine-calls > > print Dumper ([\%read_data]); > > # on print of %read_data here is the output > ##$VAR1 = [ > # { > # '[email protected]' => { > # 'name ' => ' tommy A', > # 'emailid ' => ' [email protected]', > # 'address ' => ' street 123 NY US ', > # 'age ' => ' 33' > # }, > # '[email protected]' => { > # 'name ' => ' agnello', > # 'emailid ' => ' > [email protected]', > # 'address ' => ' c 123 andheri > mumbai', > # 'age ' => ' 25' > # } > # } > # ]; > # > > > my %database; > $/="\n"; You should localise $/ using local. > > my @allask = qw(name age address emailid ); > foreach my $ask (@allask) { > print " please enter $ask:"; > while ( chomp($database{$ask} = <STDIN>) ) { Normally, <> (short for <ARGV>) is preferable over <STDIN>. In your case, I think inputting a file from the user would be inconvenient for the user and you should look into Getopt::Long. > if ( $read_data{$database{$ask}}{'emailid '} ) { # if emailid exist > print "$database{$ask} already exist \n"; > exit; > } > last if $database{$ask}; Always label loops using last and use last LABEL: http://perl-begin.org/tutorials/bad-elements/#flow-stmts-without-labels > print "please enter $ask: "; > } > } I don't understand what the purpose of this loop is. > > open(OUTFILE,">>","$txt") or die " cannot open file : $!"; You should use lexical filehandles: http://perl-begin.org/tutorials/bad-elements/#open-function-style > foreach (@allask) { > print OUTFILE "$_ : $database{$_}\n"; > } Don't iterate using $_ in serious code. > print OUTFILE "\n"; > close (OUTFILE); > > #-------------------- > > sub get_data () { > > $/=""; > Again - localise $/. > my %hoh = (); No need to initialise a hash to () - it's already empty upon creation. > open(IN1,"<","$txt" ) or die " cannot opne file $!" ; Lexical filehandle, but otherwise a clueful form. No need to do "$txt" - $txt would be good enough: http://perl-begin.org/tutorials/bad-elements/#vars_in_quotes > my $ass; $ass has bad connotations in English: http://en.wiktionary.org/wiki/ass#Noun_2 > while ( $ass = <IN1>) { > chomp ($ass); > my ($email) = reverse split (/:/, "$ass" ) ; Again - no need for enclosing in double quotes. And you can also do: my $email = (split/:/, $ass)[-1]; > $email =~ s/\s+//g; > foreach (split (/\n/,$ass) ) { Don't iterate using $_. > $hoh{$email}{$1} = $2 if /(.*):(.*)$/ ; Note that the first .* will be greedy here. Also use \z instead of "$": http://perl-begin.org/tutorials/bad-elements/#caret_and_dollar_sign_in_regexes > } > } > > return %hoh; Better return a hash reference. It's faster and cleaner. See: http://perl-begin.org/topics/references/ Regards, Shlomi Fish -- ----------------------------------------------------------------- Shlomi Fish http://www.shlomifish.org/ First stop for Perl beginners - http://perl-begin.org/ Chuck Norris can make the statement "This statement is false" a true one. Please reply to list if it's a mailing list post - http://shlom.in/reply . -- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected] http://learn.perl.org/
