Hi
Well I'm not sure if you are asking about OO in general, or OO as implemented in Perl. Also I'm struggling to understand some parts of your code... Maybe if you could boil your code down to a simple example that illustrates your question...
Anyway, here are some observations to start with (just stating the obvious in case I've misunderstood something):
- You've a class called MeteoalarmCountries
- new() sets a property for the user agent
- The method countries:
- Keeps a lookup of filenames for images
- Retrieves $self and stores the rest of the parameters in $self as hash keys
- Builds a URL and fetches it with LWP
- splits the result on <a href=
- Checks each element of the split for some ID codes and stores something in $self based on looking up in the image hash
- deletes the keys user_agent, day and type from $self
- returns $self
- You later mention that by storing the country codes in $self->{countries} you can distinguish them from the parameters and avoid the need to delete them.
Now for some commentary:
You are correct that you can differentiate between the country data and other things by using a nested structure. However, why are you storing the method parameters and method results in the object? The object's properties should be things needed between calls to the method. The use of user_agent seems appropriate, though later you may want to create get/set method(s) for it.
Why not just store the method parameters in local variables or a hash?
The same for the return value: if it's a list or hash just build it into a local variable and just return it. If that information is required for later method calls, storing it in the object is appropriate, but the code would be clearer with a get/set method in my view.
Sometimes we return $self to allow method calls to be chained (e.g. $self->do_something->do_something_else->etc). However you don't need to return $self because the caller already has it. In your case, the method is called countries so I think it should return some countries.
Here's some pseudo-Perl illustrating how I think you could structure it:
# pseudo-Perl only, not for executing
Package MeteoAlarmCountries;
# ... use stuff...
sub new {
# ...
}
sub fetch_country_warnings {
my $self = shift;
my %p = @_;
my $url = _make_url($p{day}, $p{type});
my $content = _fetch_content($url, $self->get_user_agent);
my $warnings = _extract_country_warnings($content);
return $warnings;
}
sub _make_url {
# ...
}
sub _fetch_content {
# ...
}
sub get_user_agent {
# return static default,
# or do a config lookup,
# or allow get/set from outside
return $ua_string;
}
sub _extract_country_warnings {
my $content = shift;
# ...
my %lookup_table = ( ... )
my %country_warnings;
foreach ( ... split ... ) {
# match and lookup
$country_warnings{$country} = $something
}
return \$country_warnings;
}
If the countries/warnings are required to be stored in the object for later, you could split fetch_country_warnings into fetch_and_store_country_warnings and a simple get/set method country_warnings
I hope this makes sense...
By the way, what happens if the required tag is written <a id="blah" href="whatever">etc</a>? It won't match your split.
FalseVinylShrub
Disclaimer: Please review and test code, and use at your own risk... If I answer a question, I would like to hear if and how you solved your problem.
| 