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.

• 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.

$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
+();
[download]

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.

$userInput = <STDIN>;
chomp $userInput;
$userInput = uc $userInput;
[download]

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();
[download]

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

And you didn't even know bears could type.

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',
    );
}
[download]

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

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

Hi,

Also take a look at IO::Prompt

Regards,

#!/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;


}
[download]

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.

---

I'm not really a human, but I play one on earth Remember How Lucky You Are