Cartesian/polar coordinates program

Question

Can I get my program checked for efficiency? As in, see if there are better ways of writing my code, for example, making more efficient use of memory and/or security options like where to use constants?

main.cpp

// MAIN.CPP
//
#include "Force.h"
#include "require.h"
#include <iostream>
#include <fstream>
#include <vector>

using namespace std;

int main()
{   
    //Read in values from "ForceList.txt"
    char ctpe;
    double real1, real2;
    vector<Force> forceCol;
    ifstream in("ForceList.txt");
    assure(in, "Forcelist.txt");    //verify its open

    //Store forces in vector 'forceCol' in cartesian as default
    while (in >> ctpe >> real1 >> real2) {
        Force cart(ctpe, real1, real2);
        if (ctpe == 'c' || ctpe == 'C') {
            forceCol.push_back(cart);
        }
        else if (ctpe == 'p' || ctpe == 'P') {
            cart.converter();
            forceCol.push_back(cart);
        }
    }

    in.close();

    // Print the forces in vector 'forceCol' in cartesian form.
    cout << "Cartesian:\n" << endl;
    cout << "No." << "              x." << "          y. \n";
    for (unsigned int i = 0; i < forceCol.size(); ++i) {
        cout << "Force " << i + 1 << ":";
        forceCol[i].print();
    }

    //Convert 'forceCol' into polar form.
    for (unsigned int i = 0; i < forceCol.size(); ++i) {
        forceCol[i].converter();
    }


    // Print the forces in vector 'forceCol' in polar form.
    cout << endl << "---------------" << endl;
    cout << "\nForces in polar:\n" << endl;
    cout << "No." << "            Radius." << "     Angle. \n";
    for (unsigned int i = 0; i < forceCol.size(); ++i) {
        cout << "Force " << i + 1 << ":";
        forceCol[i].print();
    }

    //Convert back to cartesian for calculations
    for (unsigned int i = 0; i < forceCol.size(); ++i) {
        forceCol[i].converter();
    }

    //Summate the forces in 'forceCol' and print the result in cartesian form.
    Force summedForce;
    for (unsigned int i = 0; i < forceCol.size(); ++i) {
        summedForce = summedForce + forceCol[i];
    }
    cout << endl << "---------------" << endl;
    cout << "\nThe summed force in cartesian form is: " << endl;
    summedForce.print();

    //Convert 'summedForce' to polar form and print.
    summedForce.converter();
    cout << "\nThe summed force in polar form is: " << endl;
    summedForce.print();

    //Print out the resultant force of the first 3 forces in cartesian form
    Force resultantForce;
    resultantForce = -(forceCol[0] - forceCol[1] + forceCol[2]);
    cout << "\nThe resultant force of the first 3 forces in cartesian form is: " << endl;
    resultantForce.print(); 

    //Convert 'resultantForce' to polar form and print.
    resultantForce.converter();
    cout << "\nThe resultant force of the first 3 forces in polar form is: " << endl;
    resultantForce.print();
    cout << endl;

}

Force.h

#ifndef Force_H
#define Force_H

#include <iostream>
#include <iomanip>
#include "require.h"

//Question 1 - Class 'Force' that reads in cartesian or polar coordinates,
//can discern which is which, and allows conversion and arthithmetical manipulation

class Force {
    char coType; // Type of coordinate - cartesian or polar
    double r1;  // 1st real number
    double r2;  // 2nd real number
public:
    //Constructor that reads in the files and checks that the first value is 'c, C, p or P'
    //and assign defeauts
    Force(char coType1 = 'c', double re1 = 0.0, double re2 = 0.0) :
        coType(coType1), r1(re1), r2(re2) {
        require(coType1 == 'p' || coType == 'P' || coType == 'c' || coType == 'C');
    };
        ~Force() {};

    //Binary operators for addition and subtraction of 'Force' objects values.
    Force operator+(Force& fo){
        return Force(coType = fo.coType, r1 + fo.r1, r2 + fo.r2);
    };
    Force operator-(Force& fo){
        return Force(coType = fo.coType, r1 - fo.r1, r2 - fo.r2);
    };
    //Unary operator to change the sign of the objects values.
    Force operator-()
    {
        return Force(coType, r1 = -r1, r2 = -r2);
    }

    //Accessors
    char get_coType() const { return coType; }
    double get_r1() const { return r1; }
    double get_r2() const { return r2; }

    //mutators 
    void set_coType(char c) { coType = c; }
    void set_r1(double a) { r1 = a; }
    void set_r2(double b) { r2 = b; }

    //printer
    int print() {
        using namespace std;
        cout << setprecision(2) << fixed;
        cout << coType << setw(10) << right << r1 << setw(10) << right << r2 << endl;
        return 0;
    }

    //Converter
    void converter();
};


#endif

Force.cpp

#include "Force.h"
#include <cmath>

using namespace std;

//Converter function
void Force::converter() {

    //Pass by reference to change original values 
    double& x = r1;
    double& y = r2;
    double& rad = r1;
    double& theta = r2;

    //'Intermeditary' variables
    double x1 = x;
    double y1 = y;
    double rad1 = rad;
    double theta1 = theta;

    //Check which type of coordinates are being converted 
    //intermeditary variables used to stop math errors in the conversion
    if (coType == 'c' || coType == 'C') {
        coType = 'p';
        rad = hypot(x1, y1);
        theta = atan2(y1, x1);
    }
    else if (coType == 'p' || coType == 'P') {
        coType = 'c';
        y = rad1 * sin(theta1);
        x = rad1 * cos(theta1);

    }


};

ForceList.txt

p 10 0.5
c 12 14
p 25 1
p 100. 0.80
c 50. 50.
p 20 3.14
c -100. 25
p 12 1.14

The first letter is what type of force cartesian or polar, the following 2 numbers are the coordinates for cartesian and the magnitude and angle for polar

Answer 1

Here are some comments that may help you improve your code.

Understand the difference between <cmath> and <math.h>

The difference between the two forms is that the former defines things within the std:: namespace versus into the global namespace. Language lawyers have lots of fun with this, but for daily use I'd recommend using <cmath> and then to use functions defined there, explicitly use the namespace. That is, write std::atan2 instead of plain atan2. See this SO question for details.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. If you use it at all, use it only within functions.

Reconsider the class design

The class requires that the user keep track of which form the data is in and to use the converter() function to change from polar to cartesian and back. That's not a good class design. Better would be to keep the internal representation in whatever format is convenient to you and and only apply conversion on input or output. Also, C++ is not Java. Don't make "accessors" and "mutators" for every member item. If that's what you really need (and usually it isn't), just use a plain struct instead of a class. If that's not clear yet, the following points should make it clear.

Use the std::complex class

Much of what you're implementing already exists within the std::complex class template. I'd recommend using that until and unless you need to go to three or more dimensions. Things will be much simpler if you declare the Force class like this:

class Force : public std::complex<double> { /*...*/ }

Use const where practical

I would not expect the print routine to alter the underlying Force on which it operate, and indeed it does not. You should make this expectation explicit by using the const keyword:

int print() const;

This declares that the print will not modify the Force, making it clear to both the compiler and to the human reader of your code.

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Use string concatenation

The main function includes these lines:

cout << endl << "---------------" << endl;
cout << "\nForces in polar:\n" << endl;
cout << "No." << "            Radius." << "     Angle. \n";

That's multiple calls to operator<< where only one is really required. I'd write it like this instead:

std::cout << "\n---------------\n" 
            "\nForces in polar:\n\n"
            "No." 
            "            Radius." 
            "     Angle. \n";

This reduces the entire thing to a single call to operator<< because consecutive strings in C++ (and in C, for that matter) are automatically concatenated into a single string by the compiler.

Have you run a spell check on comments?

If you run a spell check on your comments, you'll find a number of things such as "arthithmetical" instead of "arithmetical" and "intermeditary" instead of "intermediary". Since your code is fairly well commented, it's worth the extra step to eliminate spelling errors.

Don't hardcode file names

The input file might be something that a user of this program wants to place elsewhere. It would be nice to allow the user to specify the input file name as a command line argument instead of hardcoding it.

Prefer a stream extractor to manual input

Right now, the input routine is in main. If this is a format you're expecting to use again, I'd recommend instead to implement a stream extractor like this:

friend std::istream &operator>>(std::istream &in, Force &f);

Then within main, you could use it like this:

std::vector<Force> forceCol;
{
    std::ifstream in(argv[1]);
    Force f;
    while (in >> f) {
        forceCol.push_back(f);
    }
}

Note that most of the lines in this snippet are enclosed within braces. This is deliberate so that in and f will go out of scope and automatically be destroyed when the code reaches the closing brace. This means there's no need for an explicit file close.

Use standard algorithms

Instead of writing the loop like this:

Force summedForce;
for (unsigned int i = 0; i < forceCol.size(); ++i) {
    summedForce = summedForce + forceCol[i];
}

You could instead write it like this:

Force summedForce{std::accumulate(forceCol.begin(), 
                                  forceCol.end(), 
                                  Force{})
};

Write two different print routines

As mentioned earlier, rather than change the internal structure of the Force, simply create two different print routines like this:

std::string asCart() const;
std::string asPolar() const;

In this case, I've elected to create and return a std::string, but one could just as easily pass a std::ostream & in (and out). Now the user doesn't need to care about the internal representation, but just asks explicitly for the desired form.

Answer 2

In your class, settle on a set of units, and use that for your internal storage.

Have specific methods for inputs e.g. SetPolar, SetCartesian. You can use your three-pronged approach for the constructor, which can then call the two input methods depending on whether c or p is passed (reducing duplication in the code).

Your specific input methods do conversions from the user numbers to your chosen units. (e.g. if you chose polar, then not much conversion is required with SetPolar).

You should also have specific properties e.g. GetPolar, GetCartesian that convert from your chosen units to what the user wants.

The above approach (better use of Class concepts) reduces your need for checking the coType - operations and calculations using the class will seem more natural. After all, presentation (polar, Cartesian) is only a human construct on top of something that happens regardless of what we use.

Answer 3

The include guard is too easily clashed with someone else's file when this is used. Use #pragma once; or if a guard is really needed, use a UUID.

---

The coType is best an enum, not a char.

---

Don’t write an empty destructor. Leave it off completely, or use =default to emphasize that it exists. You’ve made your class to not be trivally destructable because of this, which means the compiler can’t do certain optimizations (that's an over simplification).

---

Learn the canonocal way to write arithmetic operators. That is, make += a member and then write + in terms of it; etc. You don’t even have the parameter right: why can’t someone add a const value?

void foo (const Force& a, const Force& b)
{
    Force c = a + b;  // fails to compile!

Your operators are assuming that the arguments are in Cartesian representation. It will get the wrong results if in polar.

---

Will something like

Force baz (const Force& in)
{
    Force res;
    res = baz;
    return res;
}

compile? Why not? Go over your checklist of functions you need in a class.

---

set_coType is silly. It is meaningless to change that without changing the existing values. Now if you had a function to change the internal representation to the chosen form, that would be better.

Having set_r1, set_r2 is not a normal thing for a simple numeric class like this. You would normally just form a new complex number, not change one of the parts of an existing value.

E.g. to get the complex conjugate of z, write Force{'C', z.r1, -z.r2};
but now you see another issue; there is no way to ensure that I’m in C format to begin with nor a way to transform it into C mode.

If we had accessors like real, imag, abs, arg then those would return the desired value whether stored or computed. E.g. if I had a polar value, real would calculate it from the polar information it has; if it was a Cartesian value, it just returns the first stored number.

I suggest modeling your complex numbers on the standard class, linked in the previous paragraph.

---

Do not write using namespace std;

---

Your converter function that switches from one to the other representation is overly complicated, and not very useful when you can’t tell which form you start with. E.g.

void bar (const Force& f1)
{
// I want that in polar mode.
Force f2= f1;
if (/* what?? no accessor defined to check for that */)
   f2.convert();
 ⋮

Instead, have a way to convert to polar regardless of what I start with: if already in polar, it’s easy! Internally, have two separate functions with different variables; your x, y, ⋯ x, rad1 is just too muddled to deal with.

Your comment “intermeditary variables used to stop math errors in the conversion” makes me think you just added them to fix a problem. Declaring all the variables you might need at the top of a function is bad. If you declared where needed, you see you only need half of those in each branch.

And the function to do the converting should be (drum roll…) the constructor!

Force f2 {f1,'P'};

an optional second argument to the copy constructor will convert if necessary.

---

print should be a non-member function that is not a core part of the class. And what’s with the int return that’s always zero?

If you needed to make it a member to print coType, you should instead take that as a sign that the public API is lacking the needed features to use the class effectively (e.g. return what is the current mode).

And why not allow printing of const values?