search

payprop / html-googlemaps-v3

Fork of HTML::GoogleMaps to fix issues
2 stars 1 forks source link

Simplified sub map_type(). #24

Closed manwar closed 5 years ago

manwar commented 5 years ago

Hi @leejo

Please review the PR.

Many Thanks. Best Regards, Mohammad S Anwar

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.07%) to 94.737% when pulling 86cb2386b866ead660df5aa1142d4518ca3bfbd4 on manwar:tidy-up-map-type into 2c6cf29dc08073c2682b01050f694d7d99886a55 on Humanstate:master.

leejo commented 5 years ago

Hey @manwar - i don't see this hash lookup being used elsewhere, so i'm not sure the reason to refactor it out and turn it into a global variable?

manwar commented 5 years ago

I agree it is not used anywhere else but in my humble opinion, it appears to be a CONSTANT and being CONSTANT, it should be declare at the top and reference where it is needed. It also de-couple sub map_type() from the CONSTANT. In future, if we ever add/remove anything from the list, you just update the list without ever touching the method itself.

I understand where you are coming from, it is not adding any improvement as such except making code little more readable, in my opinion.

Feel free to dump it, if it doesn't suit your idea :-)

leejo commented 5 years ago

If the hash lookup were used elsewhere I would agree with you that it's a constant, but it's only used in the one place as subroutine argument validation. There is nothing else currently using it.

IMO this actually reduces the readability of the code as a reader now has to reference a hash outside the subroutine, a couple of hundred lines above it. Given the subroutine in question is only a dozen lines long this feels like it isn't necessary.

I'm not adverse to changes that increase readability BTW, it's just that I don't feel this is one of those changes. I'm more concerned (at least I have become more concerned in recent years) about the history of a repo, namely bisects, blames, and lineage.