why_bird:

Disclaimer: (1) I've never written any OO perl code, (2) I've not reviewed your code thoroughly. I hate the writing in this rambly node, but not quite enough to abandon it. Any comments on improving the writing / organization / code examples in this node would be greatly welcomed!

I noticed two OO mistakes: First, if you have a bunch of if/else/... statements in a chunk of OO code, it's a place to look at and think about factoring. (Specifically, in both get_option and set_option, you have a sequence of conditionals based on the type of the argument, that *begs* for a factorization.) Second (related to the first), it's not extensible. What do you do if you want to add a "date" type? Perhaps a "filename" type might be nice (so it can automatically check for the presence of a file or some such?).

So I'd first factor out a new base class to represent a typed option ... say "option_type". Initially, I'd give it two member functions: set_value and get_value. Then you can subclass it based on the types you want to handle. Thus, you'd get "option_bool", "option_int", "option_num" and "option_str". Factoring out the set_value and get_value code would give you something like^[1]:

package option_str;

sub set_value {
    my ($class, $self, $value) = (shift, shift, shift);
    $self->{value} = $value;
}

sub get_value {
    my ($class, $self, $value) = (shift, shift, shift);
    return $self->{value};
}
[download]

Then the code in your set_option and get_option subroutines would look more like^[2]:

sub set_option
{
    check_args(4,@_);
    my $class=shift;
    my $self=shift;
    my $opt_name=shift;
    my $value=shift;

    my $found=getopt_dev->is_option($self,$opt_name);
    croak "$opt_name is not a valid option" if($found==0);

    $opt_name=~s/^--?//;

    $options->{$opt_name}->set_value($value);
}

sub get_option
{
    check_args(3,@_);
    my $class=shift;
    my $self=shift;
    my $opt_name=shift;
    my $value=0;
    my $type;

    $opt_name=~s/^--?//;

    return $options->{$opt_name}->{value};
}
[download]

After this factorization, you'll find that there are other things that belong in your "option_type" classes: Obviously, there would be a default value for each class type, which you could set in the object constructor (allowing you to rid yourself a bit of code in add_option):

package option_bool;
sub new
{
    my $self = [{ value => 0 },""];
    bless($self);
    return $self;
}
[download]

And, of course, you'd want the name, desc, valid etc. values you currently store for the options in there. You could add the is_valid method to each class, so your is_number and is_int methods would become member functions. Since you don't have is_XXX for bool and str, you could let the base class handle it (by assuming valid, for instance?). Then, you'd review your code (again) to find other things to factor out and make specific to the object type(s). You might have a value formatter for your print_options method, etc.

Then, when you want to add a new type of option to the system, you could add it without having to touch so many bits of your getopt_dev package. If you guess correctly about what bits need to be factored into your option_type base class, you wouldn't have to touch the getopt_dev module at all. You could just add your new option type(s) in another module.

Yark! I hate the way I ramble through this node. But I don't want to throw it away, and I know my code is screwed up. But I think the basic points I wanted to make come through, so here goes.

Notes:

[1] Here's where I first show my perl OO ignorance. I'm certain that my syntax here is screwed up, but I think this'll get the point across.

[2] I'm just going to assume you have a hash of option_type variables called "options", as I don't know where you're storing them...

...roboticus

---

---

• Are you posting in the right place? Check out Where do I post X? to know for sure.
• Posts may use any of the Perl Monks Approved HTML tags. Currently these include the following:

  <code> <a> <b> <big> <blockquote> <br /> <dd> <dl> <dt> <em> <font> <h1> <h2> <h3> <h4> <h5> <h6> <hr /> <i> <li> <nbsp> <ol> <p> <small> <strike> <strong> <sub> <sup> <table> <td> <th> <tr> <tt> <u> <ul>

• Snippets of code should be wrapped in <code> tags not <pre> tags. In fact, <pre> tags should generally be avoided. If they must be used, extreme care should be taken to ensure that their contents do not have long lines (<70 chars), in order to prevent horizontal scrolling (and possible janitor intervention).
• Want more info? How to link or How to display code and escape characters are good places to start.