Simple complex number class

Question

This is my first time writing C++, so I would appreciate advice in the areas of:

• Code style (naming conventions, indentation, etc)
• Memory usage (am I performing unnecessary object copies?)
• Class design (move constructors, destructors, etc, are they necessary?)
• Correct usage of standard library functions (especially the string parsing part)

complex.h:

#ifndef COMPLEX_H
#define COMPLEX_H

#include <string>

class Complex
{
private:
    double real_;
    double imag_;

public:
    Complex();
    Complex(const Complex& obj);
    Complex(const std::string& str);
    Complex(double real);
    Complex(double real, double imag);
    double real() const;
    double imaginary() const;
    double argument() const;
    double modulus() const;
    Complex conjugate() const;
    Complex pow(double power) const;
    std::string toString() const;
    Complex operator+(const Complex& rhs) const;
    Complex operator-(const Complex& rhs) const;
    Complex operator*(const Complex& rhs) const;
    Complex operator/(const Complex& rhs) const;
    bool operator==(const Complex& rhs) const;
    bool operator!=(const Complex& rhs) const;
};

#endif

complex.cpp:

#include <cmath>
#include <sstream>
#include <regex>

#include "complex.h"

Complex::Complex(const Complex& obj) : Complex(obj.real_, obj.imag_) { }

Complex::Complex(const std::string& str) {
    double real = 0.0, imag = 0.0;
    std::regex realRegex("^(-)?\\s*(\\d+(\\.\\d+)?)$");
    std::regex imagRegex("^(-)?\\s*(\\d+(\\.\\d+)?)i$");
    std::regex bothRegex("^(-)?\\s*(\\d+(\\.\\d+)?)\\s*([-+])\\s*(\\d+(\\.\\d+)?)i$");
    std::smatch match;
    if (std::regex_match(str.begin(), str.end(), match, realRegex)) {
        real = std::atof(match[2].str().c_str());
        if (match[1].matched) {
            real = -real;
        }
    } else if (std::regex_match(str.begin(), str.end(), match, imagRegex)) {
        imag = std::atof(match[2].str().c_str());
        if (match[1].matched) {
            imag = -imag;
        }
    } else if (std::regex_match(str.begin(), str.end(), match, bothRegex)) {
        real = std::atof(match[2].str().c_str());
        imag = std::atof(match[5].str().c_str());
        if (match[1].matched) {
            real = -real;
        }
        if (match[4].str() == "-") {
            imag = -imag;
        }
    } else {
        throw std::runtime_error("Invalid number format");
    }
    real_ = real;
    imag_ = imag;
}

Complex::Complex() : Complex(0.0) { }

Complex::Complex(double real) : Complex(real, 0.0) { }

Complex::Complex(double real, double imag) : real_(real), imag_(imag) { }

double Complex::real() const {
    return real_;
}

double Complex::imaginary() const {
    return imag_;
}

double Complex::argument() const {
    return std::atan2(imag_, real_);
}

double Complex::modulus() const {
    return std::sqrt(real_ * real_ + imag_ * imag_);
}

Complex Complex::conjugate() const {
    Complex result(real_, -imag_);
    return result;
}

Complex Complex::pow(double power) const {
    double mod = modulus();
    double arg = argument();
    mod = std::pow(mod, power);
    arg *= power;
    double real = mod * std::cos(arg);
    double imag = mod * std::sin(arg);
    Complex result(real, imag);
    return result;
}

std::string Complex::toString() const {
    std::stringstream fmt;
    if (imag_ == 0) {
        fmt << real_;
    } else if (real_ == 0) {
        fmt << imag_ << "i";
    } else {
        fmt << real_;
        if (imag_ < 0) {
            fmt << " - " << -imag_;
        } else {
            fmt << " + " << imag_;
        }
        fmt << "i";
    }
    return fmt.str();
}

Complex Complex::operator+(const Complex& rhs) const {
    Complex result(real_ + rhs.real_, imag_ + rhs.imag_);
    return result;
}

Complex Complex::operator-(const Complex& rhs) const {
    Complex result(real_ - rhs.real_, imag_ - rhs.imag_);
    return result;
}

Complex Complex::operator*(const Complex& rhs) const {
    double newReal = real_ * rhs.real_ - imag_ * rhs.imag_;
    double newImag = real_ * rhs.imag_ + imag_ * rhs.real_;
    Complex result(newReal, newImag);
    return result;
}

Complex Complex::operator/(const Complex& rhs) const {
    double denom = rhs.real_ * rhs.real_ + rhs.imag_ * rhs.imag_;
    double newReal = (real_ * rhs.real_ + imag_ * rhs.imag_) / denom;
    double newImag = (imag_ * rhs.real_ - real_ * rhs.imag_) / denom;
    Complex result(newReal, newImag);
    return result;
}

bool Complex::operator==(const Complex& rhs) const {
    return real_ == rhs.real_ && imag_ == rhs.imag_;
}

bool Complex::operator!=(const Complex& rhs) const {
    return !(*this == rhs);
}

Answer 1

1. Well, your code-style is quite common, and consistently-applied, so that's a plus.

2. Your names though can be improved:

   • I wouldn't use modulus for the absolute value, even though it seems to be perfectly correct, because there's a far more common and shorter way: Just call it abs.
   • .argument() is normally shortened to .arg(), .imaginary() to .imag(). Those can be debated though.

3. You should provide compound-assignment-operators +=, -=, *= and /=, and implement +, -, * and / in terms of them.

4. Is there a reason you are explicitly defining your copy-constructor? The default one you get by omitting the declaration is fine.

5. You are far too fond of member-functions, and the increased coupling it brings. Read GotW 84: Monoliths "Unstrung".

   Of your members, only real(), imaginary(), and the ones the language forces you to make members should be. (You should add free functions for the first two, or make them friend-functions instead though.)

6. .toString() should be the free function to_string(), like the standard-library one.
   Consider also adding a stream-inserter. Due to the format you chose, it's not possible to write a good stream-extracctor.

7. Construction from a std::string should be marked explicit, as it might fail or loose information.
   All other constructors (and all functions but to_string) should be marked constexpr.
   And most should be marked noexcept.

8. Construction from std::string is complex enough you should add a doc-comment giving all accepted formats.

9. Consider merging your default-constructor, constructor from double, and constructor from real- and double- components into one using default-arguments.
   Also, implementing it in-class is potentially superior.
   Actually, consider in-class implementations for all small functions.

10. Consider providing the square of the absolute value (as norm), to avoid the costly square-root unless needed.

11. As the class only contains two doubles, pass-by-value might actually be more efficient than pass-by-reference. That depends on the specific architecture and ABI though.

(You might benefit from comparing your code with std::complex<double>.)

Answer 2

Constructors

You can combine all your constructors into a single one:

Complex();
Complex(double real);
Complex(double real, double imag);

You can just declare a single one with defaults.

Complex(double real = 0.0, double imag = 0.0);

This will cover the three you have defined above.

The copy constructor is unnecessary because the default compiler generated one will work perfectly well.

Complex(const Complex& obj);

Free functions

When doing arithmetic functions, using free functions is usually an advantage because it allows the compiler to auto convert operands to the correct type (Normally I am against auto conversion but for arithmetic this is the one scenario that it actually pays off).

Example:

Complex   var1(5.6, 6.7);
Complex   var2  = var1 + 5.6;   //  Compiles
Complex   var3  = 5.6 + var1;   //  Fails to compile

You would expect operator+ to work the same way in both cases. But at the moment it will fail to compile.

But if you use free standing functions it allows the compiler to convert one parameter to another type and this will allow the above scenario to compile.

Complex operator+(Complex const& lhs, Complex const& rhs)
{
    return Complex(lhs.real() + rhs.real(), lhs.img() + rhs.img());
}

Mathematical operators

If you define mathematical operators. It can be efficient to define all of them (especially the assignment operators). Each operator X can be defined efficiently in terms of X=

// Example:
Complex& Complex::operator+=(Complex const& rhs)
{
    real_   += rhs.real();
    img_    += rhs.img();
    return *this;
}
Complex operator+(Complex const& lhs, Complex const& rhs)
{
    Complex   result(lhs);
    return result += rhs;
}

Just to show it the other way around. Defining X= in terms of X.

Complex operator+(Complex const& lhs, Complex const& rhs)
{
    return Complex(lhs.real() + rhs.real(), lhs.img() + rhs.img());
}
Complex& Complex::operator+=(Complex const& rhs)
{
    (*this) = (*this) + rhs;  // Does not look as intuitive.
}

Answer 3

I haven't thought this entirely through, but I can see subtle bugs happening in the client code because they've accidentally swapped the real and imaginary parts. Would it be better to create types for these in order to gain some extra type safety? They would both basically just defer to double, but it would ensure that this method isn't accidentally called with the wrong arguments.

Complex::Complex(double real, double imag) : real_(real), imag_(imag) { }

I'm thinking of a signature along the lines of:

Complex::Complex(Real real, Imaginary imag)

It could be YAGNI territory, but I'm a big fan of getting away from primitives as often as it makes sense to.