HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppModerate

Simple complex number class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
numbersimpleclasscomplex

Problem

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 

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
#include
#include

#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 i

Solution

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

-
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.



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

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

-
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.)

-
.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.

-
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.

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

-
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.

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

-
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.)

Context

StackExchange Code Review Q#110783, answer score: 10

Revisions (0)

No revisions yet.