slight improvement to _init_os()
manwar opened this issue · 2 comments
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
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