Apparently I also need a hint with the readmore tag.

<readmore>
<c>
use strict;
</c>
</readmore>
[download]

Thanks, that helped with the readmore tag.

The other problem seems to be more a problem of the object design. With

$self->{'countries'}->{$country_code}->{'fullname'}= $country;
$self->{'countries'}->{$country_code}->{'warnings'} = \%warn;
return $self->{'countries'};
[download]

I am able to return just the wanted values without deleting any other properties of the object.
Are there any other ways to return just the wanted value?

Invoke laziness first: is there any reason I need to delete the other properties? Or will providing a wanted_value method (obviously a better name is needed!) suffice?

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:
  1. Keeps a lookup of filenames for images
  2. Retrieves $self and stores the rest of the parameters in $self as hash keys
  3. Builds a URL and fetches it with LWP
  4. splits the result on <a href=
  5. Checks each element of the split for some ID codes and stores something in $self based on looking up in the image hash
  6. deletes the keys user_agent, day and type from $self
  7. 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;
}
[download]

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

Thanks a lot FalseVinylShrub and ikegami for your detailed answers!!

There is no need to keep the method options in the object. So, as you pointed, it is a good idea to keep them in a variable. Since I want to distinguish between fullname and warnings I store this values in a hash

$country_warnings->{$country_code}->{'fullname'} =$country;
$country_warnings->{$country_code}->{'warnings'} = \%warn;
return $country_warnings;
[download]

Doing this gets me a return value that I want: MeteoalarmCountry::countries->{fullname} and ...:countries->{warnings}. I have no need to keep this values in the object.
And you are perfectly right it is also a good idea to check if $country_code is defined before assigning values to this key.

and expect a hashref of the return value.

Then what are you doing returning an object ($self)? I'm not clear on what your object is suppose to do. Tell us about your object and what the referenced hash should contain and we'll help you further.

After looking some more, I think my approach would be something like

{
   package Meteoalarm;

   sub new {
      my ($class, %args) = @_;
      my $self = bless({}, $class);
      $self->{ua} = _make_ua();
      return $self;
   }

   sub _make_ua {
      return LWP::UserAgent->new(
         ...
      );
   }

   sub get_warnings {
      my ($self) = @_;
      my $ua = $self->{ua};

      ...

      my %by_country;
      for (...) {
         ...
         $by_country{$country} = \%warning;
      }

      return \%by_country;
   }
}
[download]

{
   package Meteoalarm;

   sub new {
      my ($class, %args) = @_;
      my $self = bless({}, $class);
      $self->{ua} = _make_ua();
      return $self;
   }

   sub _make_ua {
      return LWP::UserAgent->new(
         ...
      );
   }

   sub fetch {
      my ($self) = @_;
      my $ua = $self->{ua};

      ...

      my @countries;
      for (...) {
         ...
         push @countries, Meteoalarm::Country->new(
            name     => $country,
            warnings => \%warnings,
         );
      }

      return \@countries;
   }
}

{
   package Meteoalarm::Country;

   sub new {
      my ($class, %args) = @_;
      my $self = bless({}, $class);
      $self->{name    } = $args{name    };
      $self->{warnings} = $args{warnings};
      return $self;
   }

   sub name     { shift->{name    } }
   sub warnings { shift->{warnings} }
}
[download]