Skip to main content
Code Review

Return to Answer

added 588 characters in body
Source Link
JDługosz
  • 11.7k
  • 19
  • 40

Your original code had n×ばつn different code paths for every possible conversion. That’s not scalable. By knowing only how to convert whatever to/from kelvin, I only need n. You also have helper functions setting global variables — that’s bad. In your code you could have easily just returned the value from the function instead. I do that many times in the code below; that is, the helper function supplies a value which the caller saves in a named variable.

(I don’t like how std::find_if requires separate begin/end parameters when I want to give it a whole collection. Boost’s range form is better here, but I stuck with std functions.)

Your original code had n×ばつn different code paths for every possible conversion. That’s not scalable. By knowing only how to convert whatever to/from kelvin, I only need n.

Your original code had n×ばつn different code paths for every possible conversion. That’s not scalable. By knowing only how to convert whatever to/from kelvin, I only need n. You also have helper functions setting global variables — that’s bad. In your code you could have easily just returned the value from the function instead. I do that many times in the code below; that is, the helper function supplies a value which the caller saves in a named variable.

(I don’t like how std::find_if requires separate begin/end parameters when I want to give it a whole collection. Boost’s range form is better here, but I stuck with std functions.)

Source Link
JDługosz
  • 11.7k
  • 19
  • 40

First, you are getting the real work mixed up with the I/O of the testing. In real code, you’ll have to supply some functionality such as converting temperature, and it works by taking inputs and producing outputs. Some other part of the program will use that while driving the GUI, or in this case, a unit test program will call it. Rather than ask the user for the test cases (that gets old fast) it might read them from a file or have them compiled in to the tester.

So, I have here a function convert that does the work, and call it from a testing function.

From the top: I use a typedef instead of double everywhere. Besides not locked into double if this ever changes, the name documents which variables and parameters hold that. Imagine a function that takes 5 doubles and 3 ints — which is which? If it takes a temperature, a cost, a time, etc. then the positions are clearer, even in cases where the compiler doesn’t check it.

Your original code had n×ばつn different code paths for every possible conversion. That’s not scalable. By knowing only how to convert whatever to/from kelvin, I only need n.

Rather than a separate function for each conversion, I see your code is repeating almost exactly the same thing each time. That is, the conversion factor should be a parameter to universal convert code.

So, I start by defining the scales in an array of structures. The rest of the code will be unchanged if I add more or otherwise change this! For example, I added Rankine and recompiled, and nothing needed to be fixed.

So, the convert function looks up the definitions for each scale, and uses the values it finds in those records.

Now the convert function, like everything else, uses an enumeration type for the different scale choices. That’s fine for hard-coded uses, but how do you read that from a file or populate a pull-down field? I supplied a find_scale function that takes an initial and returns the matching scale record. Note that it’s not a case statement (nor an if statement)! Instead, it’s a call to find_if passed over the array of scales it knows about. That’s why it understood 'R' without having to update the function!

The stuff to ship is at the top, and then there is a clear separator with the tester below. In real life, that would be a separate file, and the stuff would be packaged in a header + implementation files, and use namespaces. But you get the idea of separating.

The declaration using std::cout; is just fine inside the tester code, or in CPP files (not .h files) in general.

#include <iostream>
#include <algorithm>
#include <stdexcept>
#include <string>
#include <type_traits>
using temperature_value = double; // used alone, represents kelvin.
enum class temperature_scale {
 kelvin = 0,
 Celsius, Fahrenheit, Rankine
 // others can be added.
};
struct temperature { // express temperature unambiguously with units
 temperature_value v;
 temperature_scale scale;
};
struct temp_scale_info_t {
 temperature_scale scale;
 const char* name;
 // conversion is linear equn Y=mX+b, where input X is value in k
 // and output Y is value in this scale.
 double m, b;
};
constexpr temp_scale_info_t temp_scale_info[] = {
 { temperature_scale::kelvin, "kelvin", 1.0, 0.0 },
 { temperature_scale::Celsius, "Celsius", 1.0, -273.15 },
 { temperature_scale::Fahrenheit, "Fahrenheit", 1.8, -459.67 },
 { temperature_scale::Rankine, "Rankine", 1.8, 0.0 }
};
const temp_scale_info_t& find_scale (char ch)
{
 const int case_difference = 'a' - 'A';
 if (ch >= 'a' && ch <= 'z') ch = char(ch - case_difference);
 const char ch2 = char(ch + case_difference);
 auto it= std::find_if (std::begin(temp_scale_info), std::end(temp_scale_info),
 [=](const auto& rec) { char initial=rec.name[0]; return initial==ch || initial==ch2; }
 );
 if (it == std::end(temp_scale_info)) throw std::invalid_argument("no such scale");
 return *it;
}
const temp_scale_info_t& get_info (temperature_scale scale)
{
 size_t index= size_t(scale);
 if (index >= std::extent_v<decltype(temp_scale_info)>) throw std::invalid_argument("no such scale");
 const auto& rec= temp_scale_info[index];
 if (rec.scale != scale) throw std::logic_error("temp_scale_info table set up incorrectly");
 return rec;
}
temperature convert (temperature t_in, temperature_scale wanted)
{
 if (t_in.scale == wanted) return t_in;
 const auto& recin= get_info (t_in.scale);
 // convert input to k
 // Y = mX + b, so X = (Y-b)/m
 temperature_value val_in_k = (t_in.v-recin.b)/recin.m;
 // convert to desired output
 const auto& recout= get_info (wanted);
 return { recout.m*val_in_k + recout.b, wanted };
}
const char* name (temperature_scale scale)
{
 const auto& rec= get_info(scale);
 return rec.name;
}
std::string format (temperature t)
{
 std::string result = std::to_string(t.v);
 result.push_back (' ');
 if (t.scale != temperature_scale::kelvin) result.append ("degrees ");
 result.append(name(t.scale));
 return result;
}

// ==============================

using std::cout;
void testit (temperature t_in, temperature_scale wanted)
{
 temperature t_out = convert (t_in, wanted);
 cout << format(t_in) << " is " << format(t_out) << '\n';
}
void run_tests()
{
 testit ({ 0,temperature_scale::Celsius }, temperature_scale::Fahrenheit);
 testit ({ 0,temperature_scale::Celsius }, temperature_scale::kelvin);
 testit ({ 0,temperature_scale::kelvin }, temperature_scale::Celsius);
 testit ({ 100,temperature_scale::Celsius }, temperature_scale::Fahrenheit);
 testit ({ -65,temperature_scale::Fahrenheit }, temperature_scale::Celsius);
 testit ({ -65,temperature_scale::Fahrenheit}, temperature_scale::kelvin);
 auto dest_scale = find_scale('R').scale; // user input, file parsing, etc. will use this
 testit ({ -65,temperature_scale::Fahrenheit }, dest_scale);
}
int main()
{
 cout << "running\n";
 try {
 run_tests();
 }
 catch (std::exception& ex) {
 cout << "ERROR: exception " << ex.what() << '\n';
 }
}
lang-cpp

AltStyle によって変換されたページ (->オリジナル) /