Refactoring (clean code) #164
Conversation
* Reduces coupling (SOLID principles)
* Broke up assignment from method call * Removing unused import
* assertEquals(expected, actual)
* Returning empty Collections instead of null (Effective Java, item mpatric#43) * Removing unused method parameter
* Always override hashCode when overriding equals
| if (obseleteFormat) { | ||
| return null; | ||
| return Collections.emptyList(); |
There was a problem hiding this comment.
Changing from returning null to returning an empty collection changes the SDK's interface, which might affect applications that are using it. I'd prefer not to make a breaking change unless it's absolutely neccessary.
There was a problem hiding this comment.
You have a valid point. I agree.
- We should probably add a new test that checks that this method implementation returns a null-value. I presumed the tests would fail if breaking changes were made. They all passed.
- Alternatively we could create a new implementation that returns an empty list and mark the old method as
@Deprecated - Also
ArrayListhas theensureCapacity()method which is not defined in theListinterface. This could potentially break something (however unlikely).
There was a problem hiding this comment.
We should probably add a new test that checks that this method implementation returns a null-value. I presumed the tests would fail if breaking changes were made. They all passed.
Done. Please refer to bd77447
This reverts commit 34872c9.
* Increased test coverage. Replaced some home-brew implementations of hashCode/equals with more generic ones and made use of EqualsVerifier. * Increased test coverage. Replaced some home-brew implementations of hashCode/equals with more generic ones and made use of EqualsVerifier. (reverted from commit 580acf5) * Increased code test coverage.
| @@ -958,9 +958,22 @@ public void setUrl(String url) { | |||
| } | |||
|
|
|||
| @Override | |||
| public List<ID3v2ChapterFrameData> getChapters() { | |||
| @Deprecated | |||
There was a problem hiding this comment.
Rationale for deprecating this method is that it couples the return-type to an implementation. Thus encouraging bad coding practices.
References:
How and When to Deprecate APIs - Oracle's Java documentation
* Assert that getChapters returns null when using obsolete format * Assert that getChapterTOC returns null when using obsolete format * Assert that listOfChapters returns an empty list when using the obsolete format * Assert that listOfChapterTOC returns an empty list when using the obsolete format
|
This PR is quite old already and we didn't manage to agree on a version to merge until now. |
Refactoring based on Clean Code (Uncle Bob) and Effective Java (Joshua Bloch):
Listinterface instead ofArrayList(SOLID principles, dependency inversion principle)Listinstead ofnull("Return empty arrays or collections, not nulls", Effective Java)assertEquals(expected, actual)true). Thus useless