Code smells that indicate you might need DI

The saying to a man with only a hammer, every problem looks like a nail is old and yet is never truer than in programming. As professionals, we should be continually striving to acquire more tools to be better equipped for whatever our job throws at us. DI, while a highly useful tool, is useful only for particular nails. In our case, these nails are code smells. Code smells are indications in the code of a potentially deeper problem.

There are many different types of code smell; in this section, we will examine only those that can be alleviated by DI. In later chapters, we will reference these smells as we attempt to remove them from our code.

Code smells generally fall into four different categories:

• Code bloat
• Resistance to change
• Wasted effort
• Tight coupling

Code bloat

Code bloat smells are cases where unwieldy slabs of code have been added to structs or functions so that they have become hard to understand, maintain, and test. Frequently found in older code, they are often the result of a gradual degradation and lack of maintenance rather than intentional choices.

They can be found with a visual scan of the source code or by employing a cyclomatic complexity checker (a software metric that indicates the complexity of a piece of code) such as gocyclo (https://github.com/fzipp/gocyclo).

These smells include the following:

• Long methods: While the code is run on computers, it is written for humans. Any method of more than about 30 lines should be split into smaller chunks. While it makes no difference to the computer, it makes it easier for us humans to understand.
• Long structs: Similar to long methods, the longer a struct, the harder it is to understand and therefore maintain. Long structs typically also indicate the struct is doing too much. Splitting one struct into several smaller ones is also a great way to increase the reusability potential of the code.

• Long parameter lists: Long parameter lists also indicate that the method is likely doing more than it should. When adding new features, it is tempting to add a new parameter to an existing function to account for the new use case. This is a slippery slope. This new parameter is either optional/unnecessary for the existing use cases or is an indication of a significant increase in complexity in the method.
• Long conditional blocks: Switch statements are amazing. The problem is they are very easy to abuse and tend to multiply like proverbial rabbits. Perhaps the most significant problem, however, is their effect on the readability of the code. Long conditional blocks take up a lot of space and interrupt the readability of the function. Consider the following code:

func AppendValue(buffer []byte, in interface{}) []byte{
   var value []byte

   // convert input to []byte
   switch concrete := in.(type) {
   case []byte:
      value = concrete

   case string:
      value = []byte(concrete)

   case int64:
      value = []byte(strconv.FormatInt(concrete, 10))

   case bool:
      value = []byte(strconv.FormatBool(concrete))

   case float64:
      value = []byte(strconv.FormatFloat(concrete, 'e', 3, 64))
   }

   buffer = append(buffer, value...)
   return buffer
}

By taking interface{} as input, anywhere we wish to use it, we are almost forced to have a switch like this one. We would be better off changing from interface{} to an interface and then adding the necessary operations to the interface. This approach is better illustrated by the json.Marshaller and driver.Valuer interfaces in the standard library.

Applying DI to these smells will typically reduce the complexity of individual pieces of code by breaking them into smaller, separate pieces, which in turn makes them easier to understand, maintain, and test.

Resistance to change

These are cases where it is difficult and/or slow to add new features. Similarly, tests are often harder to write, especially tests for failure conditions. Similar to code bloat, these smells can be the result of a gradual degradation and lack of maintenance, but they can also be caused by a lack of up-front planning or poor API design.

They can be found by examining the pull request log or commit history and, in particular, determining if new features require many small changes in different parts of the code. 
If your team tracks feature velocity and you notice it is declining, this is also a likely cause.

These smells include the following:

• Shotgun surgery: This is when small changes made to one struct necessitate changes in other structs. These changes imply that the organisation or abstraction used was incorrect. Typically, all of these changes should be in one class.
  In the following example, you can see how adding an email field to the person data would result in changing all three structs (Presenter, Validator, and Saver):

// Renderer will render a person to the supplied writer
type Renderer struct{}

func (r Renderer) render(name, phone string, output io.Writer) {
  // output the person
}

// Validator will validate the supplied person has all the 
// required fields
type Validator struct{}

func (v Validator) validate(name, phone string) error {
  // validate the person
  return nil
}

// Saver will save the supplied person to the DB
type Saver struct{}

func (s *Saver) Save(db *sql.DB, name, phone string) {
  // save the person to db
}

• Leaking implementation details: One of the more popular idioms in the Go community is accept interfaces, return structs. It's a catchy turn of phrase, but its simplicity masks its cleverness. When a function accepts a struct, it ties the user to a particular implementation—a strict relationship that makes future changes or additional usage difficult. By extension, if that implementation detail were to change, the API changes and forces changes on its users.

Applying DI to these smells is typically a good investment in the future. While not fixing them is not fatal, the code will progressively degrade until you are dealing with the proverbial big ball of mud. You know the type—a package that no-one understands, no-one trusts, and only the brave or stupid are willing to make changes to. DI enables you to decouple from the implementation choices, thereby making it easier to refactor, test, and maintain small chunks of code in isolation.

Wasted effort

These smells are cases where the cost to maintain the code is higher than it needs to be. They are typically caused by laziness or lack of experience. It's always easier to copy/paste code than to carefully refactor it. The problem is, coding like this is like eating unhealthy snacks. It feels great in the moment, but the long-term consequences suck.

They can be found by taking a critical look at the source code and asking yourself do I really need this code? Or, can I make this easier to understand?

Using tools such as dupl (https://github.com/mibk/dupl) or PMD (https://pmd.github.io/) will also help you identify areas of the code to investigate.

These smells include the following:

• Excessive duplicated code: Firstly, please, please do not become a zealot about this one. While in most cases, duplicated code is a bad thing, sometimes copying code can result in a system that is easier to maintain and can evolve. We will deal with a common source of this smell in Chapter 8, Dependency Injection by Config.
• Excessive comments: Leaving a note for those that come after you, even it is only you 6 months from now, is a friendly and professional thing to do. But when that note becomes an essay, then it's time to refactor:

// Excessive comments
func outputOrderedPeopleA(in []*Person) {
  // This code orders people by name.
  // In cases where the name is the same, it will order by 
  // phone number.
  // The sort algorithm used is a bubble sort
  // WARNING: this sort will change the items of the input array
  for _, p := range in {
    // ... sort code removed ...
  }

  outputPeople(in)
}

// Comments replaced with descriptive names
func outputOrderedPeopleB(in []*Person) {
  sortPeople(in)
  outputPeople(in)
}

• Overly complicated code: The harder code is for other people to understand, the worse it is. Typically, this is the result of someone trying to be too fancy or not putting enough effort into structure or naming. Taking a more selfish view, if you are the only one who understands a piece of code, you are the only one that can work on it. Meaning, you are doomed to maintain it forever. What does the following code do:

for a := float64(0); a < 360; a++ {
   ra := math.Pi * 2 * a / 360
   x := r*math.Sin(ra) + v
   y := r*math.Cos(ra) + v
   i.Set(int(x), int(y), c)
}

• DRY/WET code: The Don't Repeat Yourself (DRY) principle is aimed at reducing duplicated efforts by grouping responsibilities together and providing clean abstractions. By contrast, in WET code, sometimes called Waste Everyone's Time code, you will find the same responsibility in many places. This smell often appears in formatting or conversion code. This sort of code should exist at the system boundaries, that is, converting user input or formatting output.

While many of these smells can be fixed without DI, DI provides an easier way to lift and shift the duplication into an abstraction that can then be used to reduce the duplication and improve the readability and maintainability of the code.

Tight coupling

For people, tight coupling might be a good thing. For Go code, it's really not. Coupling is a measure of how objects relate to or depend on each other. When the tight coupling is present, this interdependence forces the objects or packages to evolve together, adding complexity and maintenance costs.

Coupling-related smells are perhaps the most insidious and obstinate but by far the most rewarding when dealt with. They are often the result of a lack of object-oriented design or insufficient use of interfaces.

Sadly, I don't have a handy tool to help you find these smells but I am confident that, by the end of this book, you will have no trouble spotting and dealing with them.

Frequently, I find it useful to implement a feature in a tightly coupled form first and then work backward to decouple and thoroughly unit test my code before submitting it. For me, it is especially helpful in cases where the correct abstractions are not obvious.

These smells include the following:

• Dependence on God objects: These are large objects that know too much or do too much. While this is a general code smell and something that should be avoided like the plague, the problem from a DI perspective is that too much of the code is dependent on this one object. When they exist and we are not careful, it won't be long before Go will be refusing to compile due to a circular dependency. Interestingly, Go considers dependencies and imports not at an object level but at a package level. So we have to avoid God packages as well.  We will address a very common God object problem in Chapter 8, Dependency Injection by Config.
• Circular dependencies: These are where package A depends on package B, and package B depends on package A. This is an easy mistake to make and sometimes a hard one to get rid of.

In the following example, while the config is arguably a God object and therefore a code smell, I am hard pressed to find a better way to import the config from a single JSON file. Instead, I would argue that the problem to be solved is the use of the config package by orders package. A typical config God object follows:

package config

import ...

// Config defines the JSON format of the config file
type Config struct {
   // Address is the host and port to bind to.  
   // Default 0.0.0.0:8080
   Address string

   // DefaultCurrency is the default currency of the system
   DefaultCurrency payment.Currency
}

// Load will load the JSON config from the file supplied
func Load(filename string) (*Config, error) {
   // TODO: load currency from file
   return nil, errors.New("not implemented yet")
}

In the attempted usage of the config package, you can see that the Currency type belongs to the Package package and so including it in config, as shown in the preceding example, causes a circular dependency:

package payment

import ...

// Currency is custom type for currency
type Currency string

// Processor processes payments
type Processor struct {
   Config *config.Config
}

// Pay makes a payment in the default currency
func (p *Processor) Pay(amount float64) error {
   // TODO: implement me
   return errors.New("not implemented yet")
}

• Object orgy: These occur when an object has too much knowledge of and/or access to the internals of another or, to put it another way, insufficient encapsulation between objects. Because the objects are joined at the hip, they will frequently have to evolve together, increasing the cost of understanding the code and maintaining it. Consider the following code:

type PageLoader struct {
}

func (o *PageLoader) LoadPage(url string) ([]byte, error) {
   b := newFetcher()

   // check cache
   payload, err := b.cache.Get(url)
   if err == nil {
      // found in cache
      return payload, nil
   }

   // call upstream
   resp, err := b.httpClient.Get(url)
   if err != nil {
      return nil, err
   }
   defer resp.Body.Close()

   // extract data from HTTP response
   payload, err = ioutil.ReadAll(resp.Body)
   if err != nil {
      return nil, err
   }

   // save to cache asynchronously
   go func(key string, value []byte) {
      b.cache.Set(key, value)
   }(url, payload)

   // return
   return payload, nil
}

type Fetcher struct {
   httpClient http.Client
   cache      *Cache
}

In this example, PageLoader repeatably calls the member variable of the Fetcher. So much so that, if the implementation of Fetcher changed, it's highly likely that PageLoader would be affected. In this case, these two objects should be merged together as PageLoader has no extra functionality.

• Yo-yo problem: The standard definition of this smell is when the inheritance graph is so long and complicated that the programmer has to keep flipping through the code to understand it. Given that Go doesn't have inheritance, you would think we would be safe from this problem. However, it is possible if you try hard enough, with excessive composition. To address this issue, it's better to keep relationships as shallow and abstract as possible. In this way, we can concentrate on a much smaller scope when making changes and compose many small objects into a larger system.
• Feature envy: When a function makes extensive use of another object, it is envious of it. Typically, an indication that the function should be moved away from the object it is envious of. DI may not be the solution to this, but this smell does indicate high coupling and, therefore, is an indicator to consider applying DI techniques:

func doSearchWithEnvy(request searchRequest) ([]searchResults, error) {
   // validate request
   if request.query == "" {
      return nil, errors.New("search term is missing")
   }
   if request.start.IsZero() || request.start.After(time.Now()) {
      return nil, errors.New("start time is missing or invalid")
   }
   if request.end.IsZero() || request.end.Before(request.start) {
      return nil, errors.New("end time is missing or invalid")
   }

   return performSearch(request)
}

func doSearchWithoutEnvy(request searchRequest) ([]searchResults, error) {
   err := request.validate()
   if err != nil {
      return nil, err
   }

   return performSearch(request)
}

As your code becomes less coupled, you will find the individual parts (packages, interfaces, and structs) will become more focused. This is referred to as having high cohesion. Both low coupling and high cohesion are desirable as they make the code easier to understand and work with.