11
\$\begingroup\$

I've written a little calculator in C++ for complex numbers:

#include <iostream>
using namespace std;

class ComplexNumber {

    public:
        double real;
        double imaginary;

        void add(ComplexNumber a, ComplexNumber b) {

            //Just add real- and imaginary-parts
            double real = a.real + b.real;
            double imaginary = a.imaginary + b.imaginary;
            ComplexNumber c = ComplexNumber(real, imaginary);
            cout << "a + b = " << c.real << " + (" << c.imaginary << ") * i" << endl; 
        }

        void sub(ComplexNumber a, ComplexNumber b) {

            //Just subtract real- and imaginary-parts
            double real = a.real - b.real;
            double imaginary = a.imaginary - b.imaginary;
            ComplexNumber c = ComplexNumber(real, imaginary);
            cout << "a - b = " << c.real << " + (" << c.imaginary << ") * i" << endl; 
        }

        void multiply(ComplexNumber a, ComplexNumber b) {

            //Use binomial theorem to find formula to multiply complex numbers
            double real = a.real * b.real - a.imaginary * b.imaginary;
            double imaginary = a.imaginary * b.real + a.real * b.imaginary;
            ComplexNumber c = ComplexNumber(real, imaginary);
            cout << "a * b = " << c.real << " + (" << c.imaginary << ") * i" << endl; 
        }


        void divide(ComplexNumber a, ComplexNumber b) {

            //Again binomial theorem
            double real = (a.real * b.real + a.imaginary * b.imaginary) / (b.real * b.real + b.imaginary * b.imaginary);
            double imaginary = (a.imaginary * b.real - a.real * b.imaginary) / (b.real * b.real + b.imaginary * b.imaginary);
            ComplexNumber c = ComplexNumber(real, imaginary);
            cout << "a : b = " << c.real << " + (" << c.imaginary << ") * i" << endl; 
        }

    /*
     * Constructor to create complex numbers
     */
    ComplexNumber(double real, double imaginary) {
        this->real = real;
        this->imaginary = imaginary;
    }
};

int main() {

    /*
     * Variables for the real- and imaginary-parts of
     * two complex numbers
     */
    double realA;
    double imaginaryA;
    double realB;
    double imaginaryB;

    /*
     * User input
     */
    cout << "enter real(A), imag(A), real(B) and imag(B) >> ";
    cin >> realA >> imaginaryA >> realB >> imaginaryB;
    cout << endl;

    /*
     * Creation of two objects of the type "ComplexNumber"
     */
    ComplexNumber a = ComplexNumber(realA, imaginaryA);
    ComplexNumber b = ComplexNumber(realB, imaginaryB);

    /*
     * Calling the functions to add, subtract, multiply and 
     * divide the two complex numbers.
     */
    a.add(a, b);
    a.sub(a, b);
    a.multiply(a, b);
    a.divide(a, b);
    return 0;
}

I would appreciate any suggestions to improve the code.


You can find my follow-up question here.

\$\endgroup\$

3 Answers 3

24
\$\begingroup\$
  • In short programs it can be OK, but in general avoid writing using namespace std. You'll find plenty of material here and elsewhere on why this is so.

  • To promote proper encapsulation of data, both real and imaginary should be declared under private, i.e., not be visible to the outside.

  • All of the four member functions that perform arithmetic take on too much responsibility and as a result, are very inconvenient for the user. That is, remember: one function, one responsibility. If you add, then you don't also print. For example, as a user, I just want to use your class for complex arithmetic - I don't want to print every time I do so!

  • Your four member functions don't modify the state of the object. This makes the whole class and its functionality quite rigid and strange. As it is, the functionality appears as it should be a collection of four free functions not inside any class (indeed, perhaps your background is in Java where I can imagine this is more common). A more intuitive interface for let's say the addition would be void add(const ComplexNumber& other) { ... }, where the implementation actually adds to real and imaginary of *this. Same for the other three operations.

  • If you wanted to get fancy, you could use operator overloading to allow for a natural way to express complex arithmetic for the user.

  • It would be useful to add a void print() const { ... } method in case the user wants to print.

  • Use an initializer list if you need to write explicit constructors, i.e., write ComplexNumber(double r, double i) : real(r), imaginary(i) { } instead. If you don't, the compiler will call default constructors on the members first which in your case is unnecessary.

  • In modern C++, we have the option of using in-class constructors for default values. This is quite handy, i.e., you could have double real {0.0}; (similarly for imaginary) if you wanted to support the creation of complex numbers without an explicit constructor call.

  • By the way, you don't have to write ComplexNumber a = ComplexNumber(realA, imaginaryA); when it's much cleaner to write ComplexNumber a(realA, imaginaryA);.

  • Perhaps you know this, but std::complex<T> does exist if you wanted to do complex arithmetic in a more serious setting.

\$\endgroup\$
5
  • \$\begingroup\$ I don't really understand the fourth point. What do you exactly mean? \$\endgroup\$
    – user214772
    Feb 2, 2020 at 20:33
  • 5
    \$\begingroup\$ @chrysaetos99 it should either be add(a, b) returning c and not modifying a or b, or a.add(b), in which a gets modified, but not a.add(a,b) which is distinctly awkward (after all, a is this, why is it being passed in as a parameter again?). \$\endgroup\$
    – muru
    Feb 3, 2020 at 6:28
  • 9
    \$\begingroup\$ I disagree with the first point. using namespace std; should be avoided in all programs. \$\endgroup\$ Feb 3, 2020 at 17:39
  • 4
    \$\begingroup\$ arguably, there's no need to declare these two fields as private because all possible states are valid for this class. \$\endgroup\$
    – Sopel
    Feb 3, 2020 at 17:54
  • 1
    \$\begingroup\$ Operator overloading for arithmetic classes is not fancy and should be the standard way to implement the functionality. \$\endgroup\$
    – Clearer
    Feb 4, 2020 at 7:20
8
\$\begingroup\$

First, note that the complex class is unnecessary because we have std::complex in the standard library, which is provided in the header <complex>. If you want to design your own class, std::complex is a good reference. Now, for two complex numbers x and y, we can use x + y, x - y, x * y, and x / y directly.

Next, notice that this pattern comes up a few times, with slight modifications:

cout << "a + b = " << c.real << " + (" << c.imaginary << ") * i" << endl;

The outputting of the complex number can be extracted into a function to reduce repetition: (std::string requires <string> and std::ostringstream requires <sstream>)

std::string format(std::complex<double> z)
{
    std::ostringstream oss{};
    oss << z.real() << " + (" << z.imag() << ") * i";
    return oss.str();
}

Similarly, we can use a separate function to read the real and imaginary parts of a complex number:

std::complex<double> read_complex()
{
    double real, imag;
    std::cin >> real >> imag;
    return {real, imag};
}

By the way, don't use std::endl unless you need the flushing semantics (which usually slows down the program). Simply use '\n' instead.

Putting everything together:

#include <complex>
#include <iostream>
#include <sstream>
#include <string>

std::string format(std::complex<double> z)
{
    std::ostringstream oss{};
    oss << z.real() << " + (" << z.imag() << ") * i";
    return oss.str();
}

std::complex<double> read_complex()
{
    double real, imag;
    std::cin >> real >> imag;
    return {real, imag};
}

int main()
{
    auto x = read_complex();
    auto y = read_complex();

    std::cout << "x + y = " << format(x + y) << '\n';
    std::cout << "x - y = " << format(x - y) << '\n';
    std::cout << "x * y = " << format(x * y) << '\n';
    std::cout << "x / y = " << format(x / y) << '\n';
}

(live demo)

\$\endgroup\$
9
  • 1
    \$\begingroup\$ Thanks for your answer, but I don't really want to use std::complex, because I wanted to learn to work with complex numbers by myself. \$\endgroup\$
    – user214772
    Feb 3, 2020 at 9:04
  • \$\begingroup\$ @chrysaetos99 Remember to add essential info like this in your future questions. Anyway, my point is that your complex class should mimic the (more natural) interface of std::complex as explained in other answers. \$\endgroup\$
    – L. F.
    Feb 3, 2020 at 9:08
  • \$\begingroup\$ note that the complex class is unnecessary because we have std::complex I strongly disagree here. While the STL provides many useful constructs, there are often strong arguments why implementing your own version is beneficial. \$\endgroup\$ Feb 3, 2020 at 17:54
  • 1
    \$\begingroup\$ Can you cite more than one, @infinitezero? The only reason I can think of to reinvent STL functionality is for educational purposes. \$\endgroup\$ Feb 3, 2020 at 18:44
  • 1
    \$\begingroup\$ @Clearer I thought about that a bit, but since complex is a standard type that already has a operator<< overload that can normally be picked up by ADL, I regarded adding my own overload in the global namespace to hide the standard one as obfuscating the code semantics a little bit (I would expect std::cout << std::complex(4, 2) to print (4,2) instead of some user-defined variant). So I provided a separate function instead \$\endgroup\$
    – L. F.
    Feb 4, 2020 at 7:53
7
\$\begingroup\$

Building off the the already excellent points made by Juho,

Redundant arguments

Within your member function, you never make reference to the object being called on. Take for instance your ComplexNumber::add function. A more sound object-oriented implementation might resemble

        void add(ComplexNumber other) {

            //Just add real- and imaginary-parts
            double real = this->real + other.real;
            double imaginary = this->imaginary + other.imaginary;
            ComplexNumber c = ComplexNumber(real, imaginary);
            cout << "a + b = " << c.real << " + (" << c.imaginary << ") * i" << endl; 
        }

No Returns

The addition, subtraction, multiplication, and division operation you implemented aren't terribly useful to the user since they have no way to access the result. Consider updating all of these member function to return a new complex number, which might look like:

        ComplexNumber add(ComplexNumber other) {

            //Just add real- and imaginary-parts
            double real = this->real + other.real;
            double imaginary = this->imaginary + other.imaginary;
            ComplexNumber c = ComplexNumber(real, imaginary);
            cout << "a + b = " << c.real << " + (" << c.imaginary << ") * i" << endl; 
            return c;
        }

Now the can perform operations such as ComplexNumber sum = a.add(b).

Printing within functions

Write to stdout from inside of a function is usually considered bad practice. If, for instance, I wanted to use your complex number library to write my own CLI application, I would have not way prevent every complex number addition from being printed out. This is rather undesirable. I would recommend moving all of your statements with cout to your main function, leaving your member functions to resemble

        ComplexNumber add(ComplexNumber other) {

            //Just add real- and imaginary-parts
            double real = this->real + other.real;
            double imaginary = this->imaginary + other.imaginary;
            ComplexNumber c = ComplexNumber(real, imaginary);
            return c;
        }

Operator Overloading

This is a more advanced C++ concept, but it is good to be aware of. Instead of writing

ComplexNumber sum = a.add(b);

you can instead have the interface

ComplexNumber sum = a + b;

by overloading the addition operator for your class. A tutorial describing how to accomplish this can be found here. A possible implementation might look like

        ComplexNumber operator+(ComplexNumber other) {

            //Just add real- and imaginary-parts
            double real = this->real + other.real;
            double imaginary = this->imaginary + other.imaginary;
            ComplexNumber c = ComplexNumber(real, imaginary);
            return c;
        }
```
\$\endgroup\$
2
  • 4
    \$\begingroup\$ Note that if you don't modify the instance you really shouldn't call the method add, since that implies modification to most people. That mistake has been made multiple times (hello Java BigInteger) and it always confuses people. So a better name for the second method might be something like plus (or just go with the operator overloading and avoid the confusion alltogether) \$\endgroup\$
    – Voo
    Feb 3, 2020 at 8:49
  • \$\begingroup\$ Operators are nothing more than functions with a funky name. It's no more advanced than regular functions. \$\endgroup\$
    – Clearer
    Feb 4, 2020 at 7:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.