5
\$\begingroup\$

After following the suggestions from the first question on that topic (link), I'd like to show you the result now:

#include <iostream>

class ComplexNumber {

    private:
        double real;
        double imaginary;

    public:
        ComplexNumber operator+(ComplexNumber b) {

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

        ComplexNumber operator-(ComplexNumber b) {

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

        ComplexNumber operator*(ComplexNumber b) {

            //Use binomial theorem to find formula to multiply complex numbers
            double real = this->real * b.real - this->imaginary * b.imaginary;
            double imaginary = this->imaginary * b.real + this->real * b.imaginary;
            ComplexNumber c = ComplexNumber(real, imaginary);
            return c;
        }


        ComplexNumber operator/(ComplexNumber b) {

            //Again binomial theorem
            double real = (this->real * b.real + this->imaginary * b.imaginary) / (b.real * b.real + b.imaginary * b.imaginary);
            double imaginary = (this->imaginary * b.real - this->real * b.imaginary) / (b.real * b.real + b.imaginary * b.imaginary);
            ComplexNumber c = ComplexNumber(real, imaginary);
            return c;
        }

        void printNumber(char mathOperator) {
            std::cout << "a " << mathOperator << " b = " << this->real << " + (" << this->imaginary << ") * i" << std::endl; 
        }

    /*
     * Constructor to create complex numbers
     */
    ComplexNumber(double real = 0.0, double imaginary = 0.0) {
        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
     */
    std::cout << "enter real(A), imag(A), real(B) and imag(B) >> ";
    std::cin >> realA >> imaginaryA >> realB >> imaginaryB;
    std::cout << std::endl;

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

    /*
     * Calling the functions to add, subtract, multiply and 
     * divide the two complex numbers.
     */
    ComplexNumber c = a + b;
    c.printNumber('+');

    c = a - b;
    c.printNumber('-');

    c = a * b;
    c.printNumber('*');

    c = a / b;
    c.printNumber('/');

    return 0;
}

If you have any suggestions on further improving the code, I would really appreciate it if you share them with me.

\$\endgroup\$
  • 2
    \$\begingroup\$ Stop using the this-> notation. It is only necessary to distinguish between member names and method parameters. Reduce the amount of typing effort and possible typos. \$\endgroup\$ – Thomas Matthews Feb 3 at 21:14
  • 2
    \$\begingroup\$ @ThomasMatthews probably a habit coming from Java. personally I like it because I think it is clearer. \$\endgroup\$ – MCMastery Feb 4 at 1:17
  • 2
    \$\begingroup\$ I've never programmed in Java, but I use this-> extensively in C++ to refer to member fields, so it's not just a Java-acquired habit. I also think it adds clarity. The alternative for me would be some sort of prefix, like m_, which a lot of people think is even uglier. Overall, if having this-> makes your code hard to read, then there are more fundamental problems, just like if having std:: makes your code hard to read. \$\endgroup\$ – Cody Gray Feb 4 at 21:48
15
\$\begingroup\$

Use field initialization lists:

So your constructor

ComplexNumber(double real = 0.0, double imaginary = 0.0) {
    this->real = real;
    this->imaginary = imaginary;
}

Can become:

ComplexNumber(double real = 0.0, double imaginary = 0.0)
    : real(real), imaginary(imaginary) { }

Simplify your returns

I could see an argument for making an extra ComplexNumber to hold your return value if you need to further modify it or if the name of that variable is explanatory in showing what the return means, but as it stands, your c is neither of those.

Simplify

ComplexNumber c = ComplexNumber(real, imaginary);
return c;

To just

return ComplexNumber(real, imaginary);

Make your operator functions const

Since you (correctly) don't modify a when you do a + b, the operator function can (and should) be declared const. That way, even if you have a const object, you'll still be able to call it (and if you accidentally try to modify the member variable, you'll know immediately in the form of a compilation error).

That'd look like:

ComplexNumber operator+(const ComplexNumber &b) const {

Notice I've also declared b as const here since you shouldn't be modifying it either. I've also passed it by reference to save you some overhead.

Make your class printable with std::cout

Your printNumber is very specific. In fact, if you ever want to use this class for anything other than simply showing arithmetic results, that print may not be what you want. Instead, I'd make a generic str() that just returns a string version of the complex number. Something like:

std::string str() {
    std::ostringstream oss;
    oss << this->real << " + (" << this->imaginary << ") * i";
    return oss.str(); 
}

And then in the global scope, you can overload the << operator for std::cout:

std::ostream& operator<<(std::ostream &os, const ComplexNumber &cn) {
    return os << cn.str();
}

And now when you want to print it in main(), you can say:

std::cout << "a + b = " << a + b << std::endl;
std::cout << "a - b = " << a - b << std::endl;
std::cout << "a * b = " << a * b << std::endl;
std::cout << "a / b = " << a / b << std::endl;

Look at how easy that becomes to read and understand!

\$\endgroup\$
  • 3
    \$\begingroup\$ Terminology - it's not a syntax error to attempt to write a read-only value. Syntax refers to parsing the program text. That's just a nit-pick; great review otherwise! \$\endgroup\$ – Toby Speight Feb 3 at 17:54
  • \$\begingroup\$ @Toby perhaps I have it wrong, but I believe in C++ trying to modify a read-only (or const) variable will result in a syntax error as a result of the language being strongly typed (and so this being something the compiler can parse and catch), no? \$\endgroup\$ – scohe001 Feb 3 at 18:15
  • \$\begingroup\$ Hmm I think we may be saying the same thing here @Toby. Trying to modify a const variable is likely the result of a semantic error, but it'll be caught at compile time and shown as a syntax error by the compiler, which lines up with: "and if you accidentally try to modify the member variable, you'll know immediately in the form of a syntax error" right? Is there some way you think I should edit that line to make it more correct with what you're saying? \$\endgroup\$ – scohe001 Feb 3 at 19:35
  • 5
    \$\begingroup\$ Yes, it's a compilation error. No, it's not an error of syntax. That's the minor distinction I'm making. In English, "My car blue is" is a syntax error, and "My car smells blue" is not a syntax error, but it's still broken. See syntax versus sematics \$\endgroup\$ – Toby Speight Feb 3 at 21:01
  • 1
    \$\begingroup\$ Ahh I think I see @Toby. I guess I've had it in my head that a compilation error is always a syntax error. But it looks like it's more of a rectangle/square situation. \$\endgroup\$ – scohe001 Feb 3 at 21:09
8
\$\begingroup\$

Member Access

In the real world, people often care about being able to look at the real and imaginary parts of a complex number individually. As such, you will want to provide an interface to them. While contrary to some of the advice you revived in your last review, I'd advise giving these members variables public access. These components are not an implementation detail of your class. Being able to freely read and mutate the components of a complex number is simply part of the expected interface.

Coupling with main and std::cout

In your current implementation, ComplexNumber includes a public function printNumber to display the complex number as an expression of a and b. However, a and b have no meaning within the class itself, and only exist in your main function. Likewise, printNumber always prints the complex number to std::cout. Out in the wild, developers may want to write a complex number to other places, such as std::cerr or a file.

Right now, this functionality isn't as useful as it could be for an outside user. What would be more helpful is the ability to print a complex number itself to any output stream.

The most robust way to accomplish this would be by overloading the I/O operators. A possible implemetation might look like

class ComplexNumber {
    // ... snip
    friend std::ostream& operator<<(std::ostream &out, ComplexNumber c);
};

std::ostream& operator<<(std::ostream &out, ComplexNumber c) {
    out << c.real << " + " << c.imaginary << 'i';
    return out;
}

Using this implementation, you can print ComplexNumber instances directly to std::cout via

ComplexNumber c(2, 3);    
std::cout << c;  // prints 2 + 3i
\$\endgroup\$
  • \$\begingroup\$ While I mostly agree with you on "Member Access", it's interesting to note that std::complex does not. \$\endgroup\$ – Toby Speight Feb 4 at 9:49
  • \$\begingroup\$ Hello, I've tried to understand the syntax for a couple of days now, but I don't really get it. Could you maybe explain the "friend"-syntax you are using to me? \$\endgroup\$ – chrysaetos99 Feb 8 at 12:16
  • 1
    \$\begingroup\$ @chrysaetos99 See the explanation given on this website. Using friend functions is a common idiom to allow overloaded operators to access the private state of a class without being member functions themselves. \$\endgroup\$ – Brian Feb 8 at 16:20
7
\$\begingroup\$

Operator Consistency

You provide operators for +, -, etc., but as it is some things I would expect to do are illegal, such as

ComplexNumber c(1, 2);
ComplexNumber d(3, 4);
d += c;

Generally the recommendation with these forms of operators is to implement the += form in your class, and then define + as a non-member in terms of +=. For example:

class ComplexNumber {
public:
  // ...
  ComplexNumber& operator+=(ComplexNumber b) {
    this->real += b.real;
    this->imaginary += b.imaginary;
    return *this;
  }

  friend ComplexNumber operator+(ComplexNumber a, ComplexNumber b) {
    // note a is a copy here
    a += b;
    return a;
  }

  // and so forth for -, *, /
};

Doing it this way also means that

ComplexNumber c(1, 2);
ComplexNumber d = c + 1;  // compiles with both your code and mine
ComplexNumber e = 1 + c;  // only compiles with the above changes

will compile. If it's not desirable that a number 1 will implicitly convert to a ComplexNumber, consider marking your constructor explicit.

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

As in your previous question, your interface is still awkward. That is, if there's an add method, I fully expect that calling a.add(b) will mean that a results in a plus b. So in particular, the state of a will be changed.

A user of your class will also find void printNumber(char mathOperator) weird. Indeed, why as a user of the class do I need to worry about such details meaning the parameter? The user will just want to get his/her complex number printed and not be forced to worry about such details. So such a function might make sense as a private workhorse (but do make it const and read more about const - it's good for you) that operator<< can call, as explained in another answer.

\$\endgroup\$
5
\$\begingroup\$

People have talked about simplifying the return temporary, but not the interior temporaries:

ComplexNumber operator+(ComplexNumber a, ComplexNumber b) {
    //Just add real- and imaginary-parts
    return ComplexNumber(a.real + b.real,
                         a.imaginary + b.imaginary);
}

Conversely, sometimes you should make a temporary. Notably, the denominator in the a/b calculation should be a temporary. Mind you, this is the absolute value of b, so maybe that line reads (given the relevant function is defined):

double abs_b = abs(b);
\$\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.