in reply to why is this code bad under -w option?

Here is another way of writing this code in perhaps a more concise way. 

#!/usr/bin/perl -w

use strict;
use IO::File;

my %multipliers = (
    'KB' => 2**10,
    'MB' => 2**20,
    'GB' => 2**30,
);

print "Enter the pathname and filename of the file to split: ";
my $filename = <STDIN>;


my $valid_block_type;
my $block_size;
my $block_type;

until($valid_block_type)
{
    print "Enter the block size \n (example: 400KB,10MB,1GB): ";
    $block_size = <STDIN>;

    chomp($block_size);

    ($block_type) = ($block_size =~ m/([a-zA-Z]{2})$/);
    $block_type ||= 'KB';
    $block_type = uc $block_type;

    if( grep($block_type eq $_, keys %multipliers) )
    {
        $block_type = $multipliers{$block_type};
        $valid_block_type = 1;
    }
    else
    {
        print "Invalid block type specified!\n\n";
    }
}

[download]
The reason this is better, is because now you only have a single branch of logic, and you can now control this logic in one place instead of two. Anytime you have the oppurtunity to control program flow/logic at a single point in the code, take it. This approach makes the code much more maintainable.

Replies are listed 'Best First'.
Re: Re: why is this code bad under -w option?
by dragonchild (Archbishop) on Jul 28, 2003 at 16:50 UTC
    Color me crazy, but why do you want to spend the time calculating 2**30 when you may not even use it?!? That's just a waste of time and space.

    Also, get rid of the $valid_block_type crud. 

    use strict;
use warnings;

my %is_valid_type = do {
    my $i = 1;
    map { $_ => 10 * $i++ } qw( KB MB GB )
};

print "Enter the pathname and filename of the file to split: ";
my $filename = <STDIN>;
chomp($filename);

my %block = map { $_ => undef } qw( size type amount );

while (1)
{
    print "Enter the block size \n (example: 400KB,10MB,1GB): ";
    chomp($block{size} = <STDIN>);

    @block{qw(amount type)} = ($block{size} =~ m/^\s*([\d.]+)\s*([a-zA
+-Z]{2})\s*$/);
    $block{type} ||= 'KB';
    $block{type} = uc $block_type;

    unless ( $is_valid_type{$block{type}} )
    {
        print "Invalid block type specified!\n\n";
        next;
    }

    $block{size} = 2 ** $is_valid_type{$block{type}};
    last;
}

    [download]
    This way, you defer all the calculation until you actually use it. You also get rid of excess variables and you can treat your block information as a whole instead of three related variables. (Anytime two things are related, make that relationship explicit with either an array or hash. Don't use parallel naming to tell your reader what's going on.)

    Also, you weren't storing the amount of KB that the user want to work with. Did you even write a spec for this? This code looks quite spec-less ...

    ------
    We are the carpenters and bricklayers of the Information Age.

    Don't go borrowing trouble. For programmers, this means Worry only about what you need to implement.

    Please remember that I'm crufty and crochety. All opinions are purely mine and all code is untested, unless otherwise specified.

[reply]
[d/l]

      Color me crazy, but why do you want to spend the time calculating 2**30 when you may not even use it?!? 

      % perl -MO=Deparse -e 'print 2**30'
print 1073741824;
-e syntax OK

      [download]

      (1) Calculating 2 ** $n, where n is a human-scale integer, is ridiculously easy and cheap for computers to do. It's just a bit shifting operation.

      (2) Sub-expressions consisting of constants, such as (2**30) are computed during the compilation stage, not the runtime stage, as demonstrated above. In Perl this doesn't matter that much for runtime savings, since you must compile on every run, but why not use Perl's compiler to do the work instead of expressing yet more (possibly buggy) Perl code to be interpreted to accomplish the same calculation?

      (3) Yes, I said possibly buggy. In your first map definition, you define KB, MB and GB as consecutive multiples of ten, not consecutive powers of 1000. If a maintenance programmer would find the numeric literal or simple expression more readable than the code that derives those values, please, use the numbers!

      Update: Okay, you wanted 10, 20, 30 for KB, MB, GB, just to do a runtime 2**$e later. I still say this doesn't save resources and it doesn't make it more readable. But I'll grant it's what you intended, and not a bug. :)

      --
      [ e d @ h a l l e y . c c ]

[reply]
[d/l]
[select]
        In your your first map definition, you define KB, MB and GB as consecutive multiples of ten, not consecutive powers of 1000.

        I'm storing the powers, not the values. So, the powers are 10, 20, and 30. I then take it to the power later. Thus, it's not buggy (at least from an analysis point of view).

        I was suggesting another solution. In no way do I ever say that you have to do it this way or I'll come over to your house with Vino and Guido to break your kneecaps. :-) And, in fact, you're right about calculating the values ahead of time, but not for the reason you think. The best reason is if the requirements change and there's a new block size called HH that resolves to 734623 bytes. My way is a little obfuscated. Your way is more explicit. Good call!

        ------
        We are the carpenters and bricklayers of the Information Age.

        Don't go borrowing trouble. For programmers, this means Worry only about what you need to implement.

        Please remember that I'm crufty and crochety. All opinions are purely mine and all code is untested, unless otherwise specified.

[reply]

      ok two things, first are you refering to my code, or the "improved code". I did store the amount the user wants to work with and the code works just fine. It may not be the best code ever and that's why I brought it here...constructive criticism...

      secondly.. no I didn't write a spec for this code, because it had to be done quick.. sue me

      Some of the things you said make sense and are good practice. There are nicer ways to say some of the other things you said. If I wanted lashings about code, I would have trolled a C++ board.
[reply]
        There are nicer ways to say some of the other things you said.

        You're right. But, if you wanted me to say everything nicely, you need to pay me. If you want my advice for free, then take it the way I say it.

        I have no idea which code I'm working with. Both versions, however, use a flag to indicate that a valid type has been entered. Both versions don't store how many of that type the block should be. The former is a style mistake and the latter is a programming error.

        As for specs ... had you written a spec, you would have realized that you're not storing the 20 of 20KB. You would also have realized that you have no way of parsing "20 KB " or "1.3GB ". Those are serious mistakes that were commented upon by no-one else.

        This also ties back in to the tone I used - I found a serious design flaw. You can either complain that I didn't sugar-coat it or you can take it and make your program be the best it can be. I don't expect effusive thanks, but I can and do expect a little humility. You were the one who posted your code asking for constructive criticism, so expect constructive criticism. Don't expect me to hold your hand. (Again, that's what you get when you pay me, not when you get your advice for free.) I tend to follow the merlyn school of thought when it comes to this. :-)

        ------
        We are the carpenters and bricklayers of the Information Age.

        Don't go borrowing trouble. For programmers, this means Worry only about what you need to implement.

        Please remember that I'm crufty and crochety. All opinions are purely mine and all code is untested, unless otherwise specified.

[reply]

In Section Seekers of Perl Wisdom