How to Find The Stinky Parts of Your Code [Part XIII]

Yet more code smells? More? Isn’t 13th bad luck?

We see several symptoms and situations that make us doubt the quality of our development.

Let's look at some possible solutions.

Most of these smells are just hints of something that might be wrong. They are not rigid rules.

Previous Parts

Part I

Part II

Part III

Part IV

Part V

Part VI

Part VII

Part VIII

Part IX

Part X

Part XI

Part XII

Let's continue...

Code Smell 61 - Coupling to Classes

Classes are handy. We can call them and invoke them any time. Is this good?

Problems

• Coupling
• Extensibility
• Hard to mock

Solutions

1. Use interfaces or traits (if available).
2. Use Dependency Injection.
3. Favor Loose Coupling.

Sample Code

Wrong

public class MyCollection { 
     public bool HasNext { get; set;} //implementation details 
     public object Next(); ////implementation details 
}

public class MyDomainObject sum(MyCollection anObjectThatCanBeIterated) {
 //Tight coupling 
}

//cannot fake or mock this method since it always expects an instance of MyCollection

Right

public interface Iterator { 
     public bool HasNext { get; set;}
     public object Next();
}

public Iterator Reverse(Iterator iterator) {
    var list = new List<int>();
    while (iterator.HasNext) {
       list.Insert(0, iterator.Next());
    }
    return new ListIterator(list);
}

public class MyCollection implements Iterator { 
     public bool HasNext { get; set;} //implementation details 
     public object Next(); ////implementation details 
}

public class myDomainObject sum(Iterator anObjectThatCanBeIterated) {
 //Loose coupling 
}

//can use any Iterator (even a mocked one as long as it adheres protocol)

Detection

We can use almost any linter to find references to classes. We should not abuse since many uses might be false positives.

Tags

• Coupling

Conclusion

Dependencies to Interfaces make a system less coupled and thus more extensible and testable.

Interfaces change less often than concrete implementations.

Some objects implement many interfaces, declaring which part depends on which interface makes the coupling more granular and the object more cohesive.

More info

Coupling: The one and only problem

Wikipedia

When your code depends on an interface, that dependency is usually very minor and unobtrusive. Your code doesn’t have to change unless the interface changes, and interfaces typically change far less often than the code behind them. When you have an interface, you can edit classes that implement that interface or add new classes that implement the interface, all without impacting code that uses the interface.

For this reason, it is better to depend on interfaces or abstract classes than it is to depend on concrete classes. When you depend on less volatile things, you minimize the chance that particular changes will trigger massive recompilation.

Michael Feathers

Code Smell 62 - Flag Variables

Flags indicate what happened. Unless their name is too generic.

Problems

• Readability
• Maintainability
• Coupling

Solutions

1. Use meaningful names
2. Try to avoid flags. They generate coupling.

Sample Code

Wrong

<?

function dummy() {

    $flag = true;

    while ($flag == true) {

        $result = doSomething();
        if ($result) {
            $flag = false;
        }
    }
}

Right

<?

function dummyFunction()
{
    $atLeastOneElementWasFound = true;

    while (!$atLeastOneElementWasFound) {

        $elementSatisfies = doSomething();
        if ($elementSatisfies) {
            $atLeastOneElementWasFound = true;
        }
    }
}

Detection

We can search all the code for bad-named flags.

Tags

• Readability

Conclusion

Flags are widespread on production code. We should restrict their usage and use clear and intention revealing names.

More Info

Wikipedia

What exactly is a name: part ii rehab]

If you lie to the compiler, it will get its revenge.

Henry Spencer

Code Smell 63 - Feature Envy

If your method is jealous and you don't trust delegation, you should start to do it.

TL;DR: Don't abuse your friend objects.

Problems

• Coupling
• Low Reuse
• Low Testability
• Bad Responsibilities Assignment
• Bijection Fault

Solutions

1. Move method to the appropriate class.

Sample Code

Wrong

class Candidate {

 void printJobAddress(Job job) {

   System.out.println("This is your position address");

   System.out.println(job.address().street());
   System.out.println(job.address().city());
   System.out.println(job.address().ZipCode());
 } 
}

Right

class Job {

 void printAddress() {

   System.out.println("This is your job position address");

   System.out.println(this.address().street());
   System.out.println(this.address().city());
   System.out.println(this.address().ZipCode());
  
  //We might even move this responsability directly to the address !
  //Some address information is relevant to a job and not for package tracking
 } 
}

class Candidate {
  void printJobAddress(Job job) {
    job.printAddress();
  }
}

Detection

Some linters can detect a sequential pattern of collaborations with another object.

Tags

• Coupling

Conclusion

• We should assign responsibilities according to real object mappers and avoid abusing other objects protocol.

More info

• Refactoring Guru
• C2 Wiki
• Wikipedia

We argue that design practices which take a data-driven approach fail to maximize encapsulation because they focus too quickly on the implementation of objects. We propose an alternative object-oriented design method which takes a responsibility-driven approach.

Rebecca Wirfs-Brock

Code Smell 64- Inappropriate Intimacy

Two classes entangled in love.

Problems

• Coupling
• Bad Responsibilities Assignments
• Bad Cohesion
• Class Interfaces too Public
• Maintainability
• Extensibility

Solutions

1. Refactor
2. Merge
3. Replace Hierarchy With Delegation.

Sample Code

Wrong

class Candidate {

 void printJobAddress(Job job) {

   System.out.println("This is your position address");

   System.out.println(job.address().street());
   System.out.println(job.address().city());
   System.out.println(job.address().zipCode());
   
   if (job.address().country() == job.country()) {
        System.out.println("It is a local job");
   } 
}

Right

final class Address {
 void print() {
   System.out.println(this.street);
   System.out.println(this.city);
   System.out.println(this.zipCode);   
 } 
 
 bool isInCounty(Country country){
  return this.country == country;
}

class Job {
 void printAddress() {

   System.out.println("This is your position address");

   this.address().print());
   
   if (this.address().isInCountry(this.country()) {
        System.out.println("It is a local job");
   } 
 } 
}

class Candidate {
  void printJobAddress(Job job) {
    job.printAddress();
  }
}

Detection

Some linters graph class relations and protocol dependency. Analyzing the collaboration graph, we can infer rules and hints.

Tags

• Coupling

Conclusion

If two classes are too related and don't talk much to others, we might need to split, merge or refactor them; Classes should know as little about each other as possible.

More info

C2 wiki

Refactoring guru

thecodebuzz.com

No matter how slow you are writing clean code, you will always be slower if you make a mess.

Code Smell 65 - Variables Named after Types

Names should always indicate role.

Problems

• Declarative
• Design for Change
• Coupling to accidental implementation

Solutions

1. Rename your variable according to the role.

Sample Code

Wrong

public bool CheckIfStringHas3To7LowercaseCharsFollowedBy3or4Numbers(string string)
{
  Regex regex = new Regex(@"[a-z]{2,7}[1-9]{3,4}")
  var hasMatch = regex.IsMatch(string);
  return hasMatch;
}

Right

public bool CheckIfStringHas3To7LowercaseCharsFollowedBy3or4Numbers(string password)
{
  Regex stringHas3To7LowercaseCharsFollowedBy3or4Numbers = new Regex(@"[a-z]{2,7}[1-9]{3,4}")
  var hasMatch = stringHas3To7LowercaseCharsFollowedBy3or4Numbers.IsMatch(password);
  return hasMatch;  
}

Detection

This is a semantic rule. We can instruct our linters to warn us from using names related to existing classes; types o reserved words since they are too implementative.

Tags

• Declarative

Conclusion

The first name we can come across is related to an accidental point of view. It takes time to build a theory on the models we are building using our MAPPERS. Once we get there, we must rename our variables-

More info

Whats is in a name

xunitpatterns.com

Credits

This idea came from this tweet

Types are essentially assertions about a program. And I think it’s valuable to have things be as absolutely simple as possible, including not even saying what the types are.

Dan Ingalls

We are done for some time (not many).

But we are pretty sure we will come across even more smells very soon!

I keep getting more suggestions on Twitter, so they won't be the last!

Send me your own!