3
\$\begingroup\$

I have this Python Package called ToTemp under development for precise Temperature Conversions and currently thinking in changing the method's implementation to be able to convert multiple values and return an Iterable with those converted values (in a list).

It would be good to see some reviews on this.

Default implementation:

    class Rankine:
    """Provides conversion of Rankine to other temperature scales"""

        @staticmethod
        def to_celsius(rankine: float | int, /, *, float_ret=True) -> float | int:
        """
        Converts Rankine to Celsius, returning a float by default.

        If the float_ret parameter is False, it returns an approximate int value
        (using the math's module trunc function).

        :param rankine: Rankine value(s) to be converted
        :param float_ret: Optional, True by default to return floats
        :return: float, int or Iterable
        """
        if float_ret:
            return float((rankine - 491.67) * 5/9)
        return trunc((rankine - 491.67) * 5/9)

Now, with the iter() method:

    class Rankine:
    """Provides conversion of Rankine to other temperature scales"""

        @staticmethod
        def to_celsius(rankine: float | int | Iterable, /, *, float_ret=True) -> float | int | Iterable:
            """
            Converts Rankine to Celsius, returning a float by default.

            If the float_ret parameter is False, it returns an approximate int value
            (using the math's module trunc function).

            :param rankine: Rankine value(s) to be converted
            :param float_ret: Optional, True by default to return floats
            :return: float, int or Iterable
            """
            try:
                iter(rankine)
                if float_ret:
                    return [float((value - 491.67) * 5/9) for value in rankine]
                return [trunc((value - 491.67) * 5/9) for value in rankine]
            except TypeError:
                if float_ret:
                    return float((rankine - 491.67) * 5/9)
                return trunc((rankine - 491.67) * 5/9)
\$\endgroup\$

2 Answers 2

6
\$\begingroup\$

Unit Typing

You are under-using the Rankine class, and I presume the other temperature classes.

Primitive Obsession (only manipulating primitive types) means never being quite sure which unit a type is in. When you have a temperature value in hand, is it expressed in Kelvin? Rankine? Celsius? Anything else? In fact... is it even a temperature?

Instead, make Rankine a @dataclass:

from dataclasses import dataclass
from typing import Generic, NewType, TypeVar

T = TypeVar('T', bound=int | float)

@dataclass
class Rankine(Generic[T]):
    value: T

Then, you can write conversion methods in a generic manner.

User's Pick

Let the user pick the type they wish automatically, and do not change it "under their back" with an option:

    def to_rankine(self) -> 'Rankine[T]':
        return Rankine(self.value)

    def to_celsius(self) -> 'Celsius[T]':
        celsius = type(self.value)((self.value - 491.67) * 5/9)

        return Celsius(celsius)

Instead, provide explicit methods to change the underlying type:

    def with_float(self) -> 'Rankine[float]':
        return Rankine(float(self.value))

    def with_int(self) -> 'Rankine[int]':
        return Rankine(int(self.value))

This means that any underlying type change is explicit, rather than the result of accidentally using the default option.

\$\endgroup\$
5
  • \$\begingroup\$ Can't I let the user put a list/tuple? Like: Rankine([12, 44.5, 25])? \$\endgroup\$
    – eddyxide
    Commented Nov 22, 2022 at 15:47
  • 1
    \$\begingroup\$ I would advise against it. From a modelling point of view, what you'd want out is a List[Rankine[float]] (or List[Rankine[int]]), you could use a dedicated function to do so, but map(Rankine, [12.0, 44.5, 25.0]) should also do the job. \$\endgroup\$ Commented Nov 22, 2022 at 15:50
  • \$\begingroup\$ Something like: rankine_temps = [12, 24.55, 95, 999] map(Rankine.to_celsius, rankine_temps) ? \$\endgroup\$
    – eddyxide
    Commented Nov 22, 2022 at 15:55
  • 1
    \$\begingroup\$ You'd need something like temps = map(Rankine, [12, 24.55, 95, 999]); map(Rankine.to_celsius, temps). First identify the correct unit of each numeric value (Rankine here), then convert it into another unit (Celsius here). You could also just use a more complex function to avoid materalizing a list of rankine temps just to throw it away. \$\endgroup\$ Commented Nov 22, 2022 at 15:59
  • \$\begingroup\$ "avoid materalizing a list of rankine temps just to throw it away", didn't understood this, can you explain it more? \$\endgroup\$
    – eddyxide
    Commented Nov 22, 2022 at 16:20
12
\$\begingroup\$

Let the caller handle iterables

I think allowing an iterable is a bad idea. The caller can already use map(Rankine.to_celsius, temps) or the like with your first version. And it should be up to the caller to process an iterable.

Image a caller passing in a huge generator and you construct a large list that might result in an OOM error condition. Don't force the caller to use a list as iterable. Also your type hint is misleading, since it will not return an Iterable, but explicitly a list.

Handle return type dependent on the input type

As for the float_ret: I don't like that either. If you want to return either a float or an int, I'd change the API to return the type that's been passed in:

from typing import TypeVar


T = TypeVar('T', bound=int | float)


class Rankine:
    """Provides conversion of Rankine to other temperature scales"""

    @staticmethod
    def to_celsius(rankine: T) -> T:
        ...

        return type(rankine)((rankine - 491.67) * 5/9)

...

print(Rankine.to_celsius(12))
print(Rankine.to_celsius(12.0))

Make sure to document this behaviour in the docstring.

Stop writing classes

And as Rankine only contains one classmethod, you can just refactor it to a pure function:

from typing import TypeVar


T = TypeVar('T', bound=int | float)


def rankine_to_celsius(rankine: T) -> T:
    ...

    return type(rankine)((rankine - 491.67) * 5/9)

...

print(rankine_to_celsius(12))
print(rankine_to_celsius(12.0))
\$\endgroup\$
6
  • \$\begingroup\$ Ty for all those very nice observations Richard! I only showed one method, but there's other 6 conversions in each <temp_scale> class. \$\endgroup\$
    – eddyxide
    Commented Nov 22, 2022 at 1:46
  • 4
    \$\begingroup\$ Why are there 6 conversion functions in each class? You just need a slope and offset to convert any value to/from kelvins, and therefore to any other temperature scale. \$\endgroup\$ Commented Nov 22, 2022 at 7:55
  • 5
    \$\begingroup\$ @eddyxide I can only review the code, you present here. \$\endgroup\$ Commented Nov 22, 2022 at 8:12
  • \$\begingroup\$ @TobySpeight could you show me an example? \$\endgroup\$
    – eddyxide
    Commented Nov 22, 2022 at 19:27
  • \$\begingroup\$ Yes - one in C++. \$\endgroup\$ Commented Nov 22, 2022 at 19:30

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.