Beefy Boxes and Bandwidth Generously Provided by pair Networks
Just another Perl shrine
 
PerlMonks  

Code Review Time!

by linuxkid (Sexton)
on Mar 14, 2012 at 22:19 UTC ( #959680=perlquestion: print w/ replies, xml ) Need Help??
linuxkid has asked for the wisdom of the Perl Monks concerning the following question:

So, I have some code, that always errors, this is the error:No -label at C:/strawberry/perl/site/lib/Tk/Widget.pm line 256. Here is the code that does causes it, it gets annoying. I Updated the code listing on here.

#!/usr/bin/perl ############################################# # Call List Manager, Copyright 2012 Sam # # Flint, linuxkid@linux.com. # # Licensed under the GNU GPLv3 # ############################################# # PMDF # # Version: 1.0 # # License: GNU GPLv3 # # Name: Call List Manager # # File: clm.pl # # Author: Sam Flint, linuxkid@linux.com # # Maintainer: Sam Flint, linuxkid@linux.com # # Commenting: verbose # ############################################# #Extensions Needed use Tk; use PDF::Reuse; use YAML::Tiny; use Tk::Tree; use Tk::ROText; use Tk::LabEntry; use Tk::Text; $iparse = YAML::Tiny->new(); $Mw = MainWindow->new(-title=> "Calling List Manager"); $menuitems = [ [Cascade => "File", -menuitems=> [ [Button => "~Open", -command=> \&open], [Button => "~Save", -command=> \&savefile], [Button => "Save ~As", -command=> \&saveas], #[Seperator => ""], [Button => "E~xit", -command=> \&exitprog], ] ], [Cascade => "Edit", -menuitems=> [ [Button => "Add Calle~r", -command=> \&acaller], [Button => "Add Calle~e", -command=> \&acallee], #[Seperator => ""], [Button => "Remove Caller", -command=> \&rcaller], [Button => "Remove Callee", -command=> \&rcallee], #[Seperator => ""], [Button => "~Generate Call Lists", -command=> \&generate], [Button => "~Mail Call Lists", -command=> \&mailout], #[Seperator => ""], [Button => "~Preferences", -command=> \&prefs], ] ], [Cascade => "Help", -menuitems=> [ [Button => "~About", -command=> \&about], [Button => "~Help", -command=> \&help], ] ] ]; $menubar = $Mw->Menu(-menuitems => $menuitems); $Mw->configure(-menu => $menubar); $wframe = $Mw->Frame()->pack(-side=>'top', -fill=>'both', -expand => 1 +); $tree = $wframe->Scrolled( 'Tree', -relief => 'flat', -scrollbars => 'osoe', -borderwidth => 0, -separator => '.', -drawbranch => 'true', -indicator => 'true', )->pack(-side => 'top', -fill => 'both', -expand => 1); $inf = $Mw->Frame()->pack(-side=> "top", -fill=> "x", -expand=> 1); $inf->Label(-text=> "Status:")->pack(-side=>'left', -expand=> 1); $inf->Label(-textvariable=> \$message)->pack(-side=>'left', -fill=>'x' +, -expand=> 1); $inf->Label(-text=> "Save State")->pack(-side=>'left', -fill=>'x',, -e +xpand => 1); $inf->Label(-textvariable=> \$savestate)->pack(-side=>'left', -fill=>' +x', -expand => 1); #openprefs(); MainLoop(); # main Subs $helptext = <<EOF; EOF sub ginf { $tree->info('selection'); } sub openfile { our $file = $Mw->getOpenFile(); our $data = $iparse->read($file) || $Mw->Dialog(-title=> "File Open + Error", -text=>"$!: Cannot Open File: $file!", -buttons=>["Ok"], -de +faultbutton=>"Ok", -bitmap=> "warning")->show();; our @list; our @callers; foreach (<FH>) { if ($type eq "caller") { $id = shift @fields; $phnum = shift @fields; $email = shift @fields; $address = shift @fields; $notes = shift @fields; $data->{$id}->{type} = 'caller'; $data->{$id}->{name} = $id; $data->{$id}->{phnum} = $phnum; $data->{$id}->{email} = $email; $data->{$id}->{addr} = $address; $data->{$id}->{notes} = $notes; push @list, $id; push @callers, $id; } elsif($type eq "callee") { $caller = shift @fields; $name = shift @fields; $phnum = shift @fields; $email = shift @fields; $address = shift @fields; $notes = shift @fields; $id = $caller.'.'.$name; $data->{$id}->{type} = 'callee'; $data->{$id}->{name} = $name; $data->{$id}->{phnum} = $phnum; $data->{$id}->{email} = $email; $data->{$id}->{addr} = $address; $data->{$id}->{caller} = $caller; $data->{$id}->{notes} = $notes; push @list, $id; } else { next; } } redotree(); } sub redotree { $tree->delete(0, 'end'); my $col = 0; foreach $node (@job_list) { $node_name = (split('.', $node))[-1]; $node_name = $node if ($node_name eq ''); $tree->add($node); $tree->itemCreate( $node, $col, -text => $node_name, -itemtype => 'text'); } $tree->autosetmode(); } sub savefile { $sfile = $file; $sfile ||= shift; $data->write($sfile); } sub saveas { savefile($Mw->getSaveFile()); } sub exitprog { savefile(); $Mw->Withdraw(); exit; } sub acaller { editentry("boogie", "new", "caller"); } sub acallee { editentry("boogie", "new", "callee"); } sub rcaller { } sub rcallee { } sub generate { } sub mailout { } sub prefs { } sub about { $tlw = $Mw->Toplevel(-title=> "About"); $info = $tlw->ROText()->pack(-side => 'top', -fill => 'both', -expa +nd => 1); $info->insert('end', "Calling List Manager, Copyright, 2012 Sam Fli +nt.\n"); $info->insert('end', "Version 1.0\n"); $info->insert('end', "Registered to $conf->{registered}\n"); $tlw->Button(-text=> "Ok", -command=> sub { $tlw->withdraw })->pack +(); } sub help { $tlw = $Mw->Toplevel(-title=>"Help"); $info = $tlw->ROText()->pack(-side => 'top', -fill => 'both', -expa +nd => 1); $tlw->Button(-text=> "Ok", -command=> sub { $tlw->withdraw })->pack +(); $info->insert('end', $helptext); } sub editentry { $path = shift; $stat = shift; $type = shift || $data->{$path}->{type}; if ($stat eq 'new') { $tlw = $Mw->Toplevel("New Entry"); if ($type eq "caller") { $tlw->LabEntry(-text=>"Name", -textvariable=>\$id)->pack(-fil +l=>'x'); $tlw->LabEntry(-text=>"Phone Number", -textvariable=>\$phnum) +->pack(-fill=>'x'); $tlw->LabEntry(-text=>"Email", -textvariable=>\$id)->pack(-fi +ll=>'x'); $tlw->LabEntry(-text=>"Address", -textvariable=>\$id)->pack(- +fill=>'x'); $tlw->Text(-text=>"Name", -textvariable=>\$notes)->pack(-fill +=>'x'); $tlw->Button(-text=> "Ok", -command=> sub { $data->{$id}->{name} = $name; $data->{$id}->{type} = 'caller'; $data->{$id}->{phnum} = $phnum; $data->{$id}->{email} = $email; $data->{$id}->{addr} = $address; $data->{$id}->{notes} = $notes; })->pack(-fill=>'x'); } elsif($type eq "callee") { } else { $tlw->withdraw(); } } elsif($stat eq 'old') { $tlw = $Mw->Toplevel(-title=> "Edit Entry"); } else { redotree(); } } sub openprefs { open FH, "~/clm.conf"; bless our $conf; foreach (<FH>) { ($key, $value) = split /:/; $conf->{$key} = $value; } } __END__
Please ignore unwritten subs, I have to get the interface working, otherwise it won't matter.

--linuxkid

imrunningoutofideas.co.cc

Comment on Code Review Time!
Select or Download Code
Re: Code Review Time!
by jwkrahn (Monsignor) on Mar 14, 2012 at 22:54 UTC

    When I compile your code I get:

    $ perl -c 959680.pl Type of arg 1 to push must be array (not scalar dereference) at 959680 +.pl line 95, near "@list;" Type of arg 1 to push must be array (not scalar dereference) at 959680 +.pl line 96, near "@callers;" Type of arg 1 to push must be array (not scalar dereference) at 959680 +.pl line 112, near "@list;" 959680.pl had compilation errors.

    Which refers to these three lines:

    95 push $id, @list; 96 push $id, @callers; 112 push $id, @list;

    The first argument to push must be an array.    Perhaps you meant:

    95 push @list, $id; 96 push @callers, $id; 112 push @list, $id;


    If I enable warnings I get these messages:

    $ perl -Mwarnings -c 959680.pl Name "main::key" used only once: possible typo at 959680.pl line 232. Name "main::savestate" used only once: possible typo at 959680.pl line + 65. Name "main::job_list" used only once: possible typo at 959680.pl line +123. Name "main::message" used only once: possible typo at 959680.pl line 6 +3. 959680.pl syntax OK
      Code review really wasn't the topic, but that wasn't the problem I was having. I was wondering about the Tk error, but thanks.

      --linuxkid


      imrunningoutofideas.co.cc

        linuxkid:

        If code review wasn't the topic you wanted, then you made an odd title choice! ;^D

        ...roboticus

        When your only tool is a hammer, all problems look like your thumb.

Re: Code Review Time!
by davido (Archbishop) on Mar 14, 2012 at 23:13 UTC

    Your question's title "Code Review Time!" gives me the impression you're looking for comments on the code. But the body of your post seems to be asking a question about a specific error, which is quite different from a code review. Which would you prefer?

    If you're really looking for a code review, start with Perl::Critic. Install the module, then "perlcritic yourscript.pl" Once you resolve any of the critiques there that make sense for your particular code (such as code before strictures), and once your code can be run with 'warnings' and 'strict', without generating warnings and errors, you might be ready for a human code review, although I would first re-run perlcritic at a stronger severity level, addressing as many of the additional critiques it comes up with as is practical. I've found that it isn't always relevant, possible, or wise to apply all of its advice, but at least looking at the advice and making conscious decisions about whether or not to take further action is an exercise that sometimes helps to uncover bugs and maintainability shorcomings.

    But until then I think you should probably be asking a question that is more specific to the nature of the error you're getting. For that, I think we would need more information, and a code snippet that just produces the error without a bunch of unnecessary code for us to sift through. In particular, you need to try to narrow down the list of dependencies needed to produce the error. I'd rather not install a whole bunch of modules I don't need, but wouldn't be opposed to installing the one or two that are required to replicate the error. You may even find that in trying to narrow the code down to the smallest test case that can possibly produce the error, you'll find out what the error actually is.

    Update: I do have one other thought though; when I installed Tk (Strawberry/Windows) I find that it won't even pass its own test suite, nor will several of the modules you're using. If you installed with the 'force' option to make it install despite failing its test suite (as opposed to actually fixing all the problems), you may just be reaping what you sewed.


    Dave

      well, yes i forced. but this was going to be pacaged with pp for a standalone executable for a charity i work with that make alot of calls. now i have to fanagle with stuff

      --linuxkid


      imrunningoutofideas.co.cc
Re: Code Review Time!
by zentara (Archbishop) on Mar 15, 2012 at 11:39 UTC
    As far as code review goes, the first thing to do is add
    use warnings; use strict;
    as others pointed out. It helps avoid the very type of problem you experience, and makes it easier for others, like us, to work with your code.

    I'm not really a human, but I play one on earth.
    Old Perl Programmer Haiku ................... flash japh
Re: Code Review Time!
by thundergnat (Deacon) on Mar 15, 2012 at 16:01 UTC

    Umm. wow. You've got some problems here. Ok, to very narrowly answer your question; the error:

    No -label at C:/strawberry/perl/site/lib/Tk/Widget.pm line 256.

    is caused by your menu commands not having any -label parameter. -text (or in this case, text) is not a valid parameter name for a Tk menu command. Look up valid parameters in perldoc Tk::Menu under $menu->add(type, ?option, value, option, value, ...?)

    Some other problems that jumped out after a brief perusal:

    There's no menu method seperator, it is spelled separator. Weird since you have it spelled correctly in some places, not in others.

    The array and value being pushed are reversed in several push operations. push @list, $id; not push $id, @list;

    You are missing a sigil on $key in line 237: $conf->{$key} = $value; not $conf->{key} = $value;

    Seriously, activate the warnings and strict pragmas and fix or at least investigate everything that they report. If you are a fairly new Perl programmer it is worth it. Yes, turning them on will make perl complain about things that are not necessarily "wrong", but it will reduce the effort you need to spend debugging by orders of magnitude.

      I've been working with perl for about 3 years, and using warnings and strict breaks everything, and it's annoying when it's something that people who just know how to type and use a mouse will end up using have to get the stuff from it.

      --linuxkid


      imrunningoutofideas.co.cc

        I've been working with perl for about 3 years, and using warnings and strict breaks everything, and it's annoying when it's something that people who just know how to type and use a mouse will end up using have to get the stuff from it.

        Read this if you want to cut your development time in half!

        Wow, I've not seen such a rant since BK coined "use strict is gay" way back in 2001 (which inspired Acme::USIG).

        Update: Though the original historic bulletin board response from "bk" to Dave Cross is currently available via the Wayback Machine link above, I also embed the original text (including typos) below because it is too important to Perl history to risk losing:

        Posted By: bk
        Date: Wednesday, 2 May 2001, at 2:29 p.m.

        In Response To: Re: Will this work? (Dave Cross)

        No,but its true-- strict really does suck. I hate it. Its gay. Dont tell me ehat to do. And nobody wants your to be back, either.

Log In?
Username:
Password:

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

How do I use this? | Other CB clients
Other Users?
Others pondering the Monastery: (5)
As of 2014-08-21 02:49 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    The best computer themed movie is:











    Results (127 votes), past polls