in reply to code for review?

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

Replies are listed 'Best First'.
Re^2: code for review?
by amadain (Novice) on Nov 11, 2004 at 12:16 UTC
    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.

        You can use Cygwin for a lot more, and it's a lot nicer, imho, than using ActivePerl. But, then again, I'm a vi-nazi who doesn't see the point of GUI IDEs. :-)

        Being right, does not endow the right to be rude; politeness costs nothing.
        Being unknowing, is not the same as being stupid.
        Expressing a contrary opinion, whether to the individual or the group, is more often a sign of deeper thought than of cantankerous belligerence.
        Do not mistake your goals as the only goals; your opinion as the only opinion; your confidence as correctness. Saying you know better is not the same as explaining you know better.

Re^2: code for review?
by tmoertel (Chaplain) on Nov 12, 2004 at 05:33 UTC

    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