Complex interfaces

The problem primarily is that your set_choices interface takes a complex datatype. This gives you very little room for in-module syntax checking. You can either reject the entire set of choices and leave the interface-programmer wondering where they went wrong, or not check it at all and end up with the interface-programmer scratching their head at what is presented to them on the screen.

Rather than taking a HoH as a single argument to the set_choices() function, you should instead repeatedly call an add_choice() function or equivalent to add choices. Yes, its a little more work, but the advantage is that you can do nice checks against the data as it comes in without having to traverse the complex type itself, you can also (but not in this instance) use perl prototyping to help the interface-programmer get their code right at compile time.

The most important benefit however is documentation simplicity. It is trivial to document the following sub, whereas documenting set_choices() above would require being very specific about the nature of each of the entries, the format required, plus the duplication of _id and the hash key (which is also $id).

sub set_choice {
    my ($self, $id, $name, $sub_or_id, $return_sub) = @_;

        $id !~ /^\d+$/ && die "ID must be a valid positive number";
        $name || die "Name must be specified";

        $self->{$id}{_id} = $id;
        $self->{$id}{_name} = $name;
        $self->{$id}{_sub_or_id} = $sub_or_id;
        $self->{$id}{_return_sub} = $return_sub;
}
[download]

This also means that the interface-programmer can store their menu entires in whatever form is convenient to them, without having to do the (possibly error-prone) translation to your format before submission, ie:

my @entries = ("Entry #1","Entry #2","Entry #3","Quit");
my $count=0;
for (@entries) {
        $menu->set_choice($count++,$_);
}
[download]

Which may be considerably more convenient for them, especially if the menu items are coming out of a database or part of a larger data structure.

Always try to keep the interfaces simple, its the point at which the interface-programmer and the module-programmers minds have to mesh, and thus the weakest point in the application.

... || die("sorry,..."), _header => ...

--Dave

Well, that solved a probable error, but it uncovered a more serious one. As I expected, there does indeed appear to be a problem with the passed hash. podmaster has mentioned (via CB) that this is probably the problem, and that I should pass a hashref instead, but I haven't figured that out just yet. Here's a new dump...

[jason@lappy jason]$ perl test_menu.pl 
$VAR1 = bless( {
                 '1' => undef,
                 '2' => undef,
                 '3' => undef,
                 '_header' => 'Just testing...',
                 '4' => undef,
                 '_comment' => 'Ok, this is where the comment section 
+would normally go.',
                 '_title' => 'Testing menu'
               }, 'Menu' );
[download]

Update: Thanks to podmaster who pointed out that set_choices() should have {}'s in the HoH assignment, rather than ()'s. Thanks everyone!

The problem is actually in the constructor, specifically in the line _title => $args{title} || die "Sorry, we need a title for this menu!". I believe that perl is parsing this such that the remainder of the hash population is read as parameters to the die statement (recall that die can take a list). Try removing the die statement and you'll get the behavior you want. (Update I just grokked dpuu's comment before. Same point). More generally, it's a good idea to separate data validation from the population of data structures precisely to avoid wierd bugs like this one.

Some other comments:

1. first of all, for an initial attempt at module writing, this is readable and fairly sophisticated, so kudos.
2. Secondly, in your constructor, it would make more sense to change $self to $class, as the first element of @_ in a class method is the name of the package, not a reference to an instance (with the exception of constructors that double as clone methods, as discussed in perl oo)
3. Next, I would standardize your argument passing conventions since at present you're passing both hashes (set_choices) and named lists (new).
4. Finally, in set_choices, since it's getting complex, assign $_ to an intelligible temporary variable. Tracking its value in anything other than simple loops, maps, etc., can get hairy.