DEV Community

Cover image for Code Smell 134 - Specialized Business Collections
Maxi Contieri
Maxi Contieri

Posted on • Originally published at maximilianocontieri.com

Code Smell 134 - Specialized Business Collections

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

  1. 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'));
    }
} 

Enter fullscreen mode Exit fullscreen mode

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));
    }
} 

Enter fullscreen mode Exit fullscreen mode

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.

Discussion (10)

Collapse
gorynych profile image
Stepan Mozyra

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.

Collapse
mcsee profile image
Maxi Contieri Author

IMHO

1) Not, why?
Specialized Collection do not bring additional benefits unless we have strong evidence.

2) Until we find differential responsibilities

Collapse
gorynych profile image
Stepan Mozyra • Edited on

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>>> :))

Thread Thread
mcsee profile image
Maxi Contieri Author

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

Thread Thread
gorynych profile image
Stepan Mozyra

Pardon, don't get it. What protocol do you mean?

Thread Thread
mcsee profile image
Maxi Contieri Author

Protocol is the set of functions an object understands, AKA behavior

Is the only important thing we need to know in order to program

Thread Thread
gorynych profile image
Stepan Mozyra

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 ;).

Collapse
moopet profile image
Ben Sinclair

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?

Collapse
mcsee profile image
Maxi Contieri Author

That is the point !!