Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I'm writing a small Android application that deals with payments and, in my function that handles transactions, I need to translate a number of installments (1-12) and whether or not it has interest (0-1) into the de facto objects for those of the SDK I'm using.

Right now I have a switch that looks like the following:

InstalmentTransactionEnum install = InstalmentTransactionEnum.ONE_INSTALMENT;
//TODO: Implement interest
switch (installments) {
    case "1":
        install = InstalmentTransactionEnum.ONE_INSTALMENT;
        break;
    case "2":
        install = InstalmentTransactionEnum.TWO_INSTALMENT_NO_INTEREST;
        break;
    case "3":
        install = InstalmentTransactionEnum.THREE_INSTALMENT_NO_INTEREST;
        break;
    case "4":
        install = InstalmentTransactionEnum.FOUR_INSTALMENT_NO_INTEREST;
        break;
    case "5":
        install = InstalmentTransactionEnum.FIVE_INSTALMENT_NO_INTEREST;
        break;
    case "6":
        install = InstalmentTransactionEnum.SIX_INSTALMENT_NO_INTEREST;
        break;
    case "7":
        install = InstalmentTransactionEnum.SEVEN_INSTALMENT_NO_INTEREST;
        break;
    case "8":
        install = InstalmentTransactionEnum.EIGHT_INSTALMENT_NO_INTEREST;
        break;
    case "9":
        install = InstalmentTransactionEnum.NINE_INSTALMENT_NO_INTEREST;
        break;
    case "10":
        install = InstalmentTransactionEnum.TEN_INSTALMENT_NO_INTEREST;
        break;
    case "11":
        install = InstalmentTransactionEnum.ELEVEN_INSTALMENT_NO_INTEREST;
        break;
    case "12":
        install = InstalmentTransactionEnum.TWELVE_INSTALMENT_NO_INTEREST;
        break;
}
stoneTransaction.setInstalmentTransactionEnum(install);

Now this is without me having implemented interest which in this method would require another massive switch statement where install would be equal to TWO_INSTALLMENT_WITH_INTEREST and so on.

My thinking is that there must be a smarter way of doing this that doesn't involve an if statement to check the interest variable (a boolean) and the massive switch for installments. What is the best way of achieving this?

share|improve this question
1  
I don't know if this is helpful, but learning to use enum.valueOf(String) effectively will be useful as a general programmer, even if it doesn't help this particular scenario. – corsiKa Mar 14 at 16:32
1  
Suppose that we end up selecting the appropriate enum — then what? In other words, why do you want an enum in the first place? – 200_success Mar 15 at 2:13
    
So this is the library hmms? stone-pagamentos.github.io/sdk-android/#provedor-de-transao – h.j.k. Mar 15 at 2:42
    
@200_success I get the appropriate enum and pass it on to a transaction object of the SDK and continue to do a transaction – Bernard Meurer Mar 15 at 11:20
    
@h.j.k., yep that's the one – Bernard Meurer Mar 15 at 11:21
up vote 16 down vote accepted

You're using the wrong tool for the job.

If there's ever going to be a loan that I will pay an amount to every week for 2 years, you're gonna have to write a script to generate the code for you, because doing it by hand would suck. You'd have to write code for 204 different enum values, including the switch cases.

If you had to map months to strings, you could have given your enums values, e.g.

enum Month {
    JANUARY(1, "January", "JAN"),
    FEBRUARI(2, "February", "FEB"),
    ...

    private int number;
    private String name;
    private String shorthandName;
    Month(int month, String fullName, String shortName){
        number = month;
        name = fullName;
        shorthandName = shortName;
    }
    //add getters....
}

Like that.

But what you have here is more like an object. You have two values to keep track of, and they are not related. This doesn't lend itself to an enumeration as you'll need to create each combination by hand.

For that, I recommend using value objects. An example of a value object is a String. It's an immutable object of which you cannot change the value.

So consider making a class like so:

class InstallmentTransaction {
    private final int installments;
    private final boolean interest;

    private static final Map<Integer, Map<Boolean, InstallmentTransaction>> instances = new HashMap<>();
    private static final Object lockObject = new Object();

    private InstallmentTransaction(int installments, boolean interest)
    {
        this.installments = installments;
        this.interest = interest;
    }

    public static InstallmentTransaction getInstance(int installments, boolean interest){
        synchronized(lockObject){
            Map<Boolean, InstallmentTransaction> map = instances.get(installments);
            if(map == null){
                map = new HashMap<>();
                instances.put(installments, map);
            }
            InstallmentTransaction instance = map.get(interest);
            if(instance == null){
                instance = new InstallmentTransaction(installments, interest);
                map.put(interest, instance);
            }
            return instance;
        }
    }

    //getters go here
}

I didn't implement the full thing, that's something you'll have to do, but this will give you the benefit of object equality combined with the ability to reference instances (getInstance(12, false)).

I also opted for Integer here, because that's the format you store numbers in. Even if the API only exposes Strings, that doesn't mean you are stuck using Strings. ""+int will give you a String, and Integer.parseInt("") will give you an integer.

synchronized is used to prevent there being two threads both calling getInstance at the same time and creating two objects for the same values.


The upsides of this approach are that it automatically supports all values of installments and interest.

The downside is that if you have a lot of values, you risk filling up your memory with all of them. You don't have this, though, as right now it's going to be 24 values (12 months * 2). For Android development, it might not even matter - apps don't have the same active duration as server applications do.

share|improve this answer
    
While trying to implement this (and I just copied and pasted to get started) I got an error – Bernard Meurer Mar 14 at 14:48
    
@BernardMeurer ideone.com/RBwKvN - I copy and pasted into ideone and I get 0 errors. Looks like you're doing something wrong. What does the line you're using look like? – Pimgd Mar 14 at 14:50
2  
Figured, when I copied it the private constructor part was still missing :) – Bernard Meurer Mar 14 at 14:55
    
Why are you reimplementing enum with a static flyweight cache and losing the compile-time integrity offered by enums? – corsiKa Mar 14 at 22:29
    
@corsiKa see answer: because if you have to expand to cover ONE_HUNDRED_AND_FOUR_INSTALMENT_NO_INTEREST, something has gone wrong. That's the situation I'm describing at the top: pay per week for 2 years. I'll expand the explanation, though. – Pimgd Mar 15 at 0:21

There is a better way indeed, which is to have the installments value as an attribute of each enumeration value.

An as example, we can simply consider 2 of those enum values. The idea is to construct each value with the given installment String. Then, we can create a look-up method that selects the right enumeration from a given value.

public enum InstalmentTransactionEnum {

    ONE_INSTALMENT("1"),
    TWO_INSTALMENT_NO_INTEREST("2");

    private final String installments;

    InstalmentTransactionEnum(String installments) {
        this.installments = installments;
    }

    public static InstalmentTransactionEnum forInstallments(String installments) {
        for (InstalmentTransactionEnum value : InstalmentTransactionEnum.values()) {
            if (value.installments.equals(installments)) {
                return value;
            }
        }
        return ONE_INSTALMENT;
    }

}

In your application code, you can then simply have:

InstalmentTransactionEnum install = InstalmentTransactionEnum.forInstallments(installments);

without the need of any switch statements.


If you intend to do this operation quite often, you could further create a Map<String, InstalmentTransactionEnum> that will serve as the loop-up base:

public enum InstalmentTransactionEnum {

    ONE_INSTALMENT("1"),
    TWO_INSTALMENT_NO_INTEREST("2");

    private static final Map<String, InstalmentTransactionEnum> LOOKUP;

    static {
        LOOKUP = new HashMap<>();
        for (InstalmentTransactionEnum value : InstalmentTransactionEnum.values()) {
            LOOKUP.put(value.installments, value);
        }
    }

    private final String installments;

    InstalmentTransactionEnum(String installments) {
        this.installments = installments;
    }

    public static InstalmentTransactionEnum forInstallments(String installments) {
        InstalmentTransactionEnum value = LOOKUP.get(installments);
        return value == null ? ONE_INSTALMENT : value;
    }

}

or, if you're using Java 8, you could further simplify this using the Stream API to construct the look-up map and getOrDefault to return the default value:

public enum InstalmentTransactionEnum {

    ONE_INSTALMENT("1"),
    TWO_INSTALMENT_NO_INTEREST("2");

    private static final Map<String, InstalmentTransactionEnum> LOOKUP =
        Arrays.stream(InstalmentTransactionEnum.values())
              .collect(Collectors.toMap(v -> v.installments, v -> v));

    private final String installments;

    InstalmentTransactionEnum(String installments) {
        this.installments = installments;
    }

    public static InstalmentTransactionEnum forInstallments(String installments) {
        return LOOKUP.getOrDefault(installments, ONE_INSTALMENT);
    }

}
share|improve this answer
    
Very nice! How would I add the WITH_INTEREST options though? – Bernard Meurer Mar 14 at 14:20
    
@BernardMeurer By adding more enum values with their respective installments value. I've just provided a sample with 2 values but you can extend it to any. – Tunaki Mar 14 at 14:21
1  
The upside of this approach is that you can have all your values in one place with compile time checking. The downside is the case I mentioned - where installments are in "weeks" and the loan takes multiple years - enjoy writing 200+ enum values. It depends on your expected use-case. – Pimgd Mar 14 at 14:22
    
@Tunaki, The issue is that, lets say, a 3 month payment both with and without interest will have installments set to 3, but the second variable interest will shift from 0 to 1 – Bernard Meurer Mar 14 at 14:23
    
@BernardMeurer Ha well that is something that you didn't mention in your post. But the idea is the same: have InstalmentTransactionEnum this time take 2 values in the constructor: the installments and the interest. And in the look-up phase, you search with the right installments and interest. The look-up Map in this case would be Map<String, Map<Integer, InstalmentTransactionEnum>>. – Tunaki Mar 14 at 14:26

In this case you can use a Map<String, InstalmentTransactionEnum>

install  = map.get(installments);

However why the enum in the first place? You can simply parse the string as an int with Integer.parseString and use the return value directly.

share|improve this answer

Assuming if we're talking about the same SDK, then perhaps the worry that you'll need to support 100+ enums is unnecessary... or you should really consider switching SDKs if you/the library developers do get to that stage.

enums values are ordered (hence the ordinal() method), and the ordering the SDK uses seem to be:

0 -> ONE_INSTALMENT
1 -> TWO_INSTALMENT_NO_INTEREST
...
11 -> TWELVE_INSTALMENT_NO_INTEREST
12 -> TWO_INSTALMENT_WITH_INTEREST
..
22 -> TWELVE_INSTALMENT_WITH_INTEREST

Unless the SDK explicitly discourages the reliance on the enum ordering, usually it's reliable enough to do so as changing the ordering will potentially have other complications (e.g. iterating through an EnumSet or EnumMap will change too).

That means it should be possible for you to derive the appropriate mapping by:

  1. Convert "1" into an int (or not do the conversion if you started out as an int).
  2. Consider if you need to apply installments or not.
  3. Math.
boolean hasInterest = /* determine if interest is required? */;
// assumes a one-month installment does not have interest
int index = (Integer.parseInt(installments) - 1) + (hasInterest ? 0 : 11);
InstalmentTransactionEnum result = InstalmentTransactionEnum.values()[index];
share|improve this answer
1  
Bad generic advice (math hax like this is not very extensible) but awesome specific advice! If such a solution exists then yes, go ahead and use it. It's shorter, doesn't have memory issues and saves quite a bit of development time. – Pimgd Mar 15 at 9:28
    
We are talking about the same SDK indeed! Loved this one, very specific but so simple! – Bernard Meurer Mar 15 at 19:50

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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