roomorama / Caldroid

A better calendar for Android
Other
1.42k stars 531 forks source link

Replaced HashMap method arguments and return values with Map. #356

Closed slavick closed 8 years ago

slavick commented 8 years ago

This change allows alternative implementations of the Map interface (such as android.support.v4.util.ArrayMap) to be used.

The motivation behind wanting to use ArrayMap can be seen in this video: https://youtu.be/ORgucLTtTDI

thomasdao commented 8 years ago

Hi @slavick, thanks for the PR. However although you have changed most of the type to Map, you still initialize caldroidData and extraData with HashMap, so there is no change in runtime. It's also not so convenient for user to switch between HashMap and ArrayMap, and I don't really think it would gain much performance improvement in reality.

slavick commented 8 years ago

Hi @thomasdao, I left the variables initialized to HashMap so there would be no worry about inadvertently changing functionality or runtime performance in the library itself.

The purpose behind my PR was to allow the user to pass Map implementations into methods such as CaldroidFragment.setBackgroundResourceForDates (which is what I am using). By only accepting HashMap arguments, the method is artificially limiting the user's ability to choose a different Map implementation in their code. Changing the argument type to Map allows the user to use either HashMap or ArrayMap or any other class that implements the Map interface. This would only increase the flexibility of the library for developers.

Thanks

gregorko commented 8 years ago

+1 Its not about performance or changes in runtime. Its about following a highly recommended GoF principle for public APIs: "Program to an interface, not an implementation". The API only needs to know it will get a Map passed that it can expect a certain behavior from. It doesn't care what kind of implementation it is, only that it supports the behavior needed. The method caller can then implement the required behavior in any desired way.

thomasdao commented 8 years ago

that makes sense, I will push soon together with a few more small fixes