Conversation
glpuga
left a comment
glpuga
left a comment
There was a problem hiding this comment.
Good work! first round of reviews!
| } | ||
| ], | ||
| "version": 4 | ||
| } No newline at end of file |
There was a problem hiding this comment.
This file should probably not have made it into the repo, right?
course/include/Vector3.h
Outdated
| @@ -0,0 +1,49 @@ | |||
| #ifndef VECTOR3_H | |||
| #define VECTOR3_H | |||
There was a problem hiding this comment.
You can now use #pragma once instead of using guard macros. https://en.wikipedia.org/wiki/Pragma_once
course/include/Vector3.h
Outdated
| public: | ||
| explicit Vector3(double x, double y, double z) : x_(x), y_(y), z_(z) {} | ||
| explicit Vector3() : x_(0.0), y_(0.0), z_(0.0) {} | ||
| double x() const { return x_; } |
There was a problem hiding this comment.
Better return a const double & and avoid an additional copy. For doubles is probably not that important, but if it where a larger variable type it can improve performance.
course/include/Vector3.h
Outdated
| explicit Vector3() : x_(0.0), y_(0.0), z_(0.0) {} | ||
| double x() const { return x_; } | ||
| double y() const { return y_; } | ||
| double z() const { return z_; } |
There was a problem hiding this comment.
You need to create updatable versions of these functions; that is, versions that return non-const lvalue references, so that you can write vector.x() = 3.0;.
course/include/Vector3.h
Outdated
| double dot(const Vector3 &q) const; | ||
| Vector3 cross(const Vector3 &q) const; | ||
| //operator overload | ||
| const double& operator[](int index) const; |
There was a problem hiding this comment.
Make the index parameter be const.
| ss << p; | ||
| EXPECT_EQ(ss.str(), "(x: 1, y: 2, z: 3)"); | ||
| EXPECT_TRUE(Vector3::kUnitX == Vector3(1., 0., 0.)); | ||
|
|
course/test/src/isometry_TEST.cc
Outdated
| EXPECT_EQ(ss.str(), "(x: 1, y: 2, z: 3)"); | ||
| EXPECT_TRUE(Vector3::kUnitX == Vector3(1., 0., 0.)); | ||
|
|
||
| EXPECT_TRUE(Vector3::kUnitX != Vector3(1., 1., 0.)); |
There was a problem hiding this comment.
Better make them an initializer list, and implement equality between vectors and initializer lists.
course/test/src/isometry_TEST.cc
Outdated
| t[0] = 1.; | ||
| t[1] = 2.; | ||
| t.z() = 3.; | ||
| t[2] = 3.; |
There was a problem hiding this comment.
This was ok the way it was. You need to implement a z() overload that returns an lvalue reference so that you can use it on the left side of assingments.
course/test/src/isometry_TEST.cc
Outdated
| t.z() = 3.; | ||
| t[2] = 3.; | ||
| EXPECT_EQ(t, p); | ||
|
|
| */ | ||
| } | ||
| } // namespace test | ||
| } // namespace cppcourse |
There was a problem hiding this comment.
Probably we don't need another namespace layer, but I'll leave it up to you.
…12020 into Vector3Object
glpuga
left a comment
glpuga
left a comment
There was a problem hiding this comment.
Good work! Second round of review done.
| @@ -0,0 +1,2 @@ | |||
| /build | |||
| /.vscode | |||
There was a problem hiding this comment.
I'm ok with this, but keep in mind that usually is not a good idea to update the .gitignore file in the clients repo to ignore files in your own personal computer setup.
| class Vector3 | ||
| { | ||
| public: | ||
| explicit Vector3(); |
| explicit Vector3(const double x, const double y, const double z); | ||
| Vector3(const std::initializer_list<double> &list); | ||
|
|
||
| double x() const {return values[0];} |
There was a problem hiding this comment.
nit: This is fine, but you could instead return a const double &, which will in general avoid an additional copy. For double is not really important, but for larger objects it may be.
| double x() const {return values[0];} | ||
| double &x() {return values[0];} | ||
|
|
||
| double y() const { return values[1]; } |
| double y() const { return values[1]; } | ||
| double &y() {return values[1];} | ||
|
|
||
| double z() const { return values[2]; } |
| { | ||
| return values[index]; | ||
| } | ||
| catch (const std::exception &ex) |
There was a problem hiding this comment.
Please add a test for this case in the test file.
| { | ||
| return values[index]; | ||
| } | ||
| catch (const std::exception &ex) |
There was a problem hiding this comment.
Please also add a test for this case.
|
|
||
| private: | ||
|
|
||
| double *values = new double[3]; |
There was a problem hiding this comment.
If you don't release this memory somewhere, this memory will get leaked everytime you create one of these objects.
| { | ||
| public: | ||
| explicit Vector3(); | ||
| explicit Vector3(const double x, const double y, const double z); |
| { | ||
| public: | ||
| explicit Vector3(); | ||
| explicit Vector3(const double x, const double y, const double z); |
There was a problem hiding this comment.
Apply the rule-of-five and create move operations: construct and assign. (this is why I wanted an allocated storage).
2 participants
No description provided.