Technology Blog

Refactor your data class and you'll sleep like a baby

May 15, 2019 - 6 min read ⏱️

jalopez
jalopez

How to refactor code coupled with a data class?


Majority of developers sometimes in their lives have tried to refactor a code that is highly coupled with one or multiple data classes. This is a way to work with it without crying like a baby.

Let’s focus in I of SOLID:

Interface segregation: many client-specific interfaces are better than one general-purpose interface

So imagine we have a class called Hotel:

@Value
public class Hotel
{
    private final String hotelId;
    private final List<MealPlan> mealPlans;
    private final BasicStaticInfo staticInfo;
}

and that class is used to sort and to filter.

@AllArgsConstructor
public class BudgetFilter{
    private final long minBudget;
    private final long maxBudget;
    public boolean passes(List<Hotel> hotels){
        return hotels.stream()
            .anyMatch(h -> h.getMealPlans().stream.anyMatch(m -> m.getPrice() <= maxBudget && m.getPrice() >= minBudget));
    }
}

@AllArgsConstructor
public class SortByPopularity{
    public List<Hotel> sort(List<Hotel> hotels){
        return hotels.stream()
            .sorted((a,b) -> b.getStaticInfo().getPopularity() - a.getStaticInfo().getPopularity()).collect(Collectors.toList());
    }
}

Let’s also take into account tests we have created to test our services.

public class BudgetFilterTest{
    @Test
    public void emptyList(){
        BudgetFilter budgetFilter = new BudgetFilter(0l,0l);
        assertThat(budgetFilter.passes(emptyList()), is(true));
    }
    @Test
    public void oneMealPlanMinBudget(){
        BudgetFilter budgetFilter = new BudgetFilter(30l, 100l);
        assertThat(budgetFitler.passes(singletonList(new Hotel(null, singletonList(new MealPlan(null, 30l)), null)), is(true));
    }
    @Test
    public void oneMealPlanMaxBudget(){
        BudgetFilter budgetFilter = new BudgetFilter(30l, 100l);
        assertThat(budgetFitler.passes(singletonList(new Hotel(null, singletonList(new MealPlan(null, 100l)), null)), is(true));
    }
    @Test
    public void oneMealPlanInsideTheBudget(){
        BudgetFilter budgetFilter = new BudgetFilter(30l, 100l);
        assertThat(budgetFitler.passes(singletonList(new Hotel(null, singletonList(new MealPlan(null, 50l)), null)), is(true));
    }
    @Test
    public void oneMealPlanOutsideTheBudget(){
        BudgetFilter budgetFilter = new BudgetFilter(30l, 100l);
        assertThat(budgetFitler.passes(singletonList(new Hotel(null, singletonList(new MealPlan(null, 110l)), null)), is(false));
    }
    @Test
    public void twoMealPlansOneInsideOneOutsideTheBudget(){
        BudgetFilter budgetFilter = new BudgetFilter(30l, 100l);
        assertThat(budgetFitler.passes(singletonList(new Hotel(null, Arrays.asList(new MealPlan(null, 110l), new MealPlan(null, 50l)), null)), is(true));
    }
}

public class SortByPopularityTest{
    @Test
    public void emptyList(){
        SortByPopularity sortByPopularity = new SortByPopularity();
        assertThat(sortByPopularity.sort(emptyList()), is(emptyList()));
    }
    @Test
    public void twoHotelDisordered(){
        SortByPopularity sortByPopularity = new SortByPopularity();
        assertThat(sortByPopularity.sort(Arrays.asList(
                new Hotel(null, null, new BasicStaticInfo(50)),
                new Hotel(null, null, new BasicStaticInfo(75)))),
            is(Arrays.asList(
                new Hotel(null, null, new BasicStaticInfo(75)),
                new Hotel(null, null, new BasicStaticInfo(50))))));
    }

}

In this situation imagine that we have a performance problem because our data store size limitations (we are storing our search results in our data store to avoid calling for every operation to our providers) and we have increased the number of searches our users do.

So our conclusion is to stop using the whole Hotel object to store the information in our data store but:

  • We are storing mealplans because of BudgetFilter
  • We are storing staticInfo because of our SortByPopularity service.

As we haven’t followed Interface Segregation principle nor Law of Demeter, in some way the module in charge of storing data in our data store is coupled with methods and memory structures used by our services and our services between them.

What to do?

  • We could try to save the world at once removing the Hotel object and replacing it with another little class. This approach is hard, because usually everything will be broken and probably you will have to expend days to solve it and later manage the conflicts because of other developers work.
  • We can try with step by step plan:

    1. We could apply Law of Demeter so we could add to hotel methods to avoid giving access to deeper classes.
    2. Replacing in BudgetFilter and SortByPopularity code by these new methods.
    3. Extracting two interfaces in Hotel one called FilterableHotel and one called SortableHotel
    4. Use those interfaces in BudgetFilter and SortByPopularity service

1. Law of Demeter

@Value
public class Hotel
{
    private final String hotelId;
    private final List<MealPlan> mealPlans;
    private final BasicStaticInfo staticInfo;
+    public boolean isPriceInBudget(double minPrice, double maxPrice){
+        return mealPlans.stream.anyMatch(m -> m.getPrice() <= maxBudget && m.getPrice() >= minBudget);
+    }
+    public int getPopularity() {
+        return staticInfo.getPopularity();
+    }
}

2. Using new methods in services

@AllArgsConstructor
public class BudgetFilter
    private final long minBudget;
    private final long maxBudget;
    public boolean passes(List<Hotel> hotels){
        return hotels.stream()
-            .anyMatch(h -> h.getMealPlans().stream.anyMatch(m -> m.getPrice() <= maxBudget && m.getPrice() >= minBudget));
+            .anyMatch(h -> h.isPriceInBudget(minBudget, maxBudget)));
    }
}

@AllArgsConstructor
public class SortByPopularity
    public List<Hotel> sort(List<Hotel> hotels){
        return hotels.stream()
-            .sorted((a,b) -> b.getStaticInfo().getPopularity() - a.getStaticInfo().getPopularity()).collect(Collectors.toList());
+            .sorted((a,b) -> b.getPopularity() - a.getPopularity()).collect(Collectors.toList());
    }
}

3. Creating interfaces

public interface FilterableHotel
{
    boolean isPriceInBudget(double minBudget, double maxBudget);
}

public interface SortableHotel
{
    int getPopularity();
}
@Value
-public class Hotel
+public class Hotel implements FilterableHotel, SortableHotel
{
    private final String hotelId;
    private final List<MealPlan> mealPlans;
    private final BasicStaticInfo staticInfo;
    public boolean isPriceInBudget(double minBudget, double maxBudget){
        return mealPlans.stream.anyMatch(m -> m.getPrice() <= maxBudget && m.getPrice() >= minBudget);
    }
    public int getPopularity() {
        return staticInfo.getPopularity();
    }
}

4. Using the new interfaces in our services

@AllArgsConstructor
public class BudgetFilter
    private final long minBudget;
    private final long maxBudget;
-    public boolean passes(List<Hotel> hotels){
+    public boolean passes(List<? extends FilterableHotel> hotels){
        return hotels.stream()
            .anyMatch(h -> h.isPriceInBudget(minBudget, maxBudget)));
    }
}

@AllArgsConstructor
public class SortByPopularity
-    public List<Hotel> sort(List<Hotel> hotels){
+    public List<SortableHotel> sort(List<? extends SortableHotel> hotels){
        return hotels.stream()
            .sorted((a,b) -> b.getPopularity() - a.getPopularity()).collect(Collectors.toList());
    }
}

In short, we are saying that:

  • We can move these responsibilities step by step, since these are small, safe refactors (BudgetFilterTest and SortByPopularityTest will remain green).
  • Once you moved these responsibilities to the interfaces, we’re free to alter the data structure itself, optimizing it.

Once this plan is executed our services have been decoupled from the Hotel class so we can start thinking how to refactor Hotel for improvements or even removing the class.


lastminute.com folks
Written by lastminute.com folks who are busy living. You should follow us on Twitter

The postings on this site are authors' opinions and experiences and do not necessarily represent the postings, strategies or opinions of lastminute.com group.