6
\$\begingroup\$

As part of my C++ training, I wanted to create a basic complex number class for basic complex number calculations.
The class should have the following:

  • constructor (non explicit, for implicit conversions)
  • << and >> operators to stream with cout and cin
  • ==, != operators to compare complex numbers
  • +, +=, -, -=, *, *=, /, /= operators for simple arithmetics
  • GetR(), GetI(), SetR(), SetI() functions to access the real and imaginary parts of the number

I am going to work in a place that uses an underscore at the end of every function parameter, so I am sorry in advance for any discomfort it may cause while reviewing my code.

This is really a part of my baby steps in C++, so any feedback about style, design and coding will be appreciated. Attached are both the header file and the cpp file:


//  complex.h
//  Written by me on Jan 28, 2015

namespace exercises
{

class Complex;

std::ostream& operator<<(std::ostream&, const Complex&);
std::istream& operator>>(std::istream&, Complex&); // assumes (a,bi) format

bool operator==(const Complex, const Complex);
bool operator!=(const Complex, const Complex);

Complex operator+(const Complex, const Complex);
Complex operator-(const Complex, const Complex);
Complex operator*(const Complex, const Complex);
Complex operator/(const Complex, const Complex);

class Complex
{
public:
    Complex(const double r_=0, const double i_=0);// non explicit on purpse

    // using generated ~tor, cctor, c=tor

    Complex& operator+=(const Complex);
    Complex& operator-=(const Complex);
    Complex& operator*=(const Complex);
    Complex& operator/=(const Complex);

    double GetR()const;
    double GetI()const;

    void SetR(const double);
    void SetI(const double);    

private:
    double m_r;
    double m_i;
};

} // namespace exercises

//  complex.h
//  Written by me on Jan 28, 2015

#include <iostream>
#include <cassert>
#include "complex.h"

namespace exercises
{

// ---------- global funcs ----------

bool operator==(const Complex a_, const Complex b_)
{
    return (a_.GetR() == b_.GetR() && a_.GetI() == b_.GetI());
}

bool operator!=(const Complex a_, const Complex b_)
{
    return (a_.GetR() != b_.GetR() && a_.GetI() != b_.GetI());
}

Complex operator+(const Complex a_, const Complex b_)
{
    Complex ret(a_.GetR()+b_.GetR(), a_.GetI()+b_.GetI());
    return ret;
}

Complex operator-(const Complex a_, const Complex b_)
{
    Complex ret(a_.GetR()-b_.GetR(), a_.GetI()-b_.GetI());
    return ret;
}

Complex operator*(const Complex a_, const Complex b_)
{
    double a=a_.GetR(), b=a_.GetI();
    double c=b_.GetR(), d=b_.GetI();

    Complex ret(a*c-b*d, b*c+a*d);

    return ret;
}

Complex operator/(const Complex a_, const Complex b_)
{
    double a=a_.GetR(), b=a_.GetI();
    double c=b_.GetR(), d=b_.GetI();
    assert(c || d);

    Complex ret((a*c+b*d)/(c*c+d*d), (b*c-a*d)/(c*c+d*d));

    return ret;
}

std::ostream& operator<<(std::ostream& os_, const Complex& comp_)
{
    return os_ << '(' << comp_.GetR() << ',' << comp_.GetI() << "i)";
}

std::istream& operator>>(std::istream& is_, Complex& comp_) // assumes (a,bi) format
{
    char ch = 0;
    double r = 0;
    double i = 0;

    is_ >> ch >> r >> ch >> i >> ch >> ch;

    comp_.SetR(r);
    comp_.SetI(i);

    return is_;
}

// ---------- interface funcs ----------

Complex::Complex(const double r_, const double i_): m_r(r_), m_i(i_)
{}

Complex& Complex::operator+=(const Complex o_)
{
    m_r += o_.m_r;
    m_i += o_.m_i;

    return *this;
}

Complex& Complex::operator-=(const Complex o_)
{
    m_r -= o_.m_r;
    m_i -= o_.m_i;

    return *this;
}

Complex& Complex::operator*=(const Complex o_)
{
    double a=m_r, b=m_i, c=o_.m_r, d=o_.m_i;
    m_r = a*c - b*d;
    m_i = b*c + a*d;

    return *this;
}

Complex& Complex::operator/=(const Complex o_)
{
    assert(o_.m_r || o_.m_i);
    double a=m_r, b=o_.m_r, c=m_i, d=o_.m_i;
    m_r = (a*c+b*d)/(c*c+d*d);
    m_i = (b*c-a*d)/(c*c+d*d);

    return *this;
}

double Complex::GetR()const
{
    return m_r;
}

double Complex::GetI()const
{
    return m_i;
}

void Complex::SetR(const double r_)
{
    m_r = r_;
}

void Complex::SetI(const double i_)
{
    m_i = i_;
}

} //namespace exercises
\$\endgroup\$
  • \$\begingroup\$ Note: There is already a std::complex \$\endgroup\$ – Martin York Feb 1 '15 at 18:01
8
\$\begingroup\$
  • To avoid bugs like one mentioned by @janos, a common recommendation is to express operator!= in terms of operator==:

    bool operator!=(const Complex a_, const Complex b_) {
        return !(a_ == b_);
    }
    
  • The same recommendation goes for many other operators, for example operator+ can and should be expressed in terms of operator+=:

    Complex operator+(Complex a_, const Complex b_) {
        return a_ += b_;
    }
    

    This enforces an important invariant (a = b + c must have the same effect as a = b; a += c), and removes the need to call getters and setters.

  • Speaking of getters and setters, in your case they serve no purpose other than hiding names. The access to private members is still unrestricted.

  • As a mathematician, I would expect Complex to have at least more methods, namely norm and conjugate.

  • const qualification of pass-by-value parameters is pointless.

\$\endgroup\$
6
\$\begingroup\$

Bug

The != operator returns true if both the real and imaginary parts are different. That's clearly a bug.

Instead of this:

bool operator!=(const Complex a_, const Complex b_)
{
    return (a_.GetR() != b_.GetR() && a_.GetI() != b_.GetI());
}

You probably meant this:

bool operator!=(const Complex a_, const Complex b_)
{
    return (a_.GetR() != b_.GetR() || a_.GetI() != b_.GetI());
}

Coding style, readibility

A common writing style is to put spaces around operators. For example instead of this:

double a=a_.GetR(), b=a_.GetI();
double c=b_.GetR(), d=b_.GetI();

Complex ret(a*c-b*d, b*c+a*d);

Complex ret(a_.GetR()+b_.GetR(), a_.GetI()+b_.GetI());

Write this way:

double a = a_.GetR(), b = a_.GetI();
double c = b_.GetR(), d = b_.GetI();

Complex ret(a * c - b * d, b * c + a * d);

Complex ret(a_.GetR() + b_.GetR(), a_.GetI() + b_.GetI());

The difference may be subtle, but it makes the code easier to read.

\$\endgroup\$
2
\$\begingroup\$

Code Review

I see little point in using getters/setters.
The interface for complex numbers is pretty well defined and is not likely to change (as it is already a couple of hundred years old).

I would make all the external functions here friends of the complex class. Even if you get rid of the getters/setters. This documents the fact that these functions are tightly coupled to the implementation of the class.

You are passing all you parameters by value. Personally I would pass them be reference (with the exception of the times when you want to make a copy). It will probably save you a register.

I hate the look of your identifiers with the trailing underscore.

operator<<

I probably would not have put braces around the expression. If I remember my maths books complex numbers looked like this:

5+6i

Personally That's the format I would have used. Also if there is no real part I would not have expect the initial 5 I would also expect it to read normal number (without the imaginary part).

I would expect all the following to work:

5
6i
5+6i

operator>>

There is no validation on the input operator. That's a bit clumsy. I would make sure that the stream is in a good state andd that each of the characters is what is expected and set the error bit if there was a mistake.

std::istream& operator>>(std::istream& is_, Complex& comp_) // assumes (a,bi) format
{
    char   open = 0;
    double r    = 0;
    char   coma = 0;
    double i    = 0;
    char   theI = 0;
    char   close= 0;

    is_ >> open >> r >> comma >> i >> theI >> close;

    if (is && open == '(' && comma == ',' && theI == 'i' && close == ')')
    {
        comp_.SetR(r);
        comp_.SetI(i);
    }
    else
    {
        is_.setstate(std::ios::failbit);
    }

    return is_;
}
\$\endgroup\$

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.