10
\$\begingroup\$

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);
}
\$\endgroup\$
10
\$\begingroup\$
  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>.)

\$\endgroup\$
  • \$\begingroup\$ I'm not too sure what you mean by using free functions - do you mean turning obj.method(arg) to method(obj, arg)? Also, I was under the impression that the modulus of a complex number is just the magnitude.. \$\endgroup\$ – Andrew Sun Nov 15 '15 at 1:42
  • 1
    \$\begingroup\$ @AndrewSun, what Deduplicator means is that it is usually better to define a non-member function instead if this function can be implemented in terms of the existing public interface. Here's another very good piece on this subject: drdobbs.com/cpp/how-non-member-functions-improve-encapsu/… \$\endgroup\$ – glampert Nov 15 '15 at 2:09
  • \$\begingroup\$ I see, so real() and imaginary() should be kept in the class interface, and the other methods like modulus() and argument() should become modulus(Complex) and argument(Complex)? \$\endgroup\$ – Andrew Sun Nov 15 '15 at 2:27
  • \$\begingroup\$ "Modulus" is a perfectly cromulent synonym for the magnitude of a complex number. \$\endgroup\$ – 200_success Nov 15 '15 at 9:17
  • \$\begingroup\$ @200_success: Well, seems so, thanks for the correction. Staying with abs, for absolute value, is better as it's far more common though. \$\endgroup\$ – Deduplicator Nov 15 '15 at 9:33
5
\$\begingroup\$

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.
}
\$\endgroup\$
  • \$\begingroup\$ You might want to add that if there's a desire to acknowledge that the copy constructor is explicitly intended to be the default, one can use Complex(const &Complex) = default; \$\endgroup\$ – Edward Nov 15 '15 at 18:07
  • \$\begingroup\$ Good point about the free functions, I never considered that! Quick question, why is + defined in terms of +=, and not the other way around? Is it to avoid an unnecessary Complex object allocation when using +=? \$\endgroup\$ – Andrew Sun Nov 15 '15 at 18:24
  • \$\begingroup\$ @AndrewSun: It's done that way because it avoids an extra-object, though that probably doesn't actually signify here as the type is so small, there is no possibility of type-conversion on the left side of assingment anyway, and all assignment-operators are forced to be class-members (thus tightly coupled), whatever you want. \$\endgroup\$ – Deduplicator Nov 15 '15 at 22:31
0
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ So the client would have to use Complex(Real(1.3), Imaginary(1.6)) instead of Complex(1.3, 1.6)? Wouldn't this be equally dangerous as the current method, for example by using Complex(Real(i), Imaginary(j)) instead of Complex(Real(j), Imaginary(i))? \$\endgroup\$ – Andrew Sun Nov 14 '15 at 22:22
  • \$\begingroup\$ That's a fair point and it wouldn't help in that case, but ideally i and j would already be their respective types and it would rarely need to make the conversion from double to real or imaginary. I'd recommend giving Steve McConnel's Code Complete a read for an in depth look at the topic. \$\endgroup\$ – RubberDuck Nov 14 '15 at 22:25
  • \$\begingroup\$ Hmm, I see. But in the case of complex numbers, I think it doesn't really make much sense for the real and imaginary components to be ever stored separately, except for the short time between parsing input and constructing the Complex object. Anyways, that book looks interesting, I'll be sure to take a look. \$\endgroup\$ – Andrew Sun Nov 14 '15 at 22:40
  • 2
    \$\begingroup\$ Complex(1.0, 1.5); What's this? 1+1.5i or i+1.5? :) Readability... \$\endgroup\$ – vp_arth Nov 14 '15 at 23:09
  • 2
    \$\begingroup\$ pf. Putting the imaginary component before the real one is very odd.So much that I would argue removing that constructor, or insisting on packaging the arguments (or at least the imaginary one) in a dedicated real/imaginary wrappers would reduce usability. \$\endgroup\$ – Deduplicator Nov 15 '15 at 0:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.