amadain has asked for the wisdom of the Perl Monks concerning the following question:

Monks
Is this where newbies can get code reviewed? I didn't want to post this in the code section as it'll probably get slated bad. I would like you to review it and let me know how to improve my coding
It creates a storage file and the asks you for premiership teams and their last results. there is then an option to see any two teams together to show their previous records when trying to predict the score of an impending match between them.

#!/usr/bin/perl use strict; use Term::ReadLine; use Shell qw(cp); local *SNMPFILE; my $term = Term::ReadLine->new(); my (@Values, @teams, $reply); @teams = ('Arsenal', 'Bolton', 'Villa', 'Chelsea', 'Middlesbrough', 'N +ewcastle', 'Birmingham', 'Blackburn', 'Palace', 'Fulham', 'Liverpool' +, 'ManchesterCity', 'Norwich', 'Portsmouth', 'Spurs', 'WBA', 'Manches +terUnited', 'Southampton', 'Charlton', 'Everton' ); # open up the storage file system("touch store.txt"); open(SNMPFILE,"+< store.txt"); # assign the elements of the storage file to an array @Values = split ('\s+', <SNMPFILE>); close(SNMPFILE); # clear the screen my $clear = `/usr/bin/clear`; print $clear; # Show Choice Menu print "___________________________\n\n0. Storage File Setup\n1. Update + Teams\n2. Check previous Results\n3. Backup Storage File\n4. quit\n\ +n___________________________\n"; $reply=$term->readline(); chomp($reply); if ($reply == "0") { &init_file(\@teams); } elsif ($reply == "1") { &team_update(\@Values, \@teams); print $clear; exit 1; } elsif ($reply == "2") { &team_find(\@Values); print $clear; exit 1; } elsif ($reply == "3") { &storage_backup(); print $clear; exit 1; } else { print "Exiting\n"; sleep 2; print $clear; exit 1; } sub init_file { my ($teams) = @_; my $element; local *SNMPFILE; system("touch store.txt"); open(SNMPFILE,"+< store.txt"); foreach $element (@$teams) { print SNMPFILE "$element S S S S "; } close(SNMPFILE); } sub team_update { my ($Values, $teams) = @_; my ($clear, $answer, $team_number, $home_result, $away_result, $sc +ore_for, $score_against, $ae); # clear the screen $clear = `/usr/bin/clear`; print $clear; # update values with this weeks results and scores for (my $j = 0; $j <= $#teams; $j++) { print "\nDo You have results for $teams[$j] (y\\n)?\n"; $answer=$term->readline(); chomp($answer); if ($answer =~ /[Yy](es)*/) { $team_number = $j*5; $Values[$team_number]=$teams[$j]; print "$j\t\t$Values[$team_number]\n"; print "\nHome Result ((W D or L) _ if its an away game +):\n"; $home_result=$term->readline(); chomp($home_result); $home_result=~s/\s+$//g; $Values[$team_number+1]=~s/($Values[$team_numb +er+1])/$1$home_result/g; print "\nAway Result ((W D or L) _ if its an home game +):\n"; $away_result=$term->readline(); chomp($away_result); $away_result=~s/\s+$//g; $Values[$team_number+2]=~s/($Values[$team_numb +er+2])/$1$away_result/g; print "\nGoals Scored:\n"; $score_for=$term->readline(); chomp($score_for); $score_for=~s/\s+$//g; $Values[$team_number+3]=~s/($Values[$team_numb +er+3])/$1$score_for/g; print "\nGoals Conceeded:\n"; $score_against=$term->readline(); chomp($score_against); $score_against=~s/\s+$//g; $Values[$team_number+4]=~s/($Values[$team_numb +er+4])/$1$score_against/g; } # clear the screen print $clear; } # update storage file open(SNMPFILE,"+< store.txt"); for ($ae = 0; $ae <= $#Values; $ae++) { print SNMPFILE "$Values[$ae] "; } close(SNMPFILE); return 1; } sub team_find { my ($Values) = @_; my ($clear, $ans, $team1, $team2); # clear the screen $clear = `/usr/bin/clear`; print $clear; # find team index $ans = "y"; while ($ans =~ /[Yy]/) { print "\nTeams to be found:\n"; $team1=$term->readline(); $team2=$term->readline(); chomp($team1); chomp($team2); $team1=~s/\s+$//g; $team2=~s/\s+$//g; for (my $i = 0; $i <= $#Values; $i++) { if ($Values[$i] =~ /$team1/) { print "\n___________________________\n\nteam:\t$Values +[$i]\n___________________________\n"; for (my $j = 1; $j < 5; $j++) { $Values[$i+$j]=~s/^S//g; } print "\nHome Record is:\n$Values[$i+1]\n"; print "Away Record is:\n$Values[$i+2]\n"; print "\nPreviously scored:\n$Values[$i+3]\n"; print "Previously Conceeded:\n$Values[$i+4]\n"; } elsif ($Values[$i] =~ /$team2/) { print "\n___________________________\n\nteam:\t$Values +[$i]\n___________________________\n"; for (my $j = 1; $j < 5; $j++) { $Values[$i+$j]=~s/^S//g; } print "\nHome Record is:\n$Values[$i+1]\n"; print "Away Record is:\n$Values[$i+2]\n"; print "\nPreviously scored:\n$Values[$i+3]\n"; print "Previously Conceeded:\n$Values[$i+4]\n"; } } print "\nAgain (y\\n)?\n"; $ans=$term->readline(); chomp($ans); } return 1; } sub storage_backup { # clear the screen my $clear = `/usr/bin/clear`; print $clear; unless(cp("store.txt", "store.txt.bak")==0) { print "Could not copy store.txt to store.txt.bak\n"; return 0; } print "Successfully copied store.txt to store.txt.bak\n"; sleep 2; return 1; }

Janitored by Arunbear - added readmore tags, as per Monastery guidelines

Replies are listed 'Best First'.
Re: code for review?
by tachyon (Chancellor) on Nov 11, 2004 at 11:23 UTC

    Here is something that look a bit cleaner. There are lots of comments. no warnings, /usr/bin/clear won't port. Hardcoded file paths are bad. Not checking the result of file opens is bad. Redundant code is bad ie most of your if/else do 'cls'+exit. You don't really need Term::Readline or Shell. Use File::Copy if you must.

    #!/usr/bin/perl use strict; use warnings; my $DATFILE = "c:/store.txt"; my @teams = qw( Arsenal Bolton Villa Chelsea Middlesbro +ugh ` Newcastle Birmingham Blackburn Palace Fulham Liverpool ManchesterCity Norwich Portsmouth Spurs WBA ManchesterUnited Southampton Charlton Everton ); clearscreen(); # if we don't have a datfile we HAVE to INIT it!!! init_file(\@teams) unless -e $DATFILE; # now we can read the datfile becuase it exists, 100% guaranteed open F, $DATFILE or die "Can't read $DATFILE, Perl says $!\n"; chomp ( my @values = <F> ); close F; print " --------------------------- 1. Update Teams 2. Check previous Results 3. Backup Storage File 4. Quit --------------------------- Option: "; chomp( my $reply = <STDIN> ); clearscreen(); if ($reply == "0") { init_file(\@teams); } elsif ($reply == "1") { team_update(\@values, \@teams); } elsif ($reply == "2") { team_find(\@values); } elsif ($reply == "3") { storage_backup(); } else { print "Exiting\n"; } exit 0; #----------------------------------------------------------------- # subs #----------------------------------------------------------------- sub clearscreen { $^O =~ m/Win32/ ? system('cls') : system('clear') } sub init_file { my ($teams) = @_; open F, ">$DATFILE" or die "Can't write $DATFILE, Perl say $!\n"; print F "$_\n" for @$teams; close F; } # yada

    cheers

    tachyon

      Thanks.
      Thats the kinda stuff I needed. Although I did use Term::ReadLine because I am running this on Cygwin and it does not handle mistakes in typing well. If I type in W, then backspace over W and type in L for example the the storage file gets WL written to it

        Ah, fair enough. I only ever use cygwin for diff and tar and gzip to be frank. You don't need cygwin to do perl on Win32 of course.

      First, excellent restructuring.

      Maybe it's just me – and it very well could be (I'm goofy) – but that menu is just begging for some more compaction:

      my $menu = { 0 => sub { init_file(\@teams) }, 1 => sub { team_update(\@values, \@teams) }, 2 => sub { team_find(\@values) }, 3 => sub { storage_backup() }, default => sub { print "Exiting\n" }, }; ( $menu->{$reply} || $menu->{default} ) -> ();

      Ah, that feels better.     ;-)

      Cheers,
      Tom

        brilliant
        I implemented all of the changes in the replies that I got and this is all great to learn from. If there are any more comments they are greatly appreciated. The updated script is as follows. Please add more feedback if you think that i can improve it more.
        Thanks