0
\$\begingroup\$

I have full working code, but when I run it, it shows these warnings:

given is experimental at grocery.pl line 38.
when is experimental at grocery.pl line 40.
when is experimental at grocery.pl line 62.
when is experimental at grocery.pl line 89.
when is experimental at grocery.pl line 100.

How can I fix these?

use strict;
use warnings;
use List::Util qw(max);
use feature "switch";
my $b;
my $c;
my $p;
my $highest;

print "____________________________STORE THE ITEM_____________________\n";
my %grocery=
(
    "soap"=>{"lux"=>13.00,"enriche"=>11.00},
    "detergent"=>{"surf"=>18.00},
    "cleaner"=>{"domex"=>75.00}
);
foreach my $c(keys %grocery)
{
print "\n";
    print"$c\n";
    foreach my $b(keys %{$grocery{$c}})
    {
        print "$b:$grocery{$c}{$b}\n";
    }
}
my $ch;
do
{
    print "________________MENU_________________\n";
    print "1.ADD ITEM\n";
    print "2.SEARCH\n";
    print "3.DISPLAY\n";
    print "4.FIND THE MAX PRICE\n";
    print "5.EXIT\n";
    print "enter your choice \n";
    $ch=<STDIN>;
    chomp($ch);
    given($ch)
    {
        when(1)
        {
            print "Enter the category you want to add";
            $c=<STDIN>;
            chomp($c);
            print "Enter brand\n";
            $b=<STDIN>;
            chomp($b);
            print "Enter price\n";
            $p=<STDIN>;
            chomp($p);
            if(exists($grocery{$c}))
            {
                $grocery{$c}{$b} = $p;  
                print "\n";
            }
            else
            {
                $grocery{$c}={$b,$p};
                print "\n";
            }       
        }
        when(2)
        {
            print "Enter the item that you want to search\n";
            $c=<STDIN>;
            chomp($c);
            if(exists($grocery{$c}))
            {
                print "category $c exists\n\n";
                print "Enter brand\n";
                $b=<STDIN>;
                chomp($b);
                if(exists($grocery{$c}{$b}))
                {
                    print "brand $b of category $c exists\n\n";
                    print "-----$c-----\n";
                    print "$b: $grocery{$c}{$b}\n"
                }
                else
                {
                    print "brand $b does not exists\n";
                }
            }
            else
            {
                print "category $c does not exists\n";
            }
        }
        when(3)
        {
            foreach $c (keys %grocery)
            {
                print "$c:\n";
                foreach $b(keys %{$grocery{$c}})
                {
                    print "$b:$grocery{$c}{$b}\n";
                }
            }
        }
        when(4)
        {
            my @all_prices;
            foreach my $c (keys %grocery) {
            push @all_prices, values %{ $grocery{$c} };
        }
        $highest = max @all_prices;
        print "$highest\n"; 
    }
    }
}while($ch!=5);
\$\endgroup\$
  • \$\begingroup\$ Does this help? perlmonks.org/?node_id=1078449 \$\endgroup\$ – πάντα ῥεῖ Dec 31 '16 at 13:10
  • 2
    \$\begingroup\$ Welcome to Code Review! This question is incomplete. To help reviewers give you better answers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question. \$\endgroup\$ – Edward Dec 31 '16 at 13:33
  • 3
    \$\begingroup\$ The question is derived from this one over at SO: stackoverflow.com/questions/41407497/… \$\endgroup\$ – simbabque Dec 31 '16 at 13:52
  • \$\begingroup\$ I'm done with my answer. If you've seen the older version, please take another look. And don't forget to edit the question to include context. I'm sure someone else has to add something. \$\endgroup\$ – simbabque Dec 31 '16 at 15:16
  • 1
    \$\begingroup\$ If the end of a code block doesn't fit on the same editor page as the beginning of the same block then you have designed your code wrongly \$\endgroup\$ – Borodin Jan 1 '17 at 22:32
6
\$\begingroup\$

The warning

The warning you are seeing is because the switch feature that gives you the given and when keywords is experimental. That means it's there in this specific Perl version, and possible future ones, but that is not clear. Sometimes experimental features make it into the core part of the language itself, but sometimes they get removed. With switch it's not clear yet which one it's going to be.

If you are use you are only going to use your specific Perl version, you can simply turn off the warnings.

use feature 'switch';
no warnings 'experimental';

Now you don't see any warnings any more. But if your code will be run on an older version that doesn't have the feature yet, or on a later version in the future where it has might have been changed or removed, your code will break.

I would not use experimental features in long-living production code. Instead, I would use either of the following two approaches, based on how complex your program is.

Using a bunch of ifs and elsifs

The most simple approach is to just replace the given/when with a couple of if statements. Your do { ... } while ( $ch != 5 ) approach is good, so we keep it. But because it's not really a loop, but rather a block with a postfix condition, we cannot use next to jump out, so we need to use an elsif construct.

my $ch;
do {
    $ch = <>;
    chomp $ch;

    if ( $ch == 1 ) {
        # new item ...
    }
    elsif ( $ch == 2 ) {
        # search ...
    }
    elsif ( $ch == 3 ) {
        # display ...
        next;
    }
    elsif ( $ch == 4 ) {
        # max price ...
    }
} while ( $ch != 5 );

We don't even need to use an elsif because we can just use next to skip to the next iteration.

Using a dispatch table

This is way more advanced, and a bit overkill for your situation, but it's very nicely extensible. A dispatch table is essentially a hash with code references that you call depending on your input.

my %dispatch = (
    1 => sub {
       # new item ...
    },
    2 => sub {
       # search ...
    },
    3 => sub {
       # display ...
    },
    4 => sub {
       # max price ...
    },
);

my $ch;
do {
    # read input ...

    if ( exists $dispatch{$ch} ) {
        $dispatch{$ch}->();
    }
} while ( $ch != 5 );

But as I said, this is an advanced technique and possibly a bit overkill for this program.

Telling the user they gave wrong input

If you enter another number than 1 to 5, your program does not complain. It just reiterates the same output. If you enter a letter, it will issue a warning.

Input validation up front

You could check the input first, issue a warning and then redo the loop.

do {
    # read input ...

    if ( $ch !~ m/^[1-5]$/ ) {
        print "Invalid input.\n";
    }
    elsif ( $ch == 1 ) { 

    # ...
} while ( $ch != 5 );

Don't use numerical operators

Or you could replace all your == and != with eq and ne, which work on strings as well as numbers, and move the warning to the end of the loop.

my $ch;
do {
    $ch = <>;
    chomp $ch;

    if ( $ch eq 1 ) {
        # new item ...
    }
    elsif ( $ch eq 2 ) {
        # search ...
    }
    elsif ( $ch eq 3 ) {
        # display ...
        next;
    }
    elsif ( $ch eq 4 ) {
        # max price ...
    }
    else {
         print "Invalid input.\n";
    }
} while ( $ch ne 5 );

Both of those approaches work.

Self-explaining variable names

Your variable names $b, $c, $p and $ch do not speak for themselves. The program is very hard to read because of that. Instead of on-letter names, name them for what they represent.

$ch could be $selection, $c could be $category, $b the $brand and $p could be $price.

Smaller scope

You are already using strict and lexical variables with my, which is very good. But you should not declare your vars at the top like in a C program. In Perl you should always use the smallest possible scope.

For example, your $c is declared outside the loop (I'll continue to say loop, although it's not a loop structure), but you use it in multiple places where you assign new values in different branches of your code.

Instead, you should declare a new, lexical version in each of those branches, which are individual from each other. That helps to protect from bugs. Right now you could easily introduce a bug, if you for example accidentally put == instead of =. Because the variable is already declared in a higher scope, Perl won't tell you that that's possibly wrong. But if you instead declare it freshly, it will complain.

print "Enter the category you want to add";
my $category = <STDIN>;   
chomp $category; # no need for parenthesis here
if ( exists( $grocery{$category} ) ) {
    print "Enter brand\n";
    my $brand = <STDIN>;
    chomp $brand;
    print "Enter price\n";
    my $price = <STDIN>;
    chomp $price ;
    $grocery{$category} = { $brand => $price };

    print "\n";
}
# ...

You should do that throughout the code.

Autovivification

When you create a new entry, you check if the category exists, and then branch the code. You don't need to do that. Right now you have duplicated all the code for getting the brand and price from the user, but you only need to write that once.

elsif ( $selection == 1 ) {
    print "Enter the category you want to add\n"; # you forgot a \n here
    my $category = <>; # <> is the same as <STDIN>

    print "Enter brand\n";
    my $brand = <STDIN>;

    print "Enter price\n";
    my $price = <STDIN>;
    chomp( $category, $brand, $price );
    $grocery{$category}->{$brand} = $price;
}

The above code sums both cases up into one. The assignment $grocery{$category}->{$brand} = $price works because Perl will automatically create a hash reference inside $grocery{$category} if it sees the ->{$brand} part. That's called autovivification and is explained in the perlglossary.

I also moved the chomp into one line so make the code more concise, as chomp can take a list of variables. In that case you need the parenthesis so it knows it's a list.

Use say instead of print "...\n"

You can enable the say builtin that works like print, but includes a newline at the end of each invocation. It will make your code more concise. To enable it, you need to

use feature 'say';

at the top of your program. You can then use it throughout the code.

if ( $selection !~ m/^[1-5]$/ ) {
    say "Invalid input.";
}

Use a HERE doc for blocks of output

HERE docs are useful for multi-line output. You can use one for the main menu that lists your options.

do {
    print <<'EOTEXT';
________________MENU_________________
1.ADD ITEM
2.SEARCH
3.DISPLAY
4.FIND THE MAX PRICE
5.EXIT
enter your choice
EOTEXT

    $selection = <>;

The delimiter EOTEXT can be chosen freely. It should be all-caps and I like to start it with EO, which means end of. The semicolon goes at the end of that same line. The single quotes '' mean that there is no interpolation. If you want to include variables in the text, use "EOTEXT" instead. The delimiter has to be placed in the line after the text, and needs to be preceded by an empty line. As of Perl 5.24, HERE docs cannot be indented. Perl 5.26 will include a new feature that allows this.

\$\endgroup\$
  • \$\begingroup\$ "Your do { ... } while ( $ch != 5 ) approach is good" I strongly disagree. The block is enormous, and the while($ch!=5) is hidden away after three or four screens of code. In addition, as you note, the do block is just a compound statement and not a proper loop, so the loop control words won't work or -- even worse -- will control a different block from the one you expect. It's always much better to use an indefinite while () { ... } and insert next and last statements to micro-control the loop in an intelligible way. \$\endgroup\$ – Borodin Jan 1 '17 at 22:26
  • \$\begingroup\$ do { ... } while and do { ... } until really shouldn't be documented as they are. They are simply examples of postfixed statement modifiers, like do { ... } if, where do is used to allow a block of multiple statements to behave as one, whereas the books imply that they are a way to write a repeat ... until block. That construction isn't provided by Perl. \$\endgroup\$ – Borodin Jan 1 '17 at 22:29
  • \$\begingroup\$ @borodin I agree with the infinite loop. When I wrote that I was thinking that the OP appears to be from a background of learning computer science, or programming. The approach of using a loop that's controlled at the bottom is the right way to deal with the situation of repeating with at least one iteration. From a learning perspective that's good. But I agree with you that it's not very readable. \$\endgroup\$ – simbabque Jan 1 '17 at 22:32
  • \$\begingroup\$ Yes. I thought so too. In fact I initially thought it was a loop. I had a part of the answer written up before I ran my code example with next when I realized it's not a loop. It's obvious from the list of compound statements if you know what you're looking for, but it could be more clear. \$\endgroup\$ – simbabque Jan 1 '17 at 22:34

Not the answer you're looking for? Browse other questions tagged or ask your own question.