5
\$\begingroup\$

I have three methods that I need to call multiple times only varying the date parameter of each. The natural solution to avoid code duplication is to put the three methods in a loop and iterate over the varying dates.

The problem with that approach is that I need to keep track of the results of each method call along with the dates that produced the result so that I can build the DTOs. So, I'm not sure how to employ a loop without resulting in redundant & verbose code or if a loop is even the right solution to avoid code duplication.

Here's my code:

    LocalDate sevenDaysAgo = endDate.minusDays(7);
    Map<String, BigDecimal> sevenDayValues = client.getValues(productIds, sevenDaysAgo, endDate);       
    Map<String, BigDecimal> sevenDayGrossValues = client.getBaselineValues(grossProductIds, sevenDaysAgo, endDate, "gross");
    Map<String, BigDecimal> sevenDayNetValues = client.getBaselineValues(netProductIds, sevenDaysAgo, endDate, "net");

    //calling same methods again varying start date
    LocalDate thirtyDaysAgo = endDate.minusDays(30);
    Map<String, BigDecimal> thirtyDayValues = client.getValues(productIds, thirtyDaysAgo, endDate);     
    Map<String, BigDecimal> thirtyDayGrossValues = client.getBaselineValues(grossProductIds, thirtyDaysAgo, endDate, "gross");
    Map<String, BigDecimal> thirtyDayNetValues = client.getBaselineValues(netProductIds, thirtyDaysAgo, endDate, "net");

    //and again
    LocalDate sixtyDaysAgo = endDate.minusDays(60);
    Map<String, BigDecimal> sixtyDayValues = client.getValues(productIds, sixtyDaysAgo, endDate);       
    Map<String, BigDecimal> sixtyDayGrossValues = client.getBaselineValues(grossProductIds, sixtyDaysAgo, endDate, "gross");
    Map<String, BigDecimal> sixtyDayNetValues = client.getBaselineValues(netProductIds, sixtyDaysAgo, endDate, "net");

    //and yet again
    LocalDate ninetyDaysAgo = endDate.minusDays(90);
    Map<String, BigDecimal> ninetyDayValues = client.getValues(productIds, ninetyDaysAgo, endDate);     
    Map<String, BigDecimal> ninetyDayGrossValues = client.getBaselineValues(grossProductIds, ninetyDaysAgo, endDate, "gross");
    Map<String, BigDecimal> ninetyDayNetValues = client.getBaselineValues(netProductIds, ninetyDaysAgo, endDate, "net");

    List<ProductValuesDto> dtos = new ArrayList<>();

    //now build the dtos. Need to know the date range 
    //that the values correspond to (see buildDtos method)
    buildDtos(dtos, 
            productId, 
            sevenDayValues, 
            thirtyDayValues, 
            ninetyDayValues, 
            threeHundredSixtyFiveDayValues);

    buildDtos(dtos, 
            grossProductIds, 
            sevenDayGrossValues, 
            thirtyDayGrossValues, 
            ninetyDayGrossValues, 
            threeHundredSixtyFiveDayGrossValues);

    buildDtos(dtos, 
            netProductIds, 
            sevenDayNetValues, 
            thirtyDayNetValues, 
            ninetyDayNetValues, 
            threeHundredSixtyFiveDayNetValues);

    return dtos;

}

public void buildDtos(List<ProductValuesDto> dtos,
        List<String> productIds,
        Map<String, BigDecimal> sevenDayValues,
        Map<String, BigDecimal> thirtyDayValues,
        Map<String, BigDecimal> ninetyDayValues,
        Map<String, BigDecimal> threeHundredSixtyFiveDayValues) {

    for(String productId : productIds) {
        ProductValuesDto dto = ProductValuesDto
                .builder(productId)
                .sevenDayValue(sevenDayValues.get(productId))
                .thirtyDayValue(thirtyDayValues.get(productId))
                .ninetyDayValue(ninetyDayValues.get(productId))
                .threeHundredSixtyFiveDayValue(threeHundredSixtyFiveDayValues.get(productId))
                .build();
        dtos.add(dto);  
    }

}

The client methods all return Map<String, BigDecimal> where the key is the productId and the value is assessed value of the product over the start to end date.

A few notes:

  1. client methods are defined in a third party library that I cannot change
  2. The DTO will be handed back to a client of my application. It needs to have fields as noted in the builder.
  3. The offset days (7, 30, 60, 90) are static requirements. But it's possible (although unlikely) that a 120 day offset may be added in the future.
  4. I'm using Java 8 and open to using functional programming techniques.
\$\endgroup\$
3
  • \$\begingroup\$ "1. client methods are defined in a third party library that I cannot change" You can always put a wrapper on top. \$\endgroup\$ Mar 19, 2020 at 19:09
  • \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ Mar 22, 2020 at 13:14
  • \$\begingroup\$ Thanks for the guidance, Sᴀᴍ Onᴇᴌᴀ. I've changed the title. \$\endgroup\$
    – James
    Mar 31, 2020 at 1:57

1 Answer 1

6
\$\begingroup\$

Thanks for sharing your code.

As you already found out, the only difference in the "map generation blocks" is the names of the variables holding the maps. So it is quite obvious, that you could move this 3 lines into the method buildDtos.

On the other hand the first line in each "map generation blocks" calls a separate method on the client object. This is an odd ball solution and the proper way to deal with that would be to enable clinet.client.getBaselineValues() to deal with a special String (Attention! resist the temptation to use null!) to return the same result as clinet.client.getValues().

until then we could introduce a FunctionalInterface to mimik that on this side. This could look like this:

@FunctionalInterface
interface ValueSelector {
    Map<String, BigDecimal> selectFrom(
          Client client, 
          List<String> ids, 
          LocalDate startDate, 
          LocalDate endDate);
}

public List<ProductValuesDto> main() {

    buildDtos(//
            dtos,
            productIds,
            (client, ids, start, end) -> client.getValues(ids, start, end));
    buildDtos(
            dtos,
            grossProductIds,
            (client, ids, start, end) -> client.getBaselineValues(ids, start, end, "gross"));
    buildDtos(//
            dtos,
            netProductIds,
            (client, ids, start, end) -> client.getBaselineValues(ids, start, end, "net"));
    return dtos;

}

public void buildDtos(List<ProductValuesDto> dtos, List<String> productIds, ValueSelector valueSelector) {

    Map<String, BigDecimal> sevenDayValues = valueSelector
            .selectFrom(client, productIds, endDate.minusDays(7), endDate);
    Map<String, BigDecimal> thirtyDayValues = valueSelector
            .selectFrom(client, productIds, endDate.minusDays(30), endDate);
    Map<String, BigDecimal> threeHundredSixtyFiveDayValues = valueSelector
            .selectFrom(client, productIds, endDate.minusDays(365), endDate);
    Map<String, BigDecimal> ninetyDayNetValues = valueSelector
            .selectFrom(client, productIds, endDate.minusDays(90), endDate);

    for (String productId : productIds) {
        ProductValuesDto dto = ProductValuesDto.builder(productId)//
                .sevenDayValue(sevenDayValues.get(productId))//
                .thirtyDayValue(thirtyDayValues.get(productId))//
                .ninetyDayValue(ninetyDayNetValues.get(productId))//
                .threeHundredSixtyFiveDayValue(threeHundredSixtyFiveDayValues.get(productId))//
                .build();
        dtos.add(dto);
    }

}

Update

Based on that approach we could improve this even further.

Improvement 1

Create an enum that provides implementations of the new interface:

enum ClientValues {
    PLAIN {

        @Override
        ValueSelector getSelectorForType(String reportType) {
            return (client, ids, start, end)
                      -> client.getValues(ids, start, end);
        }
    },
    BASELINE {

        @Override
        ValueSelector getSelectorForType(String reportType) {
            return (client, ids, start, end)
                      ->  client.getBaselineValues(ids, start, end, reportType);
        }
    };
    abstract ValueSelector getSelectorForType(//
            String reportType);
}

create a Map that assigns this enum constants to the strings used:

    Map<String, ClientValues> valuesByType = new HashMap<>();
    valuesByType.put("plain", ClientValues.PLAIN);

We intentionally leave out the constant that is used most often.

Next we create another map that assignes the Strings used with the respective ID lists:

    Map<String, List<String>> idsByType = new HashMap<>();
    idsByType.put("plain", productIds);
    idsByType.put("gross", grossProductIds);
    idsByType.put("net", netProductIds);

Complete mapping this time.

Both maps could even be class members (with static initializers) or object members constructed somewhere else and injected into this class.

Now we can iterate over the entries of the second map using the first one like this:

    for (String idType : idsByType.keySet()) {
        List<String> ids = idsByType.get(idType);
        ClientValues clientValues = 
                       valuesByType.getOrDefault(idType, ClientValues.BASELINE);
        buildDtos(//
                dtos,
                ids,
                clientValues.getSelectorForType(idType));
    }
    return dtos;

Advantage: an new "report type" would only require a new entry in idsByType and maybe a new entry in valuesByType if the client has another special method for fetching than values.

Improvement 2

The same approach can simplify buildDtos too.

Again we start by creating an enum:

enum ProductValuesBuilderFacade {
    SEVEN_DAY_VALUES(7) {
        ProductValuesDto setValueTo(
                  ProductValuesDto builder, BigDecimal value) {
            return builder.sevenDayValue(value);
        }
    },
    THIRTY_DAY_VALUES(30) {
        ProductValuesDto setValueTo(
                  ProductValuesDto builder, BigDecimal value) {
            return builder.thirtyDayValue(value);
        }
    },
    NINETY_DAY_VALUES(90) {
        ProductValuesDto setValueTo(
                  ProductValuesDto builder, BigDecimal value) {
            return builder.ninetyDayValue(value);
        }
    },
    THREE_HUNDRED_SIXTY_FIVE_DAY_VALUES_DAY_VALUES(365) {
        ProductValuesDto setValueTo(
                  ProductValuesDto builder, BigDecimal value) {
            return builder.threeHundredSixtyFiveDayValue(value);
        }
    };

    private final long timeOffsetInDays;

    ProductValuesBuilderFacade(long timeOffsetInDays) {
        this.timeOffsetInDays = timeOffsetInDays;
    }

    abstract ProductValuesDto setValueTo(
                 ProductValuesDto builder, BigDecimal bigDecimal);

    public LocalDate getStartDate(LocalDate endDate) {
        return endDate.minusDays(timeOffsetInDays);
    }
}

This gives us the opportunity to iterate over its constants inside the method:

public void buildDtos(
               List<ProductValuesDto> dtos,
               List<String> productIds,
               ValueSelector valueSelector) {
    for (String productId : productIds) {
        ProductValuesDto dto = ProductValuesDto.builder(productId);
        ProductValuesBuilderFacade[] productValuesBuilderFacades =
               ProductValuesBuilderFacade.values();
        for (ProductValuesBuilderFacade productValuesBuilderFacade 
               : productValuesBuilderFacades) {
            Map<String, BigDecimal> allValuesForDate 
                     = valueSelector.selectFrom(
                          client,
                          productIds,
                          productValuesBuilderFacade
                                .getStartDate(endDate),
                          endDate);
            dto = productValuesBuilderFacade.setValueTo(
                          dto,
                          allValuesForDate.getOrDefault(productId, BigDecimal.ZERO));
        }
        dtos.add(dto.build());
    }
}

Advantages:

  • again the enum can live in its own file.
  • a new setter method in the DTO requires only a new constant in the enum.
\$\endgroup\$
6
  • \$\begingroup\$ Thank you for providing a solution. The FunctionalInterface seems like a great way to refactor the code. client.getBaselineValues and client.getValues do very different things although they return the same type of data. I think that may explain why the 3rd party developer chose different methods. As you noted though, that knowledge could be hidden from its clients by allowing a special value for the String param to return the same result as client.getValues. \$\endgroup\$
    – James
    Mar 19, 2020 at 21:22
  • \$\begingroup\$ @James "seems like a great way to refactor the code." It is just a technical detail. The "way" to refactor code is: convert similar looking code into equal looking code so that the duplication becomes obvious and your IDE can support you resolving the duplication. \$\endgroup\$ Mar 20, 2020 at 9:40
  • \$\begingroup\$ Thanks for the comment. By the way, I believe the buildDtos method in Improvement 2 has an issue. The outer loop iterates over productIds while inner loop calls selectFrom for all productIds. So, selectFrom is called once for every product id & ProductValuesBuilderFacade but it only needs to be called once per ProductValuesBuilderFacade. \$\endgroup\$
    – James
    Apr 3, 2020 at 20:45
  • \$\begingroup\$ @James "I believe the buildDtos method in Improvement 2 has an issue." --- maybe. But on one hand this site is not focused on correctness, My aim was to provide a "recipe" for the improvement, not a working solution. And for second you did not provide a Minimal, Complete, and Verifiable example including UnitTests to support stable behavior while refactoring... \$\endgroup\$ Apr 4, 2020 at 8:46
  • \$\begingroup\$ @ Timothy Truckle, I'm very grateful for your solution & I hope that my comment did not come off as unappreciative. When I went to incorporate your improvements into my code, I simply noticed the unwanted client calls (which are slow). To eliminate that, it resulted in builDtos being a bit more involved. Besides getting thoughts about if I was maybe missing something, I was trying to provide general acknowledgement of the issue for anyone else reading this post. Your point is taken though & once again, I very much appreciate all your time & work to provide a thoughtful solution. \$\endgroup\$
    – James
    Apr 6, 2020 at 15:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.