More code smells. No joke.
Let’s look at some possible solutions.
From our partners:
Most of these smells are just hints of something that might be wrong. They are not rigid rules.
This is part V. Part I can be found here, Part II here, Part III is here, Part IV here, part V, VI, VII, VIII and IX.
Let’s continue…
Code Smell 46 — Repeated Code
DRY is our mantra. Teachers tell us to remove duplication. We need to go beyond.
Problems
- Code Duplication
- Maintainability
- Don’t Repeat Yourself
Solutions
- Find repeated patterns (not repeated code).
- Create an abstraction.
- Parametrize abstraction calls.
- Use composition and never inheritance.
- Unit test new abstraction.
Sample Code
Wrong
<?
final class WordProcessor {
function replaceText(string $patternToFind, string $textToReplace) {
$this->text = '<<<' . str_replace($patternToFind, $textToReplace, $this->text) . '>>>';
}
}
final class Obfuscator {
function obfuscate(string $patternToFind, string $textToReplace) {
$this->text = strlower(str_ireplace($patternToFind, $textToReplace, $this->text));
}
}
Right
<?
final class TextReplacer {
function replace(string $patternToFind, string $textToReplace, string $subject, string $replaceFunctionName, $postProcessClosure) {
return $postProcessClosure($replaceFunctionName($patternToFind, $textToReplace, $subject));
}
}
//Lots of tests on text replacer so we can gain confidence.
final class WordProcessor {
function replaceText(string $patternToFind, string $textToReplace) {
$this->text = (new TextReplacer())->replace($patternToFind, $textToReplace, $this->text, 'str_replace', fn($text) => '<<<' . $text . '>>>');
}
}
final class Obfuscator {
function obfuscate(string $patternToFind, string $textToReplace) {
$this->text = (new TextReplacer())->replace($patternToFind, $textToReplace, $this->text, 'str_ireplace', fn($text) => strlower($text));
}
}
Detection
Linters can find repeated code.
There are not very good finding similar patterns.
Maybe soon machine learning will help us find such abstractions automatically.
For now, it is up to us, humans.
Examples
Tags
- Duplication
Conclusion
Repeated code is always a smell.
Copying and pasting code is always a shame.
With our refactoring tools, we need to accept the duplication remove challenge trusting our tests as a safety net.
More info
Copy and paste is a design error.
– David Parnas
Code Smell 47 — Diagrams
Diagrams are not code. They cannot be a code smell. They are just Diagram Smells.
Problems
- Maintainability
- Trash code
- Code Duplication
- Diagrams focus only on structure (accidental) and not behavior (essential).
Solutions
- Use diagrams only to communicate ideas with other humans.
- Program on your favorite IDE.Thrash all diagrams.
- Even the ones generated from the source code.
- Trust your tests. They are alive and well maintained.
- Use Domain Driven Design technique.
Sample Code
Wrong
Right
w<?
final class BookItem {
//No. We will not inherit from concrete class Book since this is another smell
//No. We will not list a bunch of anemic attributes. since yet this is another smell
function numberOfPages() {
//
}
function language(): Language {
//
}
function book(): Book {
//Book can tell us a lot about her/his author, title etc.
}
function edition(): BookEdition {
//..
}
//Loan and overdues are not book items responsibility
}
final class LoanTracker {
function loan(BookItem $bookCopy, LibraryUser $reader, DatePeriod $loanDates) {
//DatePeriod is better than anemic $fromDate and $toDate
}
}
final class LoanTrackerTests extends TestCase {
//Lots of maintained tests telling us how the system really works
}
Detection
We can remove all code annotations and forbid them by policy.
Examples
Conclusion
Designing is a contact sport. We need to prototype and learn from our running models.
Papers and JPGs don’t run. They live in the utopic world where everything works smoothly.
CASE was a very hot trend back in the 90s. No good system was developed with these tools.
More info
The Diagram is Not the Model. The model is not the diagram. It is an abstraction, a set of concepts and relationships between them.
– Eric Evans
Code Smell 48 — Code Without Standards
Working on a solo project is easy. Unless you go back to it after some months. Working with many other developers requires some agreements.
Problems
- Maintainability
- Readability
Solutions
- Automate your styles and indentation.
- Enforce agreed policies.
Sample Code
Wrong
Correct sample taken from Sandro Mancuso’s bank kata
package org.craftedsw.domain;
import static org.craftedsw.domain.Amount.amountOf;
import java.io.PrintStream;
import java.util.Date;
public class MY_Account {
//Some class have different case, underscores
private Statement privStatement;
//Attributes have visibility prefixes
private Amount currentbalance = amountOf(0);
public SetAccount(Statement statement) {
this.statement = statement;
}
//Setters and getters are not normalized
public GiveAccount(Statement statement)
{ this.statement = statement; }
//Indentation is not uniform
public void deposit(Amount value, Date date) {
recordTransaction(value, date);
//some variables are named after type and not role.
}
public void extraction(Amount value, Date date) {
recordTransaction(value.negative(), date);
//the opposite of *deposit* should be withdrawal
}
public void voidPrintStatement(PrintStream printer)
{
statement.printToPrinter(printer);
//Name is redundant
}
private void privRecordTransactionAfterEnteredthabalance(Amount value, Date date) {
Transaction transaction = new Transaction(value, date);
Amount balanceAfterTransaction = transaction.balanceAfterTransaction(balance);
balance = balanceAfterTransaction;
statement.addANewLineContainingTransation(transaction, balanceAfterTransaction);
//naming is not uniform
}
}
Right
The right example has several other smells, but we keep it loyal to its GIT version in order to show only code standardization issues.
package org.craftedsw.domain;
import static org.craftedsw.domain.Amount.amountOf;
import java.io.PrintStream;
import java.util.Date;
public class Account {
private Statement statement;
private Amount balance = amountOf(0);
public Account(Statement statement) {
this.statement = statement;
}
public void deposit(Amount value, Date date) {
recordTransaction(value, date);
}
public void withdrawal(Amount value, Date date) {
recordTransaction(value.negative(), date);
}
public void printStatement(PrintStream printer) {
statement.printTo(printer);
}
private void recordTransaction(Amount value, Date date) {
Transaction transaction = new Transaction(value, date);
Amount balanceAfterTransaction = transaction.balanceAfterTransaction(balance);
balance = balanceAfterTransaction;
statement.addLineContaining(transaction, balanceAfterTransaction);
}
}
Detection
Linters and IDEs should test coding standards before a merge request is approved.
We can add our own naming conventions related to Objects, Classes, Interfaces, Modules etc.
Examples
Tags
- Standardization
Conclusion
Use coding standards in your projects.
A well-written clean code always follows standards about naming conventions, formatting and code style.
Such standards are helpful because they make things clear and deterministic for the ones who read your code, including yourself.
Code styling should be automatic and mandatory on large organizations to enforce Collective Ownership.
More info
The nice thing about standards is that there are so many to choose from.
– Andrew S. Tannenbaum
Code Smell 49 — Caches
Caches are sexy. They are a one-night stand. We need to avoid them in a long-term relationship.
Problems
- Coupling
- Testability
- Cache invalidation.
- Maintainability
- Premature Optimization
- Erratic Behavior
- Lack of transparency
- Non-Deterministic behavior
Solutions
- If you have a conclusive benchmark and are willing to pay for some coupling: Put an object in the middle.
- Unit test all your invalidation scenarios. Experience shows we face them in an incremental way.
- Look for a real world cache metaphor and model it.
Sample Code
Wrong
<?
final class Book {
private $cachedBooks;
public function getBooksFromDatabaseByTitle(string $title) {
if (isset($cachedBooks[$title])) {
return $cachedBooks[$title];
} else {
return $this->doGetBooksFromDatabaseByTitle($title);
}
}
private function doGetBooksFromDatabaseByTitle(string $title) {
globalDatabase()->selectFrom('Books', 'WHERE TITLE = ' . $title);
}
}
Right
<?
final class Book {
//Just Book related Stuff
}
interface BookRetriever {
public function bookByTitle(string $title);
}
final class DatabaseLibrarian implements BookRetriever {
public function bookByTitle(string $title) {
//Go to the database (not global hopefully)
}
}
final class HotSpotLibrarian implements BookRetriever {
//We always look for real life metaphors
private $inbox;
private $realRetriever;
public function bookByTitle(string $title) {
if ($this->inbox->includesTitle($title)) {
//We are lucky. Someone has just returned the book copy.
return $this->inbox->retrieveAndRemove($title);
} else {
return $this->realRetriever->bookByTitle($title);
}
}
}
Detection
This is a design smell.
It will be difficult to enforce by policy.
Tags
- Premature Optimization
Conclusion
Caches should be functional and intelligent.
In this way we can manage invalidation.
General purpose caches are suitable only for low level objects like operating systems, files and streams.
We shouldn’t cache domain objects.
This page is hosted on a cached website.
More Info
There are only two hard things in Computer Science: cache invalidation and naming things.
– Phil Karlton
Code Smell 50 — Object Keys
Primary keys, IDs, references. The first attribute we add to our objects. They don’t exist in the real world.
Problems
- Coupling
- Accidental Implementation
- Bijection Principle Violation.
Solutions
- Reference object to objects.
- Build a MAPPER.
- Only use keys if you need to provide an external (accidental) reference. Databases, APIs, Serializations.
- Use dark keys or GUIDs when possible.
- If you are afraid of getting a big relation graph use proxies or lazy loading.
- Don’t use DTOs.
Sample Code
Wrong
class Teacher {
static getByID(id) {
//go to the coupled database
}
constructor(id, fullName) {
this.id = id;
this.fullName = fullName;
}
}
class School {
static getByID(id) {
//go to the coupled database
}
constructor(id, address) {
this.id = id;
this.address = address;
}
}
class Student {
constructor(firstName, lastName, id, teacherId, schoolId) {
this.firstName = firstName;
this.lastName = lastName;
this.id = id;
this.teacherId = teacherId;
this.schoolId = schoolId;
}
school() {
return School.getById(this.schoolId);
}
teacher() {
return Teacher.getById(this.teacherId);
}
}
Right
class Teacher {
constructor(fullName) {
this.fullName = fullName;
}
}
class School {
constructor(address) {
this.address = address;
}
}
class Student {
constructor(firstName, lastName, teacher, school) {
this.firstName = firstName;
this.lastName = lastName;
this.teacher = teacher;
this.school = school;
}
}
//If need to expose a School to an external API or a Database. Another object (not school) will keep the mapping externalId<->school and so on
Detection
This is a design policy.
We can enforce business objects to warn us if we define an attribute or function including the sequence id.
Tags
- Accidental
Conclusion
Ids are not necessary for OOP. You reference objects (essential) and never ids (accidental).
In case you need to provide a reference out of your system’s scope (APIs, interfaces, Serializations) use dark and meaningless IDs (GUIDs).
More info
The nice thing about standards is that there are so many to choose from.
This article is republished from hackernoon.com
For enquiries, product placements, sponsorships, and collaborations, connect with us at [email protected]. We'd love to hear from you!
Our humans need coffee too! Your support is highly appreciated, thank you!
