Struck with Switch, reducing cyclomatic complexity

问题:

while working with java i was stuck in a weird situation, lets start with code first.

public static String constructMessage(final String reason, final String xId,
        final String yId, final IonStruct metadataStruct) throws DataSanityException {
    String message = "";
        switch (reason) {
            case "NOT_ACTIVE":
                if (xId != null) {
                    message = String.format("Y %s X %s not active: status is inactive%n", yId, xId);
                } else {
                    message = String.format("Y %s not active: status is inactive%n", yId);
                }
                break;
            case "PRICE_GONE":
                if (metadataStruct.containsKey("applied")) {
                    final String applied = metadataStruct.get("applied");
                    if (applied.equals("single")) {
                        message = String.format("X %s is not active. price for the X is gone. "
                                + "Y price %s for an X should not be gone by %s.%n", xId,
                                metadataStruct.get("yPrice"), metadataStruct.get("zPrice"));
                    } else {
                        if (metadataStruct.get("gonePeriod").equals("gonegone")) {
                            message = String.format("Y price is %s whereas the gone price is %s."
                                    + " Y price for an X should be at least %s%% lower than gone.%n",
                                    metadataStruct.get(yPrice), metadataStruct.get(zPrice), metadataStruct.get("xyzPrice"));
                        } else {
                            message = String.format("Y price %s for an X should be at least %s%% lower than gone price %s in last %s days.%n",
                                    metadataStruct.get(yPrice), metadataStruct.get("mpqPrice"), metadataStruct.get(lpzPrice),
                                    metadataStruct.get("numberOfDays"));
                        }
                    }
                } else {
                    message = String.format(
                            "X %s in Y %s is not active. is %s%%, min mpux is %s%%. ", xId,
                            yId, metadataStruct.get("lpux"), metadataStruct.get("mpxPrice"));
                }
                break;

         and so on ----------- around 20 cases.
         default:
                message = "";
                break;
}

there are 20 cases like this in the complete switch with different messages and different variables used in each of the cases. And the depth of ifs is also upto 3 in some of the switches.

So the cyclomatic complexity is coming 29(too high), so i want to refactor the code with low cyclomatic complexity and better readability.

Any Suggestion ?

回答1:

From Martin Fowler's Refactoring book

I can suggest following refactoring method:

import java.util.HashMap;
import java.util.Map;

public class Refactoring {

    private static class IonStruct {};

    private static interface MessageBuilder {
        String  build(final String xId,final String yId, final IonStruct metadataStruct);
    }


    private static class NotActiveMessageBuilder implements MessageBuilder {

        private static final String YX_NON_ACTIVE_FMT = "Y %s X %s not active: status is inactive%n;";
        private static final String Y_NON_ACTIVE_FMT = "Y %s not active: status is inactive%n";

        @Override
        public String build(String xId, String yId, IonStruct metadataStruct) {
            return (xId != null) ? String.format(YX_NON_ACTIVE_FMT, yId, xId) : String.format(Y_NON_ACTIVE_FMT, yId) ;
        }

    }

    private static enum MessageTypeChine
    {
        instance();
        private final Map<String,MessageBuilder> builders;
        private MessageTypeChine() {
            this.builders = new HashMap<>();
            this.builders.put("NOT_ACTIVE", new NotActiveMessageBuilder());
        }

        public String constructMessage(final String reason, final String xId,
                final String yId, final IonStruct metadataStruct) {
            MessageBuilder builder = this.builders.get(reason);
            if(null != builder)
                return builder.build(xId, yId, metadataStruct);

            throw new UnsupportedOperationException("Message type is not supported");
        }
    }


    public static String constructMessage(final String reason, final String xId,
            final String yId, final IonStruct metadataStruct) {
        return MessageTypeChine.instance.constructMessage(reason, xId, yId, metadataStruct);
    }

}

回答2:

With "use enums instead", Kayaman means something like:

interface MessageGenerator {
    String getMessage(String xId, String yId, IonStruct metadataStruct);
}

enum Reason implements MessageGenerator {
    NOT_ACTIVE((xId, yId, unused) -> 
        (xId != null) ? String.format("Y %s X %s not active: status is inactive%n", yId, xId)
                      : String.format("Y %s not active: status is inactive%n", yId)
    }),
    PRICE_GONE(...),
    DEFAULT(xId, yId, metadataStruct) -> 
        String.format("X %s in Y %s is not active. is %s%%, min mpux is %s%%. ", 
        xId, yId, metadataStruct.get("lpux"), metadataStruct.get("mpxPrice")));

    private MessageGenerator delegate;
    private Reason(MessageGenerator delegate) {
        this.delegate = delegate;
    }
    // implement MessageGenerator interface by delegation
    public String getMessage(String xId, String yId, IonStruct metadataStruct) {
        return delegate.getMessage(xId, yId, metadataStruct);
    }

    public static Reason findReason(String reason) {
        Reason result = DEFAULT;
        try {
            result = valueOf(reason);
        } catch (RuntimeException ex) {}
        return result;
    }
}

and you can do

String constructMessage(String reason, final String xId,
    final String yId, final IonStruct metadataStruct) {
    Reason r = Reason.findReason(reason);
    return r.getMessage(xId, yId, metadataStruct);
}