0
\$\begingroup\$

Will be grateful if anyone would reasonably comment on my code. I am interested in the style only. If there will be any comments which are not related to the style I am glad to read them as well.

Main.cpp

#include "Matrix.h"

int main(void)
{
    Matrix2x2 m;
    std::cout << m << std::endl;
    std::cin >> m;
    std::cout << m << std::endl;

    Matrix2x2 m1{1, 2, 3, 4};
    Matrix2x2 res1{ 5 * m1 };
    std::cout << res1 << std::endl;

    Matrix2x2 m2{ 1, 2, 3, 4 };
    Matrix2x2 res2{ 5+m2 };
    std::cout << res2 << std::endl;

    Matrix2x2 m3{ 1, 1, 1, 1 };
    Matrix2x2 m4{ 1, 2, -1, -1 };
    Matrix2x2 m5 = m3 + m4;
    std::cout << m5 << std::endl;

    m4 += m5;
    std::cout << m4 << std::endl;

    return 0;
}

Matrix2x2.h

#pragma once

#include <iostream>

struct Matrix2x2
{double _00, _01, _10, _11;};

std::istream& operator>>(std::istream&, Matrix2x2&);
std::ostream& operator<<(std::ostream&, Matrix2x2&);
const Matrix2x2 operator*(const double&, const Matrix2x2&);
const Matrix2x2 operator+(const double&, const Matrix2x2&);
const Matrix2x2 operator+(const Matrix2x2&, const Matrix2x2&);
Matrix2x2& operator+=(Matrix2x2&, const Matrix2x2&);

Matrix2x2.cpp

#include "Matrix.h"

std::istream& operator>>(std::istream& is, Matrix2x2& m)
{
    return is >> m._00 >> m._01 >> m._10 >> m._11;
}

std::ostream& operator<<(std::ostream& os, Matrix2x2& m)
{
    return os << m._00 << ' ' << m._01 << std::endl 
        << m._10 << ' ' << m._11 << std::endl;
}

const Matrix2x2 operator*(const double& c, const Matrix2x2& m)
{
    Matrix2x2 res{c*m._00, c*m._01, c*m._10, c*m._11};
    return res;
}

const Matrix2x2 operator+(const double& c, const Matrix2x2& m)
{
    Matrix2x2 res{ c+m._00, c+m._01, c+m._10, c+m._11 };
    return res;
}

const Matrix2x2 operator+(const Matrix2x2& m1, const Matrix2x2& m2)
{
    Matrix2x2 almostRes{m1};
    return almostRes+=m2;
}

Matrix2x2& operator+=(Matrix2x2& trg, const Matrix2x2& src)
{
    trg._00 += src._00;
    trg._01 += src._01;
    trg._10 += src._10;
    trg._11 += src._11;
    return trg;
}
\$\endgroup\$
  • \$\begingroup\$ The code already is ok, may be open to some nitpicks. Without more context there is really nothing to say. What are you planning to use it for? What problem(s) does it solve? \$\endgroup\$ – Incomputable Nov 30 '17 at 22:22
  • \$\begingroup\$ @Incomputable, thank you for the attention. I am going to have an exam related to that and the task from in the previous year was to implement all that. \$\endgroup\$ – trafalgarLaww Nov 30 '17 at 22:40
2
\$\begingroup\$

Leading _

Some combinations of leading underscore are reserved specifically __ and _<CapitalLetter>, you're one character away from invalid code it's easier to stay away. See this answer.

Document Datatypes

The only way to discern what values are where in your matrix is to assume that the indices of your member variables are row/column, there is no documentation to that effect. I have to look at the out stream operator to figure that out. Always document assumptions or implicit behavior.

Scalar operations missing reverse operator

Matrix m1, m2;
double d;
m1 = m2 * d;

won't compile, you need to implement both version of asymmetric operators somehow.

Various matrix operations missing

For completeness there are quite a few operations missing, multiplication, inverse, determinant ...

const qualifier on returns will cause extra copy

Matrix m1, m2, m3;
m1 = m2 + m3;

will create an extra copy of the result matrix on the assignment. You marked the return value of + as const this means that when it's assigned to a non-const variable a copy will be created. You are doing this on all the binary operators is there a specific reason you did this ?

Fixed Matrices

There should be an easy way to get an identity, or a zero matrix it's not necessary but a nice to have.

Operator + might be inefficient

While delegating the addition to the += operator saves you from repeating some code it could be better to construct the correct matrix in place and return it

const Matrix2x2 operator+(const Matrix2x2& m1, const Matrix2x2& m2)
{
    return Matrix2x2{m1._00 + m2._00, m1._01 + m2._01,
                     m1._10 + m2._10, m1._11 + m2._11};
}

Btw you could use this form for all you operations.

Edit 1

const return value

You responded that this was something the lecturer advised you to do to protect against f() = x being written, if that is what he asks you should do it in that class. They are correct, having a const return value will prevent the compilation. This is a good thing as a statement like that most likely has no effect and the result of f() will immediately go out of scope.

struct X;

X function() {};
const X constFunction() {};

X newValue;
// Compiles correctly but there is no effect (in most cases)
// as the result of function() will be discarded immediately
// There would be an effect if operator= is overridden and has 
// side effects. I would consider that a really bad idea.
function() = newValue; 

// The following line won't compile, therefor telling you
// "Don't do this"
constFunction() = newValue; 

In the code I usually work with i would definitely advise against it. In general I would be suspicious of any code that assigned directly to the result of a free function. I'd rather go with "Don't assign to the result of a free function" as a style rule. In modern c++ doing this prevents the use of move semantics on the result of your function.

With member functions this would be different returning a reference or a const references is something that may happen a lot. So there are cases where you might actually write object.getter() = x. But it's dealing with references as results and not values.

Operator efficiency

I changed the wording to reflect this being an option, it's great that you went and checked the assembly, i did the same (https://godbolt.org/g/Kh3BsJ). For me it looks like that for clang and gcc the solutions are equivalent except for a different use of registers, I am not qualified to say how this affects the actual performance. For MSVC the case looks different the while in place solution is almost the same as what clang/gcc do, the two call path is definitely longer. As a general thought, the simpler your code is the easier it will be to optimize for the compiler. This is a discussion to be had when the need for optimization arises. You're doing great, keep going.

\$\endgroup\$
  • \$\begingroup\$ I severely disagree with your last point. Any compiler will be able to optimize this away, and the value of not having to duplicate code far exceeds the minor performance hit in debug builds. \$\endgroup\$ – Frank Nov 30 '17 at 23:04
  • \$\begingroup\$ Yeah, about the last point you are definitely wrong. I looked at the assembly commands and there are seven times less commands when implementing addition in my way. \$\endgroup\$ – trafalgarLaww Dec 3 '17 at 0:00
  • \$\begingroup\$ The lecturer said to us that it is better to return const value, since then you won`t be able to write f(p) = s;. Actually I do not understand it very clear. \$\endgroup\$ – trafalgarLaww Dec 3 '17 at 0:01
  • \$\begingroup\$ @trafalgarLaww added more information to the two point under discussion \$\endgroup\$ – Harald Scheirich Dec 3 '17 at 15:19
  • \$\begingroup\$ @HaraldScheirich, thanks for the attention. \$\endgroup\$ – trafalgarLaww Dec 3 '17 at 17:15

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.