I am doing a college exercise and would like to know how to improve the execution of the idea. The idea is to create a program that converts temperature measurements.
Main.java
package app.cnv;
public class Main {
public static void main(String[] args) {
Celsius celsius = new Celsius(37);
System.out.println(celsius.convert(new Fahrenheit()));
Fahrenheit fahrenheit = new Fahrenheit(98.6);
try {
System.out.println(fahrenheit.convert(new Celsius()));
} catch (AbsoluteZeroException e) {
System.out.println(e.getMessage());
}
}
}
Temperature.java
package app.cnv;
public abstract class Temperature {
public abstract double convert(Temperature to) throws AbsoluteZeroException;
}
Celsius.java
package app.cnv;
public class Celsius extends Temperature {
double celsius;
Celsius() {
}
Celsius(double celsius) {
this.celsius = celsius;
}
@Override
public double convert(Temperature to) {
if (to instanceof Fahrenheit) {
return (celsius * 9) / 5 + 32;
}
return celsius;
}
}
Fahrenheit.java
package app.cnv;
public class Fahrenheit extends Temperature {
double fahrenheit;
Fahrenheit() {
}
Fahrenheit(double fahrenheit) {
this.fahrenheit = fahrenheit;
}
@Override
public double convert(Temperature to) throws AbsoluteZeroException {
if (to instanceof Celsius) {
if (fahrenheit < -459.67) {
throw new AbsoluteZeroException("The temperature cannot be less than absolute zero.");
} else {
return ((fahrenheit - 32) * 5) / 9;
}
}
return fahrenheit;
}
}
AbsoluteZeroException.java
package app.cnv;
public class AbsoluteZeroException extends Exception {
public AbsoluteZeroException(String err) {
super(err);
}
}
-
8\$\begingroup\$ Wishing to practice is OK but I'd agree with the others that using 5 classes for such a simple problem seems overwrought. Perhaps you might come up with a more ambitious idea that requires a more complicated scheme like this. Also: I see no Java-doc comments at all, which is bad for complicated classes. Other than that, I think your concepts are good, now you just need a more suitable project. \$\endgroup\$markspace– markspace2019年12月17日 06:01:42 +00:00Commented Dec 17, 2019 at 6:01
-
\$\begingroup\$ Fahrenheit and Celsius are very dependent on each other now, are you sure that's what you want? \$\endgroup\$Mast– Mast ♦2019年12月17日 17:55:53 +00:00Commented Dec 17, 2019 at 17:55
-
\$\begingroup\$ Do you have a usage example with this? Because I don't think this is as useful as you wanted it to be and a usage example may just show this to yourself. \$\endgroup\$Mast– Mast ♦2019年12月17日 18:56:21 +00:00Commented Dec 17, 2019 at 18:56
-
3\$\begingroup\$ I'm not sure what other Temperature scales you might want to use but if you wanted to make a universal extensible converter, you can make the Temperature interface require conversion to and from Kelvin. Then you can convert any scale to any other by first converting to Kelvin and then to the desired scale. \$\endgroup\$JimmyJames– JimmyJames2019年12月17日 20:19:49 +00:00Commented Dec 17, 2019 at 20:19
-
\$\begingroup\$ Related issue (sort of): codereview.stackexchange.com/questions/87594/length-converter \$\endgroup\$Quintec– Quintec2019年12月18日 00:54:40 +00:00Commented Dec 18, 2019 at 0:54
7 Answers 7
What if I add a class Kelvin
without modifying the other classes? When I convert Celsius
to Kelvin
then, your code says that 20 °C = 20 K, which is simply wrong.
Your whole approach is doomed to fail. What you should do instead is to define one reference temperature scale. I would take Kelvin for that. Then, for every other temperature scale, have methods fromKelvin
and toKelvin
. That's all. This approach is well-known from constructing compilers, which has the same problem of several similar source languages and several different target machines.
For converting between arbitrary temperature scales you can have a helper method in the Temperature
class like this:
public static Temperature convert(Temperature from, TemperatureScale to) {
double kelvin = from.toKelvin();
return to.fromKelvin(kelvin);
}
You can see that there are two completely different classes involved:
- A temperature scale, like Kelvin or Fahrenheit
- A specific temperature, consisting of a temperature scale and a value, like 293 Kelvin
Regarding the exception class: There is no good reason to give it a String
parameter. The name of the class is already expressive enough. Therefore you should remove that parameter and just throw new AbsoluteZeroException()
. You should probably rename it to BelowAbsoluteZeroException
. Or you should just take the predefined IllegalArgumentException
and be fine. In many cases you don't need to invent your own exception classes.
-
5\$\begingroup\$ The
AbsoluteZeroException
(or below) could store the invalid number that caused it. \$\endgroup\$JollyJoker– JollyJoker2019年12月17日 08:22:48 +00:00Commented Dec 17, 2019 at 8:22
Using 2 classes here doesn't make sense.
Every time you add another class, you'll have to modify every classes conversion method.
There is a tight coupling between Celsius & Fahrenheit. You are using an instance of Fahrenheit to determine the type you want to convert to. This isn't necessary.
Note: In programming you want low coupling, high cohesion. Lots of information is available online
Temperatures would work better as an ENUM. Then you can do something like:
Tempature tempatureCels = new Tempature(98.6, TempatureType.CELSIUS);
Tempature tempatureFahrenheit = Tempature.convert(tempatureCels, TempatureType.FAHRENHEIT);
-
2\$\begingroup\$ I would do this, but not have a
Temperature
have any storedTemperatureType
. Your second row would bedouble temperatureFahrenheit = temperatureCels.getTemperature(TemperatureType.FAHRENHEIT)
(well the "Cels" in the name would be wrong but you see what I mean) \$\endgroup\$JollyJoker– JollyJoker2019年12月17日 08:29:01 +00:00Commented Dec 17, 2019 at 8:29 -
\$\begingroup\$ @JollyJoker how would you create the instance, then?
Temperature temperature = new Temperature(98.6)
doesn't make sense. As my elementary school teacher used to say:The temperature is 98.6 'what'? 98.6 oranges? apples? years?
\$\endgroup\$Ivan García Topete– Ivan García Topete2019年12月18日 15:11:01 +00:00Commented Dec 18, 2019 at 15:11 -
3\$\begingroup\$ @IvanGarcíaTopete
new Tempature(98.6, TempatureType.CELSIUS);
Have the constructor require a type, just using it to convert to kelvin. So the temperature itself has no type, but accessing the value always requires a type. \$\endgroup\$JollyJoker– JollyJoker2019年12月18日 15:16:20 +00:00Commented Dec 18, 2019 at 15:16 -
1\$\begingroup\$ @JollyJoker ok I get you, something like this answer. Then e.g.
temperature.Convert(TemperatureType.FARENHEIT)
\$\endgroup\$Ivan García Topete– Ivan García Topete2019年12月18日 15:34:33 +00:00Commented Dec 18, 2019 at 15:34 -
\$\begingroup\$ @IvanGarcíaTopete Exactly, upvoted that one. \$\endgroup\$JollyJoker– JollyJoker2019年12月18日 17:14:33 +00:00Commented Dec 18, 2019 at 17:14
You might follow the example of java.util.Date
. The internal representation of a Date
is always in UTC, and it's converted to other time zones and date formats as needed. Similarly, you could define a Temperature
class that is always in Kelvin, and a TemperatureScale
interface with as many implementations as you like to perform conversions and formatting.
This is a hard question to answer because this problem is so simple it simply does not need an object-oriented solution. You'd be much better just writing two methods named convertCToF
and convertFToC
which will throw an Exception
at absolute zero (if you desire).
Critique:
- You should make your
Constructor(...)
methodspublic
, as per convention - There is no need for empty constructors.
- If you are going the object-oriented route in Java, it's best practice to use encapsulation and declare member fields to be
private
and writeget
andset
methods for said fields. - You could instead declare an
interface
with a method calledconvert
and attach them to your classes, that way it will be mandatory to implement aconvert
method rather than having to use@Override
But overall you really need to reconsider if you actually need to use OOP for this. See this video covering the overuse of class
in Python. https://www.youtube.com/watch?v=o9pEzgHorH0
-
3\$\begingroup\$ Not sure what exactly you mean by OOP (since that term is ambiguous), but something you definitely need is a compiler that prevents you to add Fahrenheit to Celsius. Having different classes would accomplish that. Another way would be to store the number together with its associated measurement unit. You should definitely create a distinguished data type for temperature values. \$\endgroup\$Roland Illig– Roland Illig2019年12月17日 13:10:51 +00:00Commented Dec 17, 2019 at 13:10
-
1\$\begingroup\$ @RolandIllig adding and subtracting is a whole different kettle of fish. I saw an article on global warming once that said that the world was going to warm by 33.8 degrees Fahrenheit in the next century. That's the same as warming by nearly 19 degrees Celsius: A temperature of 1 degree Celsius is equal to 33.8 degrees Fahrenheit, but a difference in temperature of 1 degree Celsius is equal to a difference of 1.8 degrees Fahrenheit. If you need to support arithmetic then you may need separate sets of classes analogous to date/time types and time span types. \$\endgroup\$phoog– phoog2019年12月18日 00:09:05 +00:00Commented Dec 18, 2019 at 0:09
All this criticism of the object model is well founded, but don't forget that this is a college exercise. College exercises sometimes ask you to do something problematic so you can understand why it's problematic. A classic example of this is bubble sort. So I wouldn't focus on that too much unless you're in a relatively advanced programming course.
The first thing that leaps out at me is that you have classes Fahrenheit and Celsius, but your convert method doesn't return instances of those classes; it returns a double. The pattern of passing a valueless instance of Fahrenheit
or Celsius
to the conversion method to indicate the desired units of the function's double
output function is profoundly unidiomatic for OOP. I suspect that your professor is after a conversion method that converts an instance of Fahrenheit to an instance of Celsius that represents the same temperature.
You might have
public abstract class Temperature {
public abstract Celsius toCelsius();
public abstract Fahrenheit toFahrenheit();
}
public class Celsius extends Temperature {
double celsius;
Celsius() {
}
Celsius(double celsius) {
this.celsius = celsius;
}
@Override
public Celsius toCelsius() {
return this;
}
@Override
public Fahrenheit toFahrenheit() {
return new Fahrenheit(celsius * 1.8 + 32);
}
}
This isn't great object oriented design, but it might just be what your professor is looking for. If your class is more advanced, it's possible that the exercise is designed to teach a more advanced technique to support methods that convert from one class to another.
As suggested elsewhere, another approach might be to have a single temperature class that stores its magnitude in Kelvin and has factory methods like Temperature fromCelsius(double celsius)
for constructing instances from a given system of units, and instance methods like double toCelsius()
that return a value in a given system of units.
Enums are great for this sort of thing in my opinion, unless you want people to invent their own temperature representations. If you want full-fledged object oriented usage with method chaining etc., it's still possible with approach I present below (would require small changes tho). It all depends on a context you are gonna use it in. Since this is a college exercise, this fresh idea may 'broaden your horizons'.
enum Temperatures {
KELVIN {
public double toKelvin(double in) {
return in;
}
public double fromKelvin(double inKelvin) {
return inKelvin;
}
},
CELSIUS {
public double toKelvin(double in) {
return in + 273.15;
}
public double fromKelvin(double inKelvin) {
return inKelvin - 273.15;
}
},
FAHRENHEIT {
public double toKelvin(double in) {
return (in + 459.67) * (5.0 / 9.0);
}
public double fromKelvin(double inKelvin) {
return (inKelvin * (9.0 / 5.0)) - 459.67;
}
};
abstract double toKelvin(double in);
abstract double fromKelvin(double kelvin);
public double convert(double in, Temperatures to) {
double inKelvin = this.toKelvin(in);
return to.fromKelvin(inKelvin);
}
}
Then you can simply:
System.out.println(Temperatures.FAHRENHEIT.convert(95, Temperatures.CELSIUS));
In my opinion you should create a factory for creating a converter which would take an input of "source" scale and "target" scale, which would basically return a converter that first converts it to kelvin and only then to target scale. For example, the factory interface would be the following:
void addConverter(Converter c);
Converter getConverter(Scale from, Scale to)
Meanwhile the converter abstract interface would be the following:
public Temperature convert(Temperature from);
And then implement the converter in pairs: CelsiusKelvinConverter
and KelvinCelsiusConverter
.
In addition: avoid casting. If you need to cast, your interface is flawed.
Regardless, if you were to ask anyone, odds are they will have a different opinion regarding the matter. My suggestion is asking about and making your conclusions.