Converting lengths and weights using functions in C++

Question

Question is:

Write a program that asks the user if he or she wants to convert lengths or weights. If the user chooses lengths, then the program asks the user if he or she wants to convert from feet and inches to meters and centimeters or from meters and centimeters to feet and inches. If the user chooses weights, a similar question about pounds, ounces, kilograms, and grams is asked. The program then performs the desired conversion. I wrote the following code:

#include<iostream>
using namespace std;

void inputData1(double& ft, double& inch);
void calcLength_m_cm(double& ft, double& inch, double& m, double& cm);
void outputData1(double m, double cm);

void inputData2(double& m, double& cm);
void calcLength_ft_inch(double& m, double& cm, double& ft, double& inch);
void outputData2(double ft, double inch);

void inputData3(double& pd, double& ou);
void calcWeight_kg_g(double& pd, double& ou, double& kg, double& g);
void outputData3(double kg, double g);

void inputData4(double& kg, double& g);
void calcWeight_pd_ou(double& kg, double& g, double& pd, double& ou);
void outputData4(double pd, double ou);

double feets, inches, meters, centimeters, pounds, ounces, kilograms, grams;

int main()
{
  char ch;
  do
  {
    int choice1, choice2;
    cout<<"Enter 1 for converting lengths and 2 for converting weights = ";
    cin>>choice1;
    if(choice1 == 1)
    {
      cout<<"Enter 1 for coverting meters and centimeters from feets and inches\n"
          <<"OR\n"
          <<"Enter 2 for converting feets and inches from meters and centimeters\n"
          <<">>";
      cin>>choice2;
      if(choice2 == 1)
      {
        inputData1(feets, inches);
        outputData1(meters, centimeters);
      }
      else if(choice2 == 2)
      {
        inputData2(meters, centimeters);
        outputData2(feets, inches);
      }
      else
      {
        cout<<"This input is invalid.\n"
            <<"You can only choose from 1 or 2.";
      }
    }
    else if(choice1 == 2)
    {
      cout<<"Enter 1 for converting kilograms and grams from poundes and ounces\n"
          <<"OR\n"
          <<"Enter 2 for converting pounds and ounces from kilograms and grams\n"
          <<">>";
      cin>>choice2;
      if(choice2 == 1)
      {
        inputData3(pounds, ounces);
        outputData3(kilograms, grams);
      }
      else if(choice2 == 2)
      {
        inputData4(kilograms, grams);
        outputData4(pounds, ounces);
      }
      else
      {
        cout<<"This input is invalid.\n"
            <<"You can only choose from 1 or 2.";
      }
    }
    else
    {
      cout<<"This input is invalid.\n"
          <<"You can only choose from 1 or 2.";
    }
    cout<<"\nEnter y to repeat: ";
    cin>>ch;
  } while (ch=='y' || ch=='Y');
}

void inputData1(double& ft, double& inch)
{
  cout<<"Enter lenghts in feet and inches\n"
      <<"Feet = ";
  cin>>ft;
  cout<<"Inches = ";
  cin>>inch;
  calcLength_m_cm(feets, inches, meters, centimeters);
}

void calcLength_m_cm(double& ft, double& inch, double& m, double& cm)
{
  m = (ft*0.3048) + (inch*0.0234);
  cm = m * 100;
}

void outputData1(double m, double cm)
{
  cout<<"Meters = "<<m<<" m\n";
  cout<<"Centimeters = "<<cm<<" cm\n";
}

void inputData2(double& m, double& cm)
{
  cout<<"Enter length in meters and centimeters\n"
      <<"Meter = ";
  cin>>m;
  cout<<"Centimeter = ";
  cin>>cm;
  calcLength_ft_inch(meters, centimeters, feets, inches);
}

void calcLength_ft_inch(double& m, double& cm, double& ft, double& inch)
{
  ft = (m*3.28) + (cm*0.03281);
  inch = ft*12;
}

void outputData2(double ft, double inch)
{
  cout<<"Feets = "<<ft<<" ft\n";
  cout<<"Inches = "<<inch<<" mn\n";
}

void inputData3(double& pd, double& ou)
{
  cout<<"Enter weight in pounds and ounces:\n"
      <<"Pounds = ";
  cin>>pd;
  cout<<"Ounces = ";
  cin>>ou;
  calcWeight_kg_g(pounds, ounces, kilograms, grams);
}

void calcWeight_kg_g(double& pd, double& ou, double& kg, double& g)
{
  kg = (pd/2.2046) + (ou/35.2736);
  g = kg*1000;
}
void outputData3(double kg, double g)
{
  cout<<"Kilograms = "<<kg<<" kg\n";
  cout<<"Grams = "<<g<<" g";
}

void inputData4(double& kg, double& g)
{
  cout<<"Enter weight in kilograms and grams:\n"
      <<"Kilograms = ";
  cin>>kg;
  cout<<"Grams = ";
  cin>>g;
  calcWeight_pd_ou(kilograms, grams, pounds, ounces);
}

void calcWeight_pd_ou(double& kg, double& g, double& pd, double& ou)
{
  pd = (kg*2.2046) + (g*.0022046);
  ou = pd*16;
}

void outputData4(double pd, double ou)
{
  cout<<"Pounds = "<<pd<<" lb\n";
  cout<<"Ounces = "<<ou;
}

Question also includes - Your program will have if-else statements embedded inside of if-else statements, but only in an indirect way. The outer if-else statement will include two function calls as its two branches. If you try to create a four way branch, you are probably on wrong track. You should only need to think about two-way branches(even though the entire program does ultimately branch into four cases).

I am beginner to cpp and only covered till functions. This question was in functions chapter from book Problem Solving with C++ . I have written the above code but not sure whether it is good enough or not. What improvements can be made?

Answer 1

Use a data-driven design

The problem with the way you wrote your program is that it doesn't scale. What if you want to add more units, say metric tons, or stones? What if you want to support ALL the SI scaling prefixes, like nano, micro, mega, giga, and so on?

There are two things that will greatly simplify your code. First is to use a data-driven design: instead of creating a function for each type of conversion, write one function that can convert things, and add a data structure that holds the information that this function would need to perform the conversion. The second thing is to not add all possible combinations from one unit to another, but to only add conversions from an arbitrary unit to a base unit, like from any length to meters. If you want to convert from inches to miles, you first convert from inch to meters, then convert from meters to miles.

Here is what the data structure could look like:

#include <unordered_map>
#include <string>

std::unordered_map<std::string, double> units = {
    {"meter", 1.0},
    {"centimeter", 0.01},
    {"inch", 0.0254},
    {"mile", 1609.344},
    ...
    {"kilogram", 1.0}, // kilogram is the base SI unit, not grams
    {"gram", 0.001},
    {"pound", 0.4536},
    ...
};

And then you can write a single function that can convert from any unit to any other unit:

double convert(const std::string &from, const std::string &to, double value) {
    return value * units[from] / units[to];
}

Of course the above function is very simplified, it does not check if the given unit names are actually known, and also incorrectly allows converting from inches to pounds, so a bit more work is necessary to add input validation. Also, instead of having to add "microgram", "milligram", "centigram", and so on, you could have one table for the units without prefixes, and then one table for prefixes, and then write some code to split a given unit name into its prefix and base name.

Avoid unncessarily passing parameters by reference

I see that in some (but not all) of the functions, you pass doubles by reference. For parameters that are only read from, this is not efficient. Simple types like ints and doubles should be passed by value. Only larger types like structs with several member variables should be passed by const reference.

For the parameters that are written to you of course need to pass by reference, although it is also possible to just return them by value. While you can only return one thing at a time, if you want to return two values you can work around that restriction by returning a std::pair for example.

Avoid having function do too many things at once

Your calc...() functions take two units in and write two units out. However, it is a bit confusing: it assumes the input has to be summed somehow and then convert to two units. But if you sum the output it is actually twice that of the input. Even worse, the conversion factors are not very exact, which leads to more problems. To give an example: by using calcLength_ft_inch() to convert 1 meter and 80 centimeters, the output is 5.9048 feet and 70.8576 inch. But if you convert 5.9048 feet and 70.8576 inches back to meters and centimeters with calcLength_m_cm(), you get 3.4578088 meters and 345.785088 centimeters.

Keep things simple, and only convert one thing at a time, and make sure you do that well.

Answer 2

You have a great review from G. Sliepen addressing design aspects. I'll point out a couple of lower-level issues.

using namespace std;

Bringing all the names from std into the global namespace increases chance of unintentional overloads of functions. Instead, learn to appreciate namespaces for the help that they are in partitioning the names used in your program. Limit using namespace to the namespaces specifically intended to be imported wholesale like that - generally the various …::literals namespaces.

---

std::cin >> choice1;

Don't ignore errors when reading input. If the read was unsuccessful, then choice1 may be uninitialised. It's easy to test a stream for errors, either using if (std::cin) after the read or by setting the stream to throw exceptions (std::cin.exceptions(std::ios_base::badbit|std::ios_base::failbit|std::ios_base::eofbit)) before reading.

---

 std::cout << "Enter lenghts in feet and inches\n"

 std::cout << "Enter 1 for converting kilograms and grams from poundes and ounces\n"

     <<" Enter 2 for converting feets and inches from meters and centimeters\n"

Spelling: should be lengths, pounds, feet.

Answer 3

I think you've missed a critical aspect of the problem. If I convert my height from 1 meter, 81 centimetres to feet and inches, I would expect the result to be 5 feet, 11.26 inches.

--> you larger units should be expressed as ints, not doubles and the equivalent amount should be removed from the smaller unit.

1m 81cm -> 1.81m -> 5.93 ft -> 5 ft 11.26 in

Answer 4

There was a lot of nitpicking, but surprisingly too few higher-level advice on code structure in the existing reviews, so I'll focus on that.

---

Never use Mutable Globals.

Variables should be local.

Mutable Globals are the ultimate "effect at a distance" issue, any function may read/write them, so they make following the data-flow impossible.

Note: Constants can be global, that's not a problem.

---

Single Responsibility.

This is perhaps the most confusing piece of advice, as a single piece of responsibility can be fairly wide.

In this particular case: separate I/O from computation.

Your main function should generally be "shell" function, or "management" function, delegating to other functions. In the case of your program, I'd expect 3 stages:

1. Input.
2. Calculation.
3. Output.

This should be reflected by having one function for each. As a benefit, it will force you to reify your design and give you a single calculate function that you can write unit-tests for... and you should definitely write tests.

---

Names Matter

inputData1, inputData2, etc... are shitty names, because they cannot be understood outside of their call-site.

The 1, 2, etc... are useless without context. Instead you should name them appropriately; for example: inputLengthInFeet, inputLengthInInches, ...

This will come to a head the moment you decide to change your choice list (swapping elements because users prefer to have 1 be cm) and suddenly find yourself having to rename all functions: you'll change inputData1 to inputData3 then inputData3 to inputData2 and if you haven't been careful that'll give you 2 inputData2 (since you had 2 inputData3 after renaming inputData1).

It's a hot mess, fix it now.

---

@G. Sliepen already explained that you could handle factors better. I definitely concur, though I'd add a minimum amount of typing to that.

First, I'd create a Dimension enum:

enum class Dimension : std::uint8_t { Length, Mass };

Then I'd create a Unit; we'll mix them up as it simplifies the code, once you learn templates you'll be able to do better:

enum class Unit : std::uint8_t { Foot, Inch, Meter, Centimeter, Kilogram, ... };

Dimension get_dimension(Dimension);

std::string_view to_shorthand(Unit);

double ratio_to_canonical(Unit);

I'll implement the latter (a bit):

double ratio_to_canonical(Unit unit) {
    switch (unit) {
    case Unit::Meter: return 1.0;
    case Unit::Centimeter: return 0.01;
    case ...
    }
    throw std::runtime_error("Unknown unit: " + std::to_string(int(unit)));
}

And create a class linking a quantity to its unit:

class Quantity {
public:
    // Should be stuff here ;)

private:
    double mQuantity = 0.0;
    Unit mUnit = Unit::Meter;
};

From there you can do... a lot of things. For example, you can check dimensional equality of structs, and do so when performing operations.

I'll go with addition:

 class Quantity {
 public:
     // Stuff

     Quantity& operator+=(Quantity const& other) {
         assert(get_dimension(mUnit) == get_dimension(other.mUnit));

         if (mUnit == other.mUnit) {
             mQuantity += other.mQuantity;
             return *this;
         }

         mQuantity += other.mQuantity * ratio_to_canonical(other.mUnit)
                                      / ratio_to_canonical(mUnit);
         return *this;
      }

      // Stuff
 };

Note: it's possible that you may want to always store the quantity in the canonical representation; it's up to you.

With this Quantity class:

• Input stage: should return a std::pair<Quantity, Unit> for the quantity entered by the user, and the Unit to convert to (or maybe a pair of units, depending on specifications).
  • Hint: output to std::ostream& and read input from std::istream& so you can test it.
• Calculation stage:
  • should verify that the request makes sense: can't convert Length to Mass...
  • should return a new Quantity, of appropriate unit (or maybe a pair of quantities, if getting a pair of units).
• Output stage: should format the received Quantity.
  • Hint: output to std::ostream&, so you can test it too.

And thus main:

int main() {
    auto const [quantity, desired_unit] =
        read_user_request(std::cout, std::cin);

    auto const desired_quantity = translate(quantity, desired_unit);

    write_user_response(desired_quantity, std::cout);
}

Good luck!