slight improvement to _init_os()

[manwar]

Hi @oalders

Just going through the codes for fun, I came across the sub _init_os() and noticed the use of index(). I think, we can make it more readable and maintainable. You can throw away the suggestion in the bin, if you don't like. I feel more comfortable discussing this with you. Hence raising the subject for your input.

My suggestion is to wrap the call to index() to something like is_matching($ua, @os_strings). What this will achieve? First it will clean up sub _init_os() and make it readable. Also less prone to any typo.

The new sub is_matching() would look like something:

  sub is_matching {
        my ($ua, $os_strings) = @_;
        return 0 unless (defined $ua && scalar(@$os_strings));

        foreach my $os_string (@$os_strings) {
             return 1 if index($ua, $os_string) != -1;
        }

        return 0;
  }

I haven't done any performance analysis yet, I must admit.

Best Regards,
Mohammad S Anwar

[oalders]

Hi @manwar,

Thanks for the suggestion. If it made the code cleaner and didn't impact performance, then I would not be opposed to it. It's probably a fairly big job, if mundane. There are a lot of index calls in this module. There are also some && interspersed with || so it would be important not to lose the flow of the logic in the conversion.

Since there are no outstanding pull requests, I don't see a problem with a big change under the hood at this point, so if you feel motivated, feel free to try. Maybe you could show me a beginning stage version so that we can judge how much of an improvement it is before you spend too much time on it?

Best,

Olaf

[manwar]

Hi @oalders

Thanks for go-ahead. I will share the first draft for review soon before pushing.

Best Regards,
Mohammad S Anwar