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;
}
}
2 Answers 2
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);
}
-
\$\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– Destroyer2020年04月10日 13:13:39 +00:00Commented Apr 10, 2020 at 13:13
-
\$\begingroup\$ @Destroyer. You're right. I think that I added them for simplification of
getStringFromCurrentMassUnit()
andgetStringFromCurrentDistanceUnit()
. \$\endgroup\$md2perpe– md2perpe2020年04月10日 13:20:53 +00:00Commented 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– Destroyer2020年04月10日 13:47:34 +00:00Commented Apr 10, 2020 at 13:47
-
\$\begingroup\$ @Destroyer. I approved your edit. You know more about the context than I do. \$\endgroup\$md2perpe– md2perpe2020年04月10日 13:51:00 +00:00Commented Apr 10, 2020 at 13:51
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.
-
\$\begingroup\$ Could you give some examples of this? (code, examples, articles) \$\endgroup\$Destroyer– Destroyer2020年04月10日 09:51:06 +00:00Commented Apr 10, 2020 at 9:51
-
2\$\begingroup\$ see edited code \$\endgroup\$Sharon Ben Asher– Sharon Ben Asher2020年04月10日 10:17:48 +00:00Commented 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– Destroyer2020年04月10日 12:49:54 +00:00Commented 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\$Sharon Ben Asher– Sharon Ben Asher2020年04月10日 13:35:25 +00:00Commented Apr 10, 2020 at 13:35 -
\$\begingroup\$ But how can this be done differently if this is bad practice? \$\endgroup\$Destroyer– Destroyer2020年04月10日 13:41:27 +00:00Commented Apr 10, 2020 at 13:41
Explore related questions
See similar questions with these tags.
context
andprofile
are package private. \$\endgroup\$getById()
could technically be improved by storing the enums in aHashMap
or something like this additionally but I don't think there are performance improvements as thete ate only two elements. \$\endgroup\$