5
\$\begingroup\$

I am developing an application in which the user can use different measurement systems. Currently, there is support for the metric and imperial measurement systems. To support these systems, I wrote a converter. Could you take a look at its code and tell how can it be improved.

public class SystemsMeasuresConverter {

private Context context;
private Profile profile;

public SystemsMeasuresConverter(Context context, Profile profile) {
    this.context = context;
    this.profile = profile;
}

/**
 * Converts mass from some units to some units
 *
 * @param input mass in some units
 * @return mass some units
 */
public Number convertToMassUnit(Number input, SystemsMeasures from, SystemsMeasures to) {
    double constant = 1;

    switch (from) {
        case Imperial:
            if (to == Metric) {
                constant = 0.455;
            }
            break;
        case Metric:
            if (to == Imperial) {
                constant = 2.2;
            }
            break;
    }

    return MathUtil.round(input.doubleValue() * constant, MathUtil.GLOBAL_SCALE);
}

/**
 * Converts mass from some units to units selected in profile
 *
 * @param input mass in some units
 * @return mass in units of measurements selected in the user profile
 */
public Number convertToMassUnit(Number input, SystemsMeasures from) {
    return convertToMassUnit(input, from, profile.getSystemMeasures());
}

/**
 * Converts mass from kilograms to units selected in profile
 *
 * @param input metric system
 * @return mass in units of measurements selected in the user profile
 */
public Number convertToMassUnit(Number input) {
    return convertToMassUnit(input, Metric, profile.getSystemMeasures());
}

/**
 * Converts distance from units selected in profile to kilograms
 *
 * @param input metric system
 * @return distance in units of measurements selected in the user profile
 */
public Number convertToKg(Number input) {
    return convertToMassUnit(input, profile.getSystemMeasures(), SystemsMeasures.Metric);
}

/**
 * Returns abbreviations for mass units selected in profile
 *
 * @return abbreviations for mass units selected in profile
 */
public String getStringFromCurrentMassUnit() {
    SystemsMeasures unit = profile.getSystemMeasures();
    switch (unit) {
        case Metric:
            return context.getString(R.string.kilogramsMassUnit);
        case Imperial:
            return context.getString(R.string.poundsMassUnit);
    }
    throw new RuntimeException("Cannot find Mass Unit");
}

/**
 * Converts distance from some units to some units
 *
 * @param input mass in some units
 * @return mass some units
 */
public Number convertToDistanceUnits(Number input, SystemsMeasures from, SystemsMeasures to) {
    double constant = 1;

    switch (from) {
        case Imperial:
            if (to == Metric) {
                constant = 2.54;
            }
            break;
        case Metric:
            if (to == Imperial) {
                constant = 0.394;
            }
            break;
    }

    return MathUtil.round(input.doubleValue() * constant, MathUtil.GLOBAL_SCALE);
}

/**
 * Converts distance from some units to units selected in profile
 *
 * @param input distance in some units
 * @return distance in units of measurements selected in the user profile
 */
public Number convertToDistanceUnits(Number input, SystemsMeasures from) {
    return convertToDistanceUnits(input, from, profile.getSystemMeasures());
}

/**
 * Converts distance from meters to units selected in profile
 *
 * @param input metric system
 * @return distance in units of measurements selected in the user profile
 */
public Number convertToDistanceUnits(Number input) {
    return convertToDistanceUnits(input, Metric, profile.getSystemMeasures());
}

/**
 * Converts distance from units selected in profile to meters
 *
 * @param input metric system
 * @return distance in units of measurements selected in the user profile
 */
public Number convertToMeter(Number input) {
    return convertToDistanceUnits(input, profile.getSystemMeasures(), SystemsMeasures.Metric);
}

/**
 * Returns abbreviations for distance units selected in profile
 *
 * @return abbreviations for distance units selected in profile
 */
public String getStringFromCurrentDistanceUnit() {
    SystemsMeasures unit = profile.getSystemMeasures();
    switch (unit) {
        case Metric:
            return context.getString(R.string.meters_distance_unit);
        case Imperial:
            return context.getString(R.string.feet_distance_unit);
    }
    throw new RuntimeException("Cannot find Mass Unit");
  }
}

SystemsMeasures

public enum SystemsMeasures {
    Metric(0),
    Imperial(1);

    private final int value;

    SystemsMeasures(int value) {
        this.value = value;
    }

    public int getValue(){
        return value;
    }

    public static SystemsMeasures getById(int id) {
        for (SystemsMeasures e : values()) {
            if (e.value == id) return e;
        }
        return Metric;
    }
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ attributes should be private and accessible using getters/setters. In your case, context and profile are package private. \$\endgroup\$ Apr 10, 2020 at 7:14
  • 1
    \$\begingroup\$ I think getById() could technically be improved by storing the enums in a HashMap or something like this additionally but I don't think there are performance improvements as thete ate only two elements. \$\endgroup\$ Apr 10, 2020 at 7:18

2 Answers 2

2
\$\begingroup\$

I would suggest changing SystemsMeasures to

public enum SystemsMeasures {
    Metric(1.0,1.0),
    Imperial(2.54,0.455);

    public final double lengthFactor;
    public final double massFactor;

    SystemsMeasures(double lengthFactor,double massFactor) {
        this.lengthUnit = lengthUnit;
        this.lengthFactor = lengthFactor;
    }
}

Then you can just write for example

/**
 * Converts mass from some units to some units
 *
 * @param input mass in some units
 * @return mass some units
 */
public Number convertToMassUnit(Number input, SystemsMeasures from, SystemsMeasures to) {
    return MathUtil.round(input.doubleValue() * from.MassFactor/to.MassFactor, MathUtil.GLOBAL_SCALE);
}
\$\endgroup\$
4
  • \$\begingroup\$ It seems to me to use LengthUnit and MassUnit will be superfluous, since this does not affect the work of the code and does not make any sense \$\endgroup\$
    – Destroyer
    Apr 10, 2020 at 13:13
  • \$\begingroup\$ @Destroyer. You're right. I think that I added them for simplification of getStringFromCurrentMassUnit() and getStringFromCurrentDistanceUnit(). \$\endgroup\$
    – md2perpe
    Apr 10, 2020 at 13:20
  • \$\begingroup\$ These lines are in R.string. Therefore, for implementation, you need to use not string but id string, and i will have to use context in enum. In general, I think this is a bad idea. \$\endgroup\$
    – Destroyer
    Apr 10, 2020 at 13:47
  • \$\begingroup\$ @Destroyer. I approved your edit. You know more about the context than I do. \$\endgroup\$
    – md2perpe
    Apr 10, 2020 at 13:51
2
\$\begingroup\$

I would replace all the switch statements with a Map.
Map key should be pair of from-to SystemsMeasures. Map value can be a container class that holds all conversion factors.
You can write custom Pair or Tuple class or use ready made one from libraries such as Apache commons, Guava, etc.
The container class can be something like

public class ConversionFactor {
    public double mass;
    public double distance;
    public ConversionFactor(double mass, double distance) {
        this.mass = mass;
        this.distance = distance;
    }
}

The map can be defined (assuming Apache commons Pair which correctly implements equals() and hashCode())

Map<Pair<SystemsMeasures, SystemsMeasures>, ConversionFactor>

Now, you just need to create a Pair instance that represents the requested conversion and get() the value from the map.

an added advantage of this approach is that you can easily load the map values from file, avoiding magic numbers anti-pattern

Example:

public static Map<Pair<SystemsMeasures, SystemsMeasures>, ConversionFactor> conversionMap =
    Map.of(
        new ImmutablePair<>(SystemsMeasures.Metric, SystemsMeasures.Imperial),
            new ConversionFactor(2.2, 0.394),
        new ImmutablePair<>(SystemsMeasures.Imperial, SystemsMeasures.Metric),
            new ConversionFactor(0.455, 2.54)
    );

public Number convertToMassUnit(Number input, SystemsMeasures from, SystemsMeasures to) {
    double constant = conversionMap.get(new ImmutablePair<>(from, to)).mass;
    return ... 
}

The above map can be represented in serialized form:

Metric, Imperial -> 2.2, 0.394
Imperial, Metric -> 0.455, 2.54

read the file into List<String> and parse it into the map.

\$\endgroup\$
8
  • \$\begingroup\$ Could you give some examples of this? (code, examples, articles) \$\endgroup\$
    – Destroyer
    Apr 10, 2020 at 9:51
  • 2
    \$\begingroup\$ see edited code \$\endgroup\$ Apr 10, 2020 at 10:17
  • \$\begingroup\$ Why did you reject the change? Initially, the question rocked Android, for which Map.of does not work. \$\endgroup\$
    – Destroyer
    Apr 10, 2020 at 12:49
  • 1
    \$\begingroup\$ the question is not specific to Android. and even if you don't want to use Map.of, it is bad practice to use double brace initialization \$\endgroup\$ Apr 10, 2020 at 13:35
  • \$\begingroup\$ But how can this be done differently if this is bad practice? \$\endgroup\$
    – Destroyer
    Apr 10, 2020 at 13:41

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.