If it walks like a duck and it quacks like a duck, then it must be a duck
TL;DR: Don't create unnecessary abstractions
Problems
Over Design
Unneeded classes
Solutions
- Use a standard class
Context
Discovering abstractions on the MAPPER is a hard task.
After refining we should remove unneeded abstractions.
Sample Code
Wrong
<?php
Namespace Spelling;
final class Dictionary {
private $words;
function __construct(array $words) {
$this->words = $words;
}
function wordsCount(): int {
return count($this->words);
}
function includesWord(string $subjectToSearch): bool {
return in_array($subjectToSearch, $this->words);
}
}
//This has protocol similar to an abstract datatype dictionary
//And the tests
use PHPUnit\Framework\TestCase;
final class DictionaryTest extends TestCase {
public function test01EmptyDictionaryHasNoWords() {
$dictionary = new Dictionary([]);
$this->assertEquals(0, $dictionary->wordsCount());
}
public function test02SingleDictionaryReturns1AsCount() {
$dictionary = new Dictionary(['happy']);
$this->assertEquals(1, $dictionary->wordsCount());
}
public function test03DictionaryDoesNotIncludeWord() {
$dictionary = new Dictionary(['happy']);
$this->assertFalse($dictionary->includesWord('sadly'));
}
public function test04DictionaryIncludesWord() {
$dictionary = new Dictionary(['happy']);
$this->assertTrue($dictionary->includesWord('happy'));
}
}
Right
<?php
Namespace Spelling;
// final class Dictionary is no longer needed
//The tests use a standard class
//In PHP we use associative arrays
//Java an other languages have HashTables, Dictionaries etc. etc.
use PHPUnit\Framework\TestCase;
final class DictionaryTest extends TestCase {
public function test01EmptyDictionaryHasNoWords() {
$dictionary = [];
$this->assertEquals(0, count($dictionary));
}
public function test02SingleDictionaryReturns1AsCount() {
$dictionary = ['happy'];
$this->assertEquals(1, count($dictionary));
}
public function test03DictionaryDoesNotIncludeWord() {
$dictionary = ['happy'];
$this->assertFalse(in_array('sadly', $dictionary));
}
public function test04DictionaryIncludesWord() {
$dictionary = ['happy'];
$this->assertTrue(in_array('happy', $dictionary));
}
}
Detection
[X] SemiAutomatic
Based on protocols, we should remove some unnecessary classes
Tags
- Protocols
Exceptions
Sometimes we need to optimize collections for performance reasons if we have enough strong evidence.
Conclusion
We need to clean up code from time to time.
Specialized collections are a good starting point.
Relations
More Info
Credits
Photo by Pisit Heng on Unsplash
Most of the effort in the software business goes into the maintenance of code that already exists.
Wietse Venema
This article is part of the CodeSmell Series.
Top comments (11)
Actually,.. there's a mix of two different things.
1) Should we use "Specialized Business Collections"? - Yes, please. Do it whenever you can. Do not use strip PHP associative arrays, do not use Java standard collection interfaces like List<>, Map<>, Set<> etc.
2) Should we reimplement something? ;) Probably, not. Please do not reimplement TreeMap or ArrayList without really good reason for that.
IMHO
1) Not, why?
Specialized Collection do not bring additional benefits unless we have strong evidence.
2) Until we find differential responsibilities
1) The most valuable benefit - it's extending your dsl. The same purpose like Value Objects in DDD - we can use UUID anywhere, but we want to wrap it with some WalletId class. I want to clearly distinguish in my code both list of strings - Dictionary and MyNames. I do not think anybody will appreciate something like List < Map < String, Map < Integer, String>>> :))
Extension should be made when protocol is different.
Dictionary and MyNames might be primitive obsession code smell. But this is the case when we need specialized behaviour. The quest is to find this behavior
Pardon, don't get it. What protocol do you mean?
Protocol is the set of functions an object understands, AKA behavior
Is the only important thing we need to know in order to program
Aha, thanks.
I tend to generalize it a bit - the only important thing is the Domain. So, if we have somewhere in our domain "MyNames" thing - we should reflect it in our code. Even if it will be just one line, like
public interface MyNames extends List {}
(of course, it's better to have
public interface MyNames extends List {} )
Protocol extension could come after that, but not have to. The better code should looks like domain expert speech, and there's no lists of strings ;).
In a situation like the one you gave as an example, the tests aren't needed - they're verifying that stuff works that's natively part of the language. I know it's a simple example, but if you don't add anything, why would you need the tests? Or the namespace for that matter? Wouldn't the "Good" variant be nothing at all?
That is the point !!
What is an abstraction?
Something you see in real world and create to mimic its behavior