Phone Book

[h4ckroot]

Hello all MOnks Out there :)... Actually i am new here as i am new to perl at all.. however i want to share some code i wrote seeking for your advice i was trying to write a script to a phonebook here is the code i wrote Please tell me any advices or better ways to modify it.

#!/usr/bin/perl -w


open TEMPFH,"DB.txt" or die "Sorry Cant Open File...$!";
#reading the File For Getting the Last id To Support Auto increment.Th
+ere is another way to do it later
#using Tail .
while(<TEMPFH>){
$_ =~/^(\d*).*$/;
    $ID=$1 if eof;
}
close TEMPFH;

print "Welcome To My Simple Phone Book:)\n";
print "What Do You want me To Do\n";
while(1){
print "[L]ist,[A]dd,[D]elete,[Q]uit\n";

$userInput = <STDIN>;
chomp $userInput;

 if (uc $userInput eq "L" or uc $userInput eq "LIST"){

 open READFH,"DB.txt" or die "Sorry Cant Open the DataBase...$!";
    for (<READFH>){

        print;
}
 close READFH;
}
 elsif (uc $userInput eq "A" or uc $userInput eq "ADD"){
 
 open WRITEFH,">>DB.txt" or die "Sorry Cant open The Database..$!";
    while(<WRITEFH>){
print;}
while(1){
    print "Please enter the name\n";
    print "Please Note that Only chars,numbers and underScore are aLlo
+wed with max 30 char and min One Char \n";
    chomp($name = <STDIN>);
    if ($name !~ /^\w{1,30}$/){
    print "Sorry Invalid Name Try Again\n";
    next;
}
else{last;}
}
    while(1){
    print "Please Enter The Number\n";
    print "Please Note that Only Numbers are allowed With max 30 digit
+ and min 1 digit\n";
    
    chomp($number=<STDIN>);
    if ($number !~ /^\d{1,30}$/){
    
    print "Sorry Invalid Number..Try Again\n";
    next;
}
else{last;}
}
    $ID++;
    print WRITEFH "$ID:$name:$number\n";
    print "Added Succefully\n";
close WRITEFH;
}
    elsif(uc $userInput eq "Q" or uc $userInput eq "QUIT"){
        print "Thank You,Keep Visiting :D\n";    
    last;
    }
    
    elsif(uc $userInput eq "D" or uc $userInput eq "DELETE"){

        print "Please Enter the Name You Wana Delete\n";
        chomp($userToDelete = <STDIN>);
        open DELETEFH,"DB.txt" or die "Sorry Couldnt Open DB...$!";
        open TEMP,">temp.txt" or die "Sorry Couldnt Open DB...$!";
        print "Successfully Deleted...\n";
    $myid=1;    
        while (<DELETEFH>){
    
    if (/^\d:$userToDelete:\d*$/)
   {
    $ID--;
    next;
   }

    else{
        $_ =~ /^\d*:(.*)$/;
        print TEMP "$myid:$1\n";
        $myid++;
   }
                }
        close DELETEFH;
        close TEMP ;
        rename "temp.txt","DB.txt" ;
$myid=1;

        }
else{
print "Sorry Invalid Option...Try Again";
}
print "\n";
}

i know i didnt use any pragmas here like strict. :D but anyway i wanted your notes and advices SO Please post your notes.. Thanks in advance..

[jethro]

Strict and consistent indentation are the first things I would change. Also you should have empty lines only at places where you want a visual separation. In short your code makes a very bad first impression

Also you have one long meandering procedure which is not easy to read. Refactor your code into subroutines, in your case you might put the list,add and delete functions into separate subroutines.

The .*$ at the end of the first regexp is useless.

You don't seem to have any use for your ID. And if your phone book never has more than a few 1000 entries you could always read the complete database into memory, into an array and have the index as id number. If it might get bigger you should use a real database engine.

[apl]

In addition to the other advice you've received:
• Put the code to prompt for a user name in a common subroutine. You can always pass a descriptive phrase ('to add', 'to delete') as a parameter.
• Open DB.txt once at the start of your code, and (if necessary) back up the old file and write the new file once at the end of your program (preferably in a subroutine). This enables you to remove multiple FH names.

[Herkum]

Break up your Choice logic so that it calls subroutines that do the actual work. It will make your code easier to understand if you do this,

$userInput = <STDIN>;
chomp $userInput;
$userInput = uc $userInput; 
# you could probably do this on the STDIN, but I am not sure.
# Another monk should be able to tell you.

    ( $userInput eq 'L' or $userInput eq 'LIST'  ) ? list_phone_book()
  : ( $userInput eq 'A' or $userInput eq 'ADD'   ) ? add_to_phone_book
+()
  : ( $userInput eq 'D' or $userInput eq 'DELETE') ? delete_from_phone
+_book()
  : ( $userInput eq 'Q' or $userInput eq 'QUIT'  ) ? quit_phone_book()
  :                                                  unknown_selection
+();

As it stands now, you would have read your whole script line by line to figure out where you are going for your selections. With the above code, you know where to look for each selection and what it is supposed to do.

You are also doing a bunch of open and closes all over the place, I beat your could get rid of a bunch of code by breaking those down in subroutines.

[Lawliet]

$userInput = <STDIN>;
chomp $userInput;
$userInput = uc $userInput;

can be written (more concisely) as

chomp(my $userInput = uc <STDIN>);

Also, I think there may be a better method to running each subroutine than using multiple conditional operators. Perhaps a dispatch table will do.

my %pbactions = (
    L    => \&list_phone_book,
    LIST => \&list_phone_book,
    A    => \&add_phone_book,
    ...
);

# Later on~

defined $pbactions{$userInput} ? $pbactions{$userInput}->()
                               : usage();

Untested lines of code, they're just to inspire.

[GrandFather]

Just a little less work gets you a crude GUI version:

use strict;
use warnings;
use Tk;

my $obj = bless {}; # Create an object to make passing parameters easi
+er and to
    # maintain state. Generally considered better than using global va
+riables.

if (open my $tempIn, '<', "DB.txt") {
    while (<$tempIn>) {
        chomp;
        my ($name, $number) = split ':';

        next unless defined $number; #Skip empty or corrupt lines

        $obj->{phoneBook}{$name} = $number;
    }
    close $tempIn;
}

$obj->{main} = MainWindow->new (-title => 'My Simple Phone Book');
$obj->{main}->Button (-text => 'List', -command => [\&doList, $obj])->
+pack ();
$obj->{main}->Label (-text => "Name:")->pack ();
$obj->{main}->Entry (-textvariable => \$obj->{name})->pack ();
$obj->{main}->Button (-text => 'Del', -command => [\&doDel, $obj])->pa
+ck ();
$obj->{main}->Label (-text => "Number:")->pack ();
$obj->{main}->Entry (-textvariable => \$obj->{number})->pack ();
$obj->{main}->Button (-text => 'Add', -command => [\&doAdd, $obj])->pa
+ck ();
$obj->{main}->Button (-text => 'Exit', -command => [\&doExit, $obj])->
+pack ();

MainLoop ();


sub doAdd {
    my ($self) = @_;

    $self->{phoneBook}{$self->{name}} = $self->{number};
}


sub doExit {
    my ($self) = @_;

    open my $out, '>', "DB.txt" or die "Sorry Cant Open File: $!";
    for my $name (sort keys %{$self->{phoneBook}}) {
        print $out "$name:$self->{phoneBook}{$name}\n";
    }
    close $out;
    exit;
}


sub doDel {
    my ($self) = @_;

    delete $self->{phoneBook}{$self->{name}};
}


sub doList {
    my ($self) = @_;
    my $msg;

    for my $name (sort keys %{$self->{phoneBook}}) {
        $msg .= sprintf "%-20s %20s\n", $name . ':', $self->{phoneBook
+}{$name};
    }

    $self->{main}->messageBox (
        -icon    => 'info',
        -message => $msg,
        -title   => 'Phone numbers',
        -type    => 'Ok',
    );
}

Note that the data is maintained in memory until the program exits.

[h4ckroot]

thanks guys for all your advices....i will re organize it and re post it...again..thanks in advance.

[Anonymous Monk]

i know i didnt use any pragmas here like strict.:D but anyway i wanted your notes and advices SO Please post your notes..
Great, you know your biggest problem, add use strict and go fix it.

[fmerges]

Hi,

Also take a look at IO::Prompt

Regards,

[h4ckroot]

Hi Guys i just made some modifications that you suggested me and here is the code again,Please any advices or improvements are more than welcomed.

#!/usr/bin/perl -w

use strict;

my $ID;
my $userInput;

open TEMPFH,"DB.txt" or die "Sorry Cant Open File...$!";
#reading the File For Getting the Last id To Support Auto increment.Th
+ere is another way to do it later
#using Tail .
while(<TEMPFH>){
$_ =~/^(\d*).*$/;
    $ID=$1 if eof;
}
close TEMPFH;


print "Welcome To My Simple Phone Book:)\n";
print "What Do You want me To Do\n";
while(1){
print "[L]ist,[A]dd,[D]elete,[Q]uit\n";

chomp($userInput = <STDIN>);

 if (uc $userInput eq "L" or uc $userInput eq "LIST"){
    &list($ID);

   }

 elsif (uc $userInput eq "A" or uc $userInput eq "ADD"){
    &add($ID);
 
   }

    elsif(uc $userInput eq "Q" or uc $userInput eq "QUIT"){
        print "Thank You,Keep Visiting :D\n";    
    last;
    }
    
    elsif(uc $userInput eq "D" or uc $userInput eq "DELETE"){
        &delete($ID);
        

        }
else{
print "Sorry Invalid Option...Try Again";
}
print "\n";
}

######################################################################
+#################################################
sub add{

 open WRITEFH,">>DB.txt" or die "Sorry Cant open The Database..$!";

my $name;
my $number;
        while(1){
    print "Please enter the name\n";
    print "Please Note that Only chars,numbers and underScore are aLlo
+wed with max 30 char and min One Char \n";
    chomp($name = <STDIN>);
    if ($name !~ /^\w{1,30}$/){
    print "Sorry Invalid Name Try Again\n";
    next;
  }
      else{
      last;


     }
}
    while(1){

    print "Please Enter The Number\n";
    print "Please Note that Only Numbers are allowed With max 30 digit
+ and min 1 digit\n";
    
    chomp($number=<STDIN>);
    if ($number !~ /^\d{1,30}$/){
    
    print "Sorry Invalid Number..Try Again\n";
    next;
}
else{
last;
}
}
    $_[0]++;
    print WRITEFH "$_[0]:$name:$number\n";
    print "Added Succefully\n";
close WRITEFH;
}
sub delete{
 my $currentID=$_[0];
 my $userToDelete;
 my $myid=1;
print "Please Enter the Name You Wana Delete\n";
        chomp($userToDelete = <STDIN>);
        open DELETEFH,"DB.txt" or die "Sorry Couldnt Open DB...$!";
        open TEMP,">temp.txt" or die "Sorry Couldnt Open DB...$!";
        
     
        while (<DELETEFH>){
    
    if (/^\d*:$userToDelete:\d*$/i)
   {
    $_[0]--;
    next;
   }

    else{
        $_ =~ /^\d*:(.*)$/i;
        print TEMP "$myid:$1\n";
        $myid++;
            }
                }
        if($_[0] < $currentID){

  print "Successfully Deleted user:$userToDelete\n";
}
else{
print "Error..Deleting User Please Check the username Again..,\n";
}
        close DELETEFH;
        close TEMP ;
        rename "temp.txt","DB.txt" ;
$myid=1;



}
sub list{
 open READFH,"DB.txt" or die "Sorry Cant Open the DataBase...$!";
    for (<READFH>){

        print;
}
 close READFH;


}

Thanks in advance.

[zentara]

If you want to make a real improvement to it, look at Gtk2 Entry with validation and Pango coloration You can pop a gui for input.