2
\$\begingroup\$

I'm trying to make a imperial/metric length unit converter in Python. Here is my full code:

Metric = MM, CM, DM ,M, DAM, HM, KM = ('mm', 'cm','dm','m', 'dam' ,'hm', 'km')
Imperial = IN, FT, YD, MI, NM, barleycorn = ('in', 'ft', 'yd', 'mi', 'nm', 'barleycorn')
Vaildunit = Metric + Imperial
# defining 
def valid(unit):
    if unit not in Vaildunit:
        print('\nnot in list. the unit need to be in ' + ' '.join(Vaildunit))
        return False
    return True

def convert2(val, unit_in, unit_out):
    sol = 0.0
    n = 0.0
    standard = {MM: 0.001, CM: 0.01, DM : 0.1, M: 1.0, DAM : 10.0, HM : 100.0, KM: 1000.0}

    # imperial to imperial
    if unit_in in Metric and unit_out in Metric:   
        sol = val*(standard[unit_in]/standard[unit_out])

    # imperial to imperial
    if unit_in in Imperial and unit_out in Imperial:
        if unit_in in [FT] and unit_out in [IN]:
            sol = val * 12
        if unit_in in [IN] and unit_out in [FT]:
            sol = val / 12
        if unit_in in [FT] and unit_out in [MI]:
            sol = val / 5280
        if unit_in in [MI] and unit_out in [FT]:
            sol = val * 5280
        if unit_in in [YD] and unit_out in [IN]:
            sol = val * 36
        if unit_in in [IN] and unit_out in [YD]:
            sol = val / 36
        if unit_in in [FT] and unit_out in [YD]:
            sol = val * 3
        if unit_in in [YD] and unit_out in [FT]:
            sol = val / 3
        if unit_in in [NM] and unit_out in [IN]:
            sol = val * 72913.4
        if unit_in in [NM] and unit_out in [FT]:
            sol = val * 6076.12
        if unit_in in [NM] and unit_out in [YD]:
             sol = val * 2025.32
        if unit_in in [NM] and unit_out in [MI]:
             sol = val * 1.15078
        if unit_in in [barleycorn] and unit_out in [IN]:
            sol = val / 3
        if unit_in in [IN] and unit_out in [barleycorn]:
            sol = val * 3

    # Metric to imperial
    if unit_in in Metric and unit_out in Imperial:
        if unit_out in [FT]:
            n = val*3.28084
        if unit_out in [IN]:
            n = val*39.37008
        if unit_out in [MI]:
            n = val/1609.35
        if unit_out in [YD]:
            n = val*1.0936132983
        if unit_out in [NM]:
            n = val / 0.000539957
        sol = n * standard[unit_in]

    # Imperial to Metric
    if unit_in in Imperial and unit_out in Metric:
        standard = {MM: 0.001, CM: 0.01, DM : 0.1, M: 1.0, DAM : 10.0, HM : 100.0, KM: 1000.0}
        if unit_in in [FT]:
            n = val / 3.28084
        if unit_in in [IN]:
            n = val / 39.37008
        if unit_in in [MI]:
            n = val * 1609.34    
        if unit_in in [YD]:
            n = val / 1.0936132983
        if unit_in in [NM]:
            n = val * 0.000539957
        sol = n / standard[unit_out]
    return sol
# check the list
def isfloat(string):
    try:
        float(string)
        return True
    except ValueError:
        return False

In the imperial-to-imperial part, the code is too long to use. I would like to shorten that part of the code.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ too long to use? \$\endgroup\$
    – depperm
    Feb 8, 2023 at 13:53
  • \$\begingroup\$ I mean its too long so it seems very unclear \$\endgroup\$
    – Rozy0930
    Feb 8, 2023 at 14:03

3 Answers 3

5
\$\begingroup\$

There's no point to keeping separate variables for Metric, MM, etc. This can reduce to one dictionary.

Vaildunit should be spelled VALID_UNIT - capitals because it's a global constant.

Don't write any if statements for this purpose.

Don't bother writing logic specifically to convert from yards to inches, etc. Just convert everything referenced to a single base unit, metres.

Don't write an equality test like this: in [NM]; instead, just compare == NM.

Your conversion quantities such as 1.0936132983 lose accuracy because they aren't expressed in terms of fundamental conversion definitions. I demonstrate the alternative below.

Suggested

UNITS = {
    'mm': 0.001,
    'cm': 0.01,
    'dm': 0.1,
    'm': 1,
    'dam': 10,
    'hm': 100,
    'km': 1000,
    'barleycorn': 0.0254/3,
    'in': 0.0254,
    'ft': 0.3048,
    'yd': 0.9144,
    'mi': 1609.344,
    'nm': 1852,
}


def convert(val: float, unit_in: str, unit_out: str) -> float:
    return val * UNITS[unit_in] / UNITS[unit_out]
\$\endgroup\$
2
\$\begingroup\$

If you think about it, the whole idea is only about multiplying your input value by some ratio that is determined by units of your output and input. You can define this nicely as data structure, something like:

unit_to_unit_ratios = {
    MM: {
        MM: 1,
        IN: 0.0393701,
        YE:...
    },
    FT: {
        ...
    }
}

Then your final code is simply just:

unit_to_unit_ratios[input_unit][output_unit] * input_value

You can optimize this, for example by checking if input and output units are equal and then just return input_value so that "1" value doesn't have to be in the dictionary. Depends if you want to have longer code or longer data structure. Both has advantages and disadvantages.

And you don't need to make any kind of difference between imperial and metric units.

\$\endgroup\$
2
  • 3
    \$\begingroup\$ I do not recommend this approach. The number of conversions you will need to express scales with O(n²). Instead prefer a system where every unit is only defined against a base unit like metres. \$\endgroup\$
    – Reinderien
    Feb 8, 2023 at 15:12
  • 1
    \$\begingroup\$ Good point there! \$\endgroup\$
    – K.H.
    Feb 8, 2023 at 15:25
0
\$\begingroup\$

The keys of the dictionary ( Imperial_to_Imperial ) are tuples that represent the input and output units, and the values are the conversion factors.

With use of dictionary you can reduce multiple if else syntax with simple dictionary object.

You can try shorter version of code :

Imperial_to_Imperial = {
    (FT, IN): 12,
    (IN, FT): 1 / 12,
    (FT, MI): 1 / 5280,
    (MI, FT): 5280,
    (YD, IN): 36,
    (IN, YD): 1 / 36,
    (FT, YD): 3,
    (YD, FT): 1 / 3,
    (NM, IN): 72913.4,
    (NM, FT): 6076.12,
    (NM, YD): 2025.32,
    (NM, MI): 1.15078,
    (barleycorn, IN): 1 / 3,
    (IN, barleycorn): 3,
    }

Usecase :

sol = val * Imperial_to_Imperial[(unit_in, unit_out)]
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Welcome to Code Review! You have presented an alternative solution, but haven't reviewed the code. Please edit to show what aspects of the question code prompted you to write this version, and in what ways it's an improvement over the original. It may be worth (re-)reading How to Answer. \$\endgroup\$ Feb 9, 2023 at 14:01
  • 2
    \$\begingroup\$ @TobySpeight The user made an indirect observation that the code is too verbose. \$\endgroup\$
    – pacmaninbw
    Feb 9, 2023 at 15:16
  • \$\begingroup\$ @TobySpeight He improved a part of the code. This is a useful feedback, in my humble opinion \$\endgroup\$ Feb 9, 2023 at 18:06
  • 1
    \$\begingroup\$ @Billal - it may be improved, but without any explanation of why it's better, it's just an independent implementation that is not a review. \$\endgroup\$ Feb 9, 2023 at 19:49

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.