Converting shoe sizes and distance

Question

I'm working on a code where it converts shoe sizes and distance.

I require help making it "nicer" because I feel like it is a little messy. I was thinking about maybe making an enum to hold the distances and then using that enum to figure out how to convert it, but I'm looking for someone to just look at my code and tell me what I can do with it.

public class ConvertValuesAndCalculate {

    final String BASE_UNIT = "centimetres";

    double distance, shoeSize;

    String distanceType, shoeRegion, gender;

    public int convertAndCalculate(String dType, double distance,
            String region, double size, String g) {

        this.distanceType = dType;
        this.distance = distance;
        this.shoeRegion = region;
        this.shoeSize = size;
        this.gender = g;

        distance = convertDistance(distance, distanceType);
        Log.i("Distance Value (inches) ", "" + distance);

        shoeSize = convertShoeSize(shoeSize, shoeRegion, gender);
        Log.i("Shoe Size (inches) ", "" + shoeSize);

        return (int) (distance / shoeSize);

    }

    private double convertShoeSize(double size, String region,
            String g) {
        double s = size;
        s = convertRegionGender(s, region, g);
        s = shoeSizeToCent(s);
        return s;
    }

    private double shoeSizeToCent(double s) {
        double sSize = s;
        if (sSize == 3.5) {
            sSize = 22.8;
        } else if (sSize == 4) {
            sSize = 23.1;
        } else if (sSize == 4.5) {
            sSize = 23.5;
        } else if (sSize == 5) {
            sSize = 23.8;
        } else if (sSize == 5.5) {
            sSize = 24.1;
        } else if (sSize == 6) {
            sSize = 24.5;
        } else if (sSize == 6.5) {
            sSize = 24.8;
        } else if (sSize == 7) {
            sSize = 25.1;
        } else if (sSize == 7.5) {
            sSize = 25.4;
        } else if (sSize == 8) {
            sSize = 25.7;
        } else if (sSize == 8.5) {
            sSize = 26;
        } else if (sSize == 9) {
            sSize = 26.35;
        } else if (sSize == 9.5) {
            sSize = 23.67;
        } else if (sSize == 10) {
            sSize = 26.99;
        } else if (sSize == 10.5) {
            sSize = 27.31;
        } else if (sSize == 11) {
            sSize = 27.62;
        } else if (sSize == 11.5) {
            sSize = 27.94;
        } else if (sSize == 12) {
            sSize = 28.26;
        } else if (sSize == 12.5) {
            sSize = 28.58;
        } else if (sSize == 13) {
            sSize = 28.89;
        } else if (sSize == 13.5) {
            sSize = 29.21;
        } else if (sSize == 14) {
            sSize = 29.53;
        }

        return sSize;
    }

    private double convertRegionGender(double s, String region,
            String g) {
        double size = s;
        if (g.equals("Male")) {
            switch (region) {
                case "UK":
                    size += .5;
                case "AUS":
                    size += .5;
                case "JAPAN":
                    size -= 18;
            }
        }
        return size;
    }

    private double convertDistance(double d, String dType) {
        double dist;
        switch (dType) {
            case "Metres":
                dist = metresToCent(d);
            case "Inches":
                dist = inchToCent(d);
            case "Millimetres":
                dist = millimetresToCent(d);
            case "Kilometres":
                dist = kilometresToCent(d);
            case "Mile":
                dist = mileToCent(d);
            case "Yard":
                dist = yardToCent(d);
            case "Foot":
                dist = footToCent(d);
            case "Nautical Mile":
                dist = nMileToCent(d);
            default:
                dist = d;
        }
        return dist;
    }

    private double mileToCent(double d) {
        d *= 160934;
        return d;
    }

    private double yardToCent(double d) {
        d *= 91.44;
        return d;
    }

    private double footToCent(double d) {
        d *= 30.48;
        return d;
    }

    private double nMileToCent(double d) {
        d *= 185200;
        return d;
    }

    private double inchToCent(double d) {
        d *= 2.54;
        return d;
    }

    private double kilometresToCent(double d) {
        d *= 100000;
        return d;
    }

    private double millimetresToCent(double d) {
        d *= 0.1;
        return d;
    }

    private double metresToCent(double d) {
        d *= 100;
        return d;

    }

}

Answer 1

For inspiration, you might look at java.util.concurrent.TimeUnit. The basic plot is that each unit of distance (meters, centimeters, furlongs) is an enum, encapsulating the conversion logic in one place. @ChrisWue is correct that there is a Distance abstraction trying to climb out. It might be that Distance includes length in some standard unit, or alternatively it may be that Distance is a length/DistanceUnit pair.

You might also review extensible enums, which would allow future contributors to create new units that would work with your code. In this case, you might have DistanceUnit.MILES, DistanceUnit.CENTIMETERS, and so on, and then ShoeLength.UK, ShoeLength.JAPAN in a completely distinct enumeration that implements the same interface.

Gender might be better represented as an enum than a string. Whether or not you need to make that enum extensible is a function of how progressive your code is.

convertRegionGender is using a switch statement to implement a lookup table. A Map<String,Double> would be cleaner.

if (convertMap.containsKey(region)) {
    size += convertMap.get(region);
}

The same idea is true for convertDistance and shoeSizeToCent (where you are using if/else instead of switch). Using a lookup table isn't a good answer here - the problem really wants to be manipulating Distance(length, Distance.Unit) pairs - but if you are going to use a lookup table, you should implement it well.

Always be sure to have unit tests on hand for your switch statements, so that if you miss a break; in a switch clause you can track it down.

The state of this object is a bit messed up - you are essentially using convertAndCalculate() as both a calculation and a constructor. Try to separate these two ideas.

public class ShoeConvertor {
    private final double shoeSize;
    private final String region;
    private final String gender;

    public ShoeConvertor(shoeSize, region, gender) {
        this.shoeSize = shoeSize;
        this.region = region;
        this.gender = gender;
    }

    public int convert(double distance, String distanceUnit) {
        ...
    }

Answer 2

I would separate calculations of shoe sizes and distances into separate classes. It is called SRP

Name your variables properly String g is just a random letter not a name for a variable.

For methods with a lot of arguments you can use some wraping object

public int convertAndCalculate(String dType, double distance,
            String region, double size, String g)

would become

public int convertAndCalculate(ShoeAndDistance shoeAndDistance)

If there is no formula to calculate shoe size from one unit to another i suggest you to put sizes in some data structure and map them together. For example in Map. Then you just query the map instead this if-else hell. Same applies to the convertRegionGender mapping.

Also i see you use Strings a lot as a constants. Consider creating enumeration of some of them.

Answer 3

1. As @DominikM mentioned you should follow the Single Responsibility Principle. I would suggest to create at least a separate class which encapsulates the distance. It should hold a distance in a defined unit internally (millimeters, centimeters, ... whatever fits your needs best) and have an interface to instantiate it from specific values in a specific unit and also convert it into a specific unit. An interface along these lines:

   Distance d = Distance.fromMiles(5.6); // static factory method
   Distance d2 = Distance.fromCentimeters(1.2); // static factory method
   
   double meters = d.toMeters(); 
   double millimeters = d2.toMillimeters();
   

2. Your shoeSizeToCent method has a massive problem: Equality comparison with floating point is bound to go wrong. So yes you should make your shoe sizes an enum or maybe a string.

3. shoeSizeToCent: Instead of silently returning the input if none of the cases match consider throwing an exception to reject invalid input.
4. Define an enum for your units rather than using strings.