I’ve been working with legacy codebases for many years. And just now, I’ve started “Refactoring” for the amazing Martin Fowler. That inspired me to write about my refactoring endeavors in the codebases I worked with

In this post, I mention some examples from codebases I encountered through the years. This is part #1 from this series

#1 - Too many methods

One thing that can be abused is breaking a method into many different functions. The justification some devs use is that the method has too many lines.

In some cases, that can make sense. But in most cases, it’s the same code divided into different functions, and the worst part is when spanned methods are not being used but by the original method.

Here’s an example:

def process_data(data)
  step1(data)
  step2(data)
  step3(data)
  # ... other steps
end

def step1(data)
  # ... logic for step1
end

def step2(data)

  # ... logic for step2
end

def step3(data)

  # ... logic for step3
end

In many cases, `process_data` is not too long. We can encapsulate all the logic in `process_data` and that can improve the clarity and make it easier to read/onboard other engineers on

The worst thing you can have is a cluttered class with many methods being used to serve in only one place

def process_data(data)
  # Logic for step1
  # ...
  # Logic for step2
  # ...
  # Logic for step3
  # ...

  # ... logic for other steps
end

If `process_date` method is too large. Think about where the logic inside it belongs to. Maybe some logic belongs to a client; maybe others belong to another domain object.

Just remember that spanning new methods because you can is like swiping the dust under the carpet

#2- Unnecessary dependency

Exposing unneeded information to a method/class/domain creates a dependency when it’s unnecessary.

Here’s an example of a simple method:

class User
  attr_accessor :first_name, :last_name, :email, :age

  def initialize(first_name, last_name, email, age)
    @first_name = first_name
    @last_name = last_name
    @email = email
    @age = age
  end
end

def display_full_name(user)
  puts "#{user.first_name} #{user.last_name}"
end

user = User.new("John", "Doe", "john.doe@example.com", 30)
display_full_name(user)

You can see in `display_full_name` we are passing the `user` object, and now we’re dependent on the object structure (i.e. `first_name` can be changed to something else)

To remove this dependency, pass the information you need from the object. Keeping this as a habit makes us more aware of the dependency, and not exposing domain knowledge when it can be avoided:

def display_full_name(first_name, last_name)
  puts "#{first_name} #{last_name}"
end

user = User.new("John", "Doe", "john.doe@example.com", 30)
display_full_name(user.first_name, user.last_name)

#3- Coupling and inflexibility

If we integrate with a payment service to collect payments/charge customers. I’ve seen things like this many times:

class PaymentClient
  def process_payment(user, amount)
    paypal = PayPalService.new
    response = paypal.charge(user.paypal_account, amount)
    if response.success?
      # Do something on success...
    else
      # Handle error...
    end
  end
end

class PayPalService
  def charge(account, amount)
    # Logic to charge with PayPal API...
    # Returns a response object...
  end
end

Some things are not quite right with this code:

• `PayPalService` is quite coupled with the client. meaning that if we switch to another payment provider, we will need to update `PaymentClient`

• If we need to add another Payment Client (without switching), there’s still quite a code change that will need to happen to adapt that in our client.

• Logic can be entirely coupled and complex as we add more features (refund, scheduled payments, etc…)

Here’s the refactored version of this code. In that code, we removed the coupling, created an interface, and used dependency injection to add/remove as many payment services as we wanted, and it won’t affect the client implementation.

# Abstract payment service interface
module PaymentService
  def charge(account, amount)
    raise NotImplementedError, "#{self.class} has not implemented method '#{__method__}'"
  end
end

# PayPal implementation
class PayPalPaymentService
  include PaymentService

  def charge(account, amount)
    # Logic to charge using the PayPal API...
    # Returns a response object...
  end
end

# Another possible future payment method
class StripePaymentService
  include PaymentService

  def charge(account, amount)
    # Logic to charge using the Stripe API...
    # Returns a response object...
  end
end

# Payment client
class PaymentClient
  def initialize(payment_service)
    @payment_service = payment_service
  end
  
  def process_payment(account, amount)
    response = @payment_service.charge(account, amount)
    if response.success?
      # Do something on success...
    else
      # Handle error...
    end
  end
end

Conclusion:

This is going to be the first part of the series for refactoring. It’ll be ongoing with examples that I face in my current & previous jobs, and also snippets from the book “Refactoring”

Hope you like it! Stay tuned for part #2