patterncppModerate
Simple complex number class
Viewed 0 times
numbersimpleclasscomplex
Problem
This is my first time writing C++, so I would appreciate advice in the areas of:
complex.h:
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
- 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;
};
#endifcomplex.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:
-
You should provide compound-assignment-operators
-
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
-
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
All other constructors (and all functions but
And most should be marked
-
Construction from
-
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
-
As the class only contains two
(You might benefit from comparing your code with
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
modulusfor the absolute value, even though it seems to be perfectly correct, because there's a far more common and shorter way: Just call itabs.
.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.