1
\$\begingroup\$

Problem statement:

Write a program that converts all given temperatures from a given input temperature scale to a given output temperature scale. The temperature scales to be supported are Kelvin, Celsius, Fahrenheit, Rankine, Delisle, Newton, Rømer, Réaumur.

Synopsis: tempconv INPUT_SCALE OUTPUT_SCALE [TEMPERATURE]...

The INPUT_SCALE and OUTPUT_SCALE shall be given as follows:

  • K for Kelvin
  • C for Celsius
  • F for Fahrenheit
  • R for Rankine
  • D for Delisle
  • N for Newton
  • for Rømer
  • for Réaumur.

Example:

tempconv K C 0 273.15 373.15
-273.15
0.0
100.0

My solution in Java:

import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.DoubleUnaryOperator;
import java.util.stream.Stream;
import static java.util.Map.entry;
import static java.util.stream.Collectors.toUnmodifiableMap;

enum TemperatureConverter {
    KELVIN    (kelvin     -> kelvin,                           kelvin -> kelvin,                            "K", "k"),
    CELSIUS   (celsius    -> celsius + 273.15,                 kelvin -> kelvin - 273.15,                   "°C", "C", "c"),
    DELISLE   (delisle    -> 373.15 - delisle * 2 / 3,         kelvin -> (373.15 - kelvin) * 3 / 2,         "°De", "De", "DE", "de"),
    FAHRENHEIT(fahrenheit -> (fahrenheit + 459.67) * 5 / 9,    kelvin -> kelvin * 9 / 5 - 459.67,           "°F", "F", "f"),
    NEWTON    (newton     -> newton * 100 / 33 + 273.15,       kelvin -> (kelvin - 273.15) * 33 / 100,      "°N", "N", "n"),
    RANKINE   (rankine    -> rankine * 5 / 9,                  kelvin -> kelvin * 9 / 5,                    "°R", "R", "r"),
    RÉAUMUR   (réaumur    -> réaumur * 5 / 4 + 273.15,         kelvin -> (kelvin - 273.15) * 4 / 5,         "°Ré", "°Re", "Ré", "RÉ", "ré", "Re", "RE", "re"),
    RØMER     (rømer      -> (rømer - 7.5) * 40 / 21 + 273.15, kelvin -> (kelvin - 273.15) * 21 / 40 + 7.5, "°Rø", "°Ro", "Rø", "RØ", "rø", "Ro", "RO", "ro"),
    ;

    private static final Map<String, TemperatureConverter> converters =
        Stream.of(values())
            .flatMap(tc -> tc.names.stream().map(name -> entry(name, tc)))
            .collect(toUnmodifiableMap(e -> e.getKey(), e -> e.getValue()));

    private final Set<String> names;
    final DoubleUnaryOperator toKelvin;
    final DoubleUnaryOperator fromKelvin;

    TemperatureConverter(final DoubleUnaryOperator toKelvin, final DoubleUnaryOperator fromKelvin, final String... names) {
        this.toKelvin = toKelvin;
        this.fromKelvin = fromKelvin;
        this.names = Set.of(names);
    }

    public static TemperatureConverter get(final String name) {
        return converters.get(name);
    }

    public static void main(final String... args) {
        var toKelvin = TemperatureConverter.get(args[0]).toKelvin;
        var fromKelvin = TemperatureConverter.get(args[1]).fromKelvin;
        Stream
            .of(args)
            .skip(2)
            .mapToDouble(Double::parseDouble)
            .map(toKelvin)
            .map(fromKelvin)
            .forEach(System.out::println);
    }
}

Note: I'm aware that the lines that define the enum constants are a bit long. In this case, I prefer to sacrifice a bit of line length to retain a tabular format.

\$\endgroup\$
1
  • \$\begingroup\$ Same as my other review: store only an offset and coefficient for each unit, not a lambda \$\endgroup\$
    – Reinderien
    Sep 7, 2022 at 21:32

2 Answers 2

3
\$\begingroup\$

Temperature scale enum

I'd redesign the temperature scales enum.

One of the outstanding features of Java's enums is that they are full-featured classes, supporting not only fields, but also methods with polymorphism.

Your TemperatureConverter enum values currently hold functions (DoubleUnaryOperator instances) that have to be passed to the outside to do anything with these functions. A much more straightforward approach (IMHO) would be to create a toKelvin() and a fromKelvin() method inside the enum. So, you can e.g. write

double kelvin = tempScaleOfInput.toKelvin(input);
double output = tempScaleOfOutput.fromKelvin(kelvin);

For the implementation of these methods, I see two alternatives:

  • Store offset and factor (following @Reinderien's comment). This way, you avoid the redundant (and thus error-prone) coding of many forward and backward conversions. This way, you can support all linear conversions with one implementation (and the scales mentioned only need linear conversions).

  • Give each enum value its own implementation of toKelvin() and fromKelvin(). Then the enum definition gets abstract declarations of toKelvin() and fromKelvin(), and you implement them individually for each enum value. This way, you can support arbitrary conversions, even non-linear ones.

Of course, a mixture is also possible: have a scale/factor based implementation in the base enum class, and override it in temperature scales that need non-linear behaviour.

Class name

A class name should describe what the instances of this class represent, preferrably based on everyday language, or based on the technical role. Your instances are the enum constants like CELSIUS.

  • In everyday language, I wouldn't call "celsius" a temperature converter, but a temperature scale.
  • In your code, the enum values like CELSIUS don't do anything, they just hold recipes how to do conversions. So, calling them "converters" is a bit misleading.

So, judging from both points of view, I'd suggest to rename the enum to TemperatureScale.

Converter class

For a temperature conversion, I'd expect a method like

double convert(double input, TemperatureScale inputScale, TemperatureScale outputScale)

to exist in an appropriate class, and a good name for this class would be TemperatureConverter.

Separation of concerns

You currently pack different tasks into one class:

  • implement the model of a temperature scale with the ability to convert to and from the Kelvin base scale,
  • the user interface, getting input from the command line, and presenting output to the console,
  • overall control, i.e. putting the pieces together so input gets accepted, parsed, directed to the conversion, and the results presented as output.

In your simple case, it might be overkill to do a proper separation of concerns, but in more complex professional software, each of these aspects goes into a class of its own (or typically, a package of related classes).

Long streams method chain

Personally, I don't like long chains of method calls. It makes it difficult to understand what the intermediate results mean. Doing smaller steps, and giving proper names to the step results makes code much more readable.

With Java streams, I very often see this style of method chaining, as if it were mandatory to do all streams operations in one long chain of method calls. Of course, this isn't true. You can as well write e.g.:

var inputStringsStream = Stream
        .of(args)
        .skip(2);
var inputValuesStream = inputStringsStream
        .mapToDouble(Double::parseDouble);
var outputValuesStream = inputValuesStream
        .map(toKelvin)
        .map(fromKelvin);
outputValuesStream.forEach(System.out::println);

And, to even further improve readability, you might want to replace var with the proper Java Stream type at that stage.

\$\endgroup\$
1
\$\begingroup\$

Just a random collection of thoughts:

  • The biggest problem is I see is the total lack of input validation, which will lead to unhelpful exceptions being thrown to the user.

  • It may be worth considering using BigDecimal instead of doubles to avoid displaying unnecessary extra (and wrong) decimal places to the user.

  • Be careful with using non-ASCII characters in identifiers. Personally I think it works here, and adds a bit a "fun" to the code, but may make it difficult for other people to edit the code.

\$\endgroup\$

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.