Krdytkyu

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

• 
  

  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.

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.

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_;

}