Beefy Boxes and Bandwidth Generously Provided by pair Networks
Don't ask to ask, just ask
 
PerlMonks  

What is wrong with this code?

by perl.j (Pilgrim)
on Jul 23, 2011 at 23:36 UTC ( [id://916354]=perlquestion: print w/replies, xml ) Need Help??

perl.j has asked for the wisdom of the Perl Monks concerning the following question:

Dear Monks,

I am trying to make an "all-in-one area calculator" for my brother to help him with his homework. I thought it would be an easy task. The problem I have is, when it asks "What kind of shape am I working with?", and I say "rectangle", it starts printing the things it is supposed to print when I type in "square". Note: The code is below. All of the scalars at the top are not needed, I just put them there to refer to.

Oh, and I know I might not have been to clear so if you have questions please ask.

use 5.12.4; use Math::Trig; use strict; ###################################################################### +##### my $square_side= shift; ##SQUARE AREA FORMULA my $square_area= $square_side*2; ###################################################################### +##### my $rectangle_side=shift; ##RECTANGLE AREA FORMULA my $rectangle_side_2=shift; my $rectangle_area = $rectangle_side*$rectangle_side_2; ###################################################################### +##### my $paralellogram_base=shift; ##PARALELLOGRAM AREA FORMULA my $paralellogram_height= shift; my $paralellogram_area=$paralellogram_base*$paralellogram_height; ###################################################################### +##### my $trapezoid_base=shift; ##TRAPEZOID AREA FORMULA my $trapezoid_base_2=shift; my $trapezoid_height=shift; my $trapezoid_area=$trapezoid_height/2*($trapezoid_base*$trapezoid_bas +e_2); ###################################################################### +##### my $circle_radius=shift; ##CIRCLE AREA FORMULA my $pi=pi; my $circle_area=$pi*($circle_radius**2); ###################################################################### +###### my $triangle_base=shift; ##NON-EQUALATERAL TRIANGLE FORMULA my $triangle_height=shift; my $triangle_area=.5*($triangle_base*$triangle_height); ###################################################################### +###### say "I am an area calculator. I can calculate the area of most basic s +hapes (mostly).\n"; say "\nWhat type of shape am I working with?\n"; my $type=<>; chomp ($type); while ($type){ if ($type eq 'square' or 'Square'){ say "Okay. I am working with a $type.\n"; say "ENTER SIDE LENGTH"; $square_side= <>; my $square_area= $square_side*2; say "ANSWER= $square_area"; last; }} while ($type){ if ($type eq 'rectangle' or 'Rectangle'){ say "Okay. I am working with a $type.\n"; say "ENTER FIRST SIDE LENGTH"; $rectangle_side=<>; say "ENTER SECOND SIDE LENGTH"; $rectangle_side_2=<>; say "ANSWER= $rectangle_area"; last;}} while ($type){ if ($type eq 'paralellogram' or 'Paralellogram'){ say "Okay. I am working with a $type.\n"; say "ENTER BASE"; $paralellogram_base=shift; say "ENTER HEIGHT"; $paralellogram_height= shift; say "ANSWER= $paralellogram_area"; last;}}

perl.j-----A Newbie To Perl

Replies are listed 'Best First'.
Re: What is wrong with this code?
by toolic (Bishop) on Jul 23, 2011 at 23:56 UTC
    Change:
    if ($type eq 'square' or 'Square'){
    to:
    if ( ($type eq 'square') or ($type eq 'Square') ){
    This is a precedence issue (see perlop). Since 'Square' is always true, you always get a square.

    You need to do this for your other shapes as well.

    Another common way to be case-insensitive is to use lc:

    if (lc($type) eq 'square'){
    One difference from your code is that this will also accept 'sQUaRe', etc.

    See also use strict and warnings.

Re: What is wrong with this code?
by GrandFather (Saint) on Jul 24, 2011 at 02:41 UTC
    1. Why use say then add explicit new lines at the end of the line?
    2. Why a series of while loops, especially as $type isn't changed from one loop to the next?
    3. Think about what happens in each loop if the body of the if is not executed (hint - what causes the loop to terminate?)
    4. The string returned by <> includes the new line character at the end of the line. You probably want to chomp before using entered strings as values.
    5. Why don't you "use warnings"?
    6. shift isn't going to do it for you for "paralellogram".

    For a version of your program to ponder a while consider:

    use 5.010; use strict; use warnings; my %types = ( square => {dimensions => ['side'], calc => \&calcSquare}, rectangle => {dimensions => ['width', 'height'], calc => \&cal +cRect}, parallelogram => {dimensions => ['base', 'height'], calc => \&cal +cRect}, ); say "I am an area calculator."; while (1) { say "Enter stop to stop or one of: ", join ' ', sort keys %types; print "What type of shape am I working with? "; my $type = <>; chomp ($type); last if lc $type eq 'stop'; if (!exists $types{lc $type}) { say "Sorry, I don't know about $type."; next; } my @dims; for my $dim (@{$types{$type}{dimensions}}) { print "Enter a value for $dim:"; my $value = <>; chomp $value; push @dims, $value; } say "The area or your $type is: ", $types{$type}{calc}->(@dims); } say "Thanks for using me. Come again sometime"; sub calcSquare { return $_[0] * $_[0]; } sub calcRect { return $_[0] * $_[1]; }

    Whenever you find yourself writing essentially the same code over and over, consider either using a loop or using a subroutine to contain the common stuff. This example use a hash which contains a reference to the subs used to do the actual calculation and a list of the dimensions required for each shape. Adding a new shape is just add the calculation sub and a new line to the hash.

    True laziness is hard work
Re: What is wrong with this code?
by jethro (Monsignor) on Jul 24, 2011 at 00:11 UTC

    Ok, the problem is with this line:

    if ($type eq 'square' or 'Square'){ #is the same as if (($type eq 'square') or ('Square')){ # is the same as if (($type eq 'square') or (1)){ # is the same as if (1) { #You want: if ($type eq 'square' or $type eq 'Square'){ #or if (lc($type) eq 'square') {

    Note that the same problem is with the other to 'if' clauses further down.

    As soon as you have done this you will notice the next bug: Your script will never exit and will either hang immediately or after you've gone through the "square" part. The three while loops you have there make no sense and will loop endlessly because in at least two of the three while loops the if-clause will be false and therefore the 'last' never executed. Just remove those while loops.

    Maybe you wanted those while loops to ask again for the $type if the user misspelled it. In that case you need one big while loop that encloses the line where the user enters the data, i.e. " my $type=<>; "

      Thank You so much jethro!

      perl.j-----A Newbie To Perl
Re: What is wrong with this code?
by ww (Archbishop) on Jul 24, 2011 at 01:09 UTC
    In addition to toolic's and jethro's catches, note that the syntax of your formula for the area of a square isn't correct. It will give the area as (side times 2); you want $square_side**2 ($square_side to the power of 2, eg, squared).

    Also, check your spelling (parallelogram and equilateral, for example), lest you teach li'l bro' errors and review your geometry text to check your title in the triangle formula.

    Other suggestions, depending on your taste:

    • When you wish to include info like your formulae in a non-executable format, include it inside POD markup, used as a block comment:
      =head formulae $area_square=$side**2; ... =cut
    • Unless you plan to expand this greatly, you'll have less overhead (and more portability) if you simply define pi as 3.141659 3.14159 (Thanks, eyepopslikeamosquito) rather than using Math::Trig.
    • You may wish to provide the user with a choice to exit or restart. Various techniques are available.
    • IMO using the equal sign in your answer lines (and generally, using the equals sign in any response to a user) risks making your code harder to proof ("Is that "=" an assignment or a symbol?"). I find usage such as print "Answer:  " . function(); or similar friendlier.

    Updated: to acknowledge jethro's excellent point, posted as I fiddled with minutia and eyepopslikeamosquito's catch on my brain-fart.

      you'll have less overhead (and more portability) if you simply define pi as 3.14159

      Blech. Get maximum precision without Math::Trig (and without programmer error):

      $PI = 4 * atan2(1,1);
      I reckon we are the only monastery ever to have a dungeon stuffed with 16,000 zombies.

      you'll have less overhead (and more portability) if you simply define pi as 3.141659 rather than using Math::Trig.
      Of course, that should read 3.14159. Or even 3.14159265358979323846264338327950288419716939937510 :)

Re: What is wrong with this code?
by flexvault (Monsignor) on Jul 24, 2011 at 15:22 UTC

    Everyone has helped you with the technical problems of your script, but you should think about the usefulness of helping your brother(end user) use the script. Just think how annoying it is to type 'CIRCE' by accident, and nothing happens!

    Use perl to help the typist, such as:

    my $help = qq|I am an area calculator that can do the following:\n \t 1 - Square \t 2- Rectangle\n \t 3 - Paralellogram \t 4 - Trapezoid\n|; ## Add additional fu +nctions here! print "\n\n$help\n"; while ( 1 ) { print "What would you like to do? "; my $type=<>; chomp ($type); $type = lc($type); while ($type) { if ( ($type eq '1' )||( $type eq 'square') ) { $type = "Square"; say "Okay. I am working with a $type.\n"; say "ENTER SIDE LENGTH"; my $square_side = chomp(<>); my $square_area= $square_side**2; say "ANSWER= $square_area"; } elsif ( ($type eq '2' )||( $type eq 'rectangle') ) { $type = "rectangle"; # etc. . . } elsif ( $type eq '' ) { last; } else { print "\n\n$help\n"; } } } print "Thank you for using my Calculator\n";
    This is untested code, so I'll leave the debugging to you. I have introduced some new functions, but the idea is to build a framework for your future work. You could add a hash or array of types. You could make each call a separate subroutine, add HTML and this could be the start of a cgi script for your web server. But best of all you have a framework for you and your brother to control your environment with perl. Maybe your brother's class mates would like to use your program and expand on it. Put a copyright on it, and now you're an author.

    Learning perl now, will enable you to do and control customized things for the rest of your life.

    Personally, I like that you found a real problem (your brother's learning) and you are solving it yourself.

    Good start!

    "Well done is better than well said." - Benjamin Franklin

      Thank You for the code. I will get back to you on the bugs (if any). The reason I didn't make it too easy to use is because I wanted to make sure the basic parts of the code worked first. I am looking to add more to it (and use some of your tips) but for now, here is my finished code:

      Oh, and by the way. I know I didn't make that as simple as it can be. I'm working on that. I just wanted to show you guys some of the fixes.

      use 5.14.1; use Math::Trig; ###################################################################### +##### my $square_side= shift; ##SQUARE AREA FORMULA my $square_area= $square_side*2; ###################################################################### +##### my $rectangle_side=shift; ##RECTANGLE AREA FORMULA my $rectangle_side_2=shift; my $rectangle_area = $rectangle_side*$rectangle_side_2; ###################################################################### +##### my $paralellogram_base=shift; ##PARALELLOGRAM AREA FORMULA my $paralellogram_height= shift; my $paralellogram_area=$paralellogram_base*$paralellogram_height; ###################################################################### +##### my $trapezoid_base=shift; ##TRAPEZOID AREA FORMULA my $trapezoid_base_2=shift; my $trapezoid_height=shift; my $trapezoid_area=$trapezoid_height/2*($trapezoid_base*$trapezoid_bas +e_2); ###################################################################### +##### my $circle_radius=shift; ##CIRCLE AREA FORMULA my $pi=pi; my $circle_area=$pi*($circle_radius**2); ###################################################################### +###### my $triangle_base=shift; ##NON-EQUALATERAL TRIANGLE FORMULA my $triangle_height=shift; my $triangle_area=.5*($triangle_base*$triangle_height); ###################################################################### +###### say "I am an area calculator. I can calculate the area of most basic s +hapes (mostly).\n"; say "\nWhat type of shape am I working with?\n"; my $type=<>; chomp ($type); if ($type eq 'square' or $type eq 'Square'){ say "Okay. I am working with a $type.\n"; say "ENTER SIDE LENGTH"; $square_side= <>; my $square_area= $square_side*2; say "ANSWER= $square_area"; } elsif ($type eq 'rectangle' or $type eq 'Rectangle'){ say "Okay. I am working with a $type.\n"; say "ENTER FIRST SIDE LENGTH"; $rectangle_side=<>; say "ENTER SECOND SIDE LENGTH"; $rectangle_side_2=<>; $rectangle_area = $rectangle_side*$rectangle_side_2; say "ANSWER= $rectangle_area";} elsif ($type eq 'paralellogram' or $type eq 'Paralellogram'){ say "Okay. I am working with a $type.\n"; say "ENTER BASE"; $paralellogram_base=<>; say "ENTER HEIGHT"; $paralellogram_height=<>; $paralellogram_area=$paralellogram_base*$paralellogram_height; say "ANSWER= $paralellogram_area"; } elsif ($type eq 'trapezoid' or $type eq 'Trapezoid'){ say "Okay. I am working with a $type.\n"; say "ENTER FIRST BASE"; $trapezoid_base=<>; say "ENTER THE SECOND BASE"; $trapezoid_base_2=<>; say "ENTER HEIGHT"; $trapezoid_height=<>; $trapezoid_area=$trapezoid_height/2*($trapezoid_base*$trapezoi +d_base_2); say "ANSWER= $trapezoid_area"; } elsif ($type eq 'circle' or $type eq 'Circle'){ print "Okay. I am working with a $type. \n"; say "ENTER RADIUS"; $circle_radius=<>; my $c_answer= ($circle_radius**2)*3.14; say "ANSWER= $c_answer"; } elsif ($type eq 'triangle' or $type eq 'Triangle'){ print "Okay. I am working with a $type.\n"; say "\n ENTER BASE"; $triangle_base=<>; say "ENTER HEIGHT"; $triangle_height=<>; $triangle_area=.5*($triangle_base*$triangle_height); say "ANSWER= $triangle_area"; }else{ die "I do not have that shape programmed in to my system (yet).Ple +ase tell my creator to add the function you have attempted to use.\n\ +n\n\n"; }

      perl.j-----A Newbie To Perl

        Glad you picked up on the use of 'elsif'. Used properly, it's a very powerful tool and you used it correctly. It can help you find all possible combinations that are acceptable ( just like you did ).

        Now this is my preference, since I do a lot of web cgi programming, is to not overuse the 'die' function. You used it correctly, but I prefer to have a 'sub DieRtn{}' that is called in a error situation. This routine can determine the severity of the error and provide a way to recover. Once you 'die', it's not easy to recover.

        And if this was a cgi script, the 'die' error would just be shown on the browser window. This wouldn't be very friendly, when a simple "I don't understand. . ." would allow the user to try it again.

        Now look at this from your user's (brother) point of view. He has to type in the command and then type the shape and then the size(s) and then he sees "I do not ...". What you want as the designer/programmer of this script is for him to use this as much as possible. That was the purpose of the loop:

        while ( 1 ) ## You may prefer while ( 1==1 ) { Do your stuff . . . elsif ( $type eq '' ) { last; } else { ... notify of input not correct and show what's acceptable . + . . } }
        Now you have a clean exit to the program and you have reduced the typing for your end-user. If this is for a typing class and not a math class, then testing his typing skills would be a good thing.

        Keep up the good work...Ed

        "Well done is better than well said." - Benjamin Franklin

Re: What is wrong with this code?
by JavaFan (Canon) on Jul 24, 2011 at 08:22 UTC
    Your program starts off by reading a whopping 11 arguments, and then doing calculations with them.

    Is that the reason you aren't using warnings? Because I doubt you're giving 11 arguments and then entering an interactive session.

      Yes, JavaFan, that is the reason I'm not using warnings. Any other time I would use it ;).
      perl.j-----A Newbie To Perl

        Yes, JavaFan, that is the reason I'm not using warnings. Any other time I would use it ;)

        Or you could use warnings and provide defaults

Re: What is wrong with this code?
by perl.j (Pilgrim) on Jul 24, 2011 at 13:18 UTC
    Thank You to everybody who helped. I fixed all of the bugs and it is working fine now : ).
    perl.j-----A Newbie To Perl

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: perlquestion [id://916354]
Approved by toolic
help
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others lurking in the Monastery: (3)
As of 2024-04-25 23:39 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found