Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vector3 object #7

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions course/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
/build
/.vscode
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

16 changes: 16 additions & 0 deletions course/.vscode/c_cpp_properties.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"configurations": [
{
"name": "Linux",
"includePath": [
"${workspaceFolder}/**"
],
"defines": [],
"compilerPath": "/usr/bin/clang",
"cStandard": "c11",
"cppStandard": "c++17",
"intelliSenseMode": "clang-x64"
}
],
"version": 4
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should probably not have made it into the repo, right?

1 change: 1 addition & 0 deletions course/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ include_directories(
# Library sources.
set(LIBRARY_SOURCES
src/foo.cc
src/Vector3.cc
)

# Library creation.
Expand Down
67 changes: 67 additions & 0 deletions course/include/Vector3.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
#pragma once
#include <iostream>
#include "math.h"

namespace ekumen
{

class Vector3
{
public:
explicit Vector3();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to do this explicit.

explicit Vector3(const double x, const double y, const double z);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a copy constructo.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apply the rule-of-five and create move operations: construct and assign. (this is why I wanted an allocated storage).

Vector3(const std::initializer_list<double> &list);

double x() const {return values[0];}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() {return values[0];}

double y() const { return values[1]; }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

double &y() {return values[1];}

double z() const { return values[2]; }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here.

double &z() {return values[2];}



double norm() const;
double dot(const Vector3 &q) const;
Vector3 cross(const Vector3 &q) const;
//operator overload
const double& operator[](const int index) const;
double& operator[](const int index);


Vector3 &operator+=(const Vector3 q);
Vector3 &operator-=(const Vector3 q);


friend Vector3 operator*(const double &cte, const Vector3 &p);
friend Vector3 operator*(const Vector3 &p, const double &cte);
friend Vector3 operator*(const Vector3 &p, const Vector3 &q);
friend Vector3 operator/(const Vector3 &p, const Vector3 &q);

friend bool operator==(const Vector3 &p, const Vector3 &q);
friend bool operator==(const Vector3 &p, const std::initializer_list<double> &list);

friend bool operator!=(const Vector3 &p, const Vector3 &q);
friend bool operator!=(const Vector3 &p, const std::initializer_list<double> &list);

friend std::ostream &operator<<(std::ostream &os, const Vector3 &p);


static const Vector3 kUnitX;
static const Vector3 kUnitY;
static const Vector3 kUnitZ;
static const Vector3 kZero;

private:

double *values = new double[3];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't release this memory somewhere, this memory will get leaked everytime you create one of these objects.


Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These line breaks are redundant and will usually be removed by an auto-formatter.

};

Vector3 operator+(Vector3 p, Vector3 q);
Vector3 operator-(Vector3 p, Vector3 q);


} // namespace ekumen
5 changes: 5 additions & 0 deletions course/include/isometry.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#pragma once
namespace cppcourse {


} // cppcourse
156 changes: 156 additions & 0 deletions course/src/Vector3.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
#include "Vector3.h"

namespace ekumen
{

Vector3::Vector3(const double x, const double y, const double z)
{
values[0] = x;
values[1] = y;
values[2] = z;
}
Vector3::Vector3()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace this constructor with a delegating constructor that uses the one above: https://www.learncpp.com/cpp-tutorial/8-6-overlapping-and-delegating-constructors/

{
values[0] = 0.;
values[1] = 0.;
values[2] = 0.;
}
Vector3::Vector3(const std::initializer_list<double> &list){
values[0] = *(list.begin() + 0);
values[1] = *(list.begin() + 1);
values[2] = *(list.begin() + 2);
}
double Vector3::norm() const
{
return sqrt(dot(Vector3(x(), y(), z())));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be dot(*this)

}

double Vector3::dot(const Vector3 &q) const
{
return ((values[0] * q.x()) + (values[1] * q.y()) + (values[2] * q.z()));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For readability's sake you can use (x() * q.x()) + (y() * q.y()) + (z() * q.z()) .

}

Vector3 Vector3::cross(const Vector3 &q) const
{
Vector3 r;
r.x() = values[1] * q.z() - values[2] * q.y();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be r.x() = y() * q.z() - z() * q.y();

r.y() = values[2] * q.x() - values[0] * q.z();
r.z() = values[0] * q.y() - values[1] * q.x();
return (r);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for the parenthesis.

}

Vector3 &Vector3::operator+=(const Vector3 q){
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The argument can be const Vector3 &

x() += q.x();
y() += q.y();
z() += q.z();
return *this;
}

Vector3 &Vector3::operator-=(const Vector3 q){
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, better add a &

x() -= q.x();
y() -= q.y();
z() -= q.z();
return *this;
}

Vector3 operator+(Vector3 p, Vector3 q)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arguments should be const Vector3 &

{
Vector3 r;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can better use your (x, y, z) constructor, or better yet, your copy constructor.

r.x() = p.x();
r.y() = p.y();
r.z() = p.z();
return (r+=q);
}

Vector3 operator-(Vector3 p, Vector3 q)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arguments should be const Vector3 &

{
Vector3 r;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use your copy constructor.

r.x() = p.x();
r.y() = p.y();
r.z() = p.z();
return (r-=q);
}

Vector3 operator*(const double &cte, const Vector3 &p)
{
Vector3 res(cte * p.x(), cte * p.y(), cte * p.z());
return res;
}

Vector3 operator*(const Vector3 &p, const double &cte)
{
Vector3 res(cte * p.x(), cte * p.y(), cte * p.z());
return res;
}

Vector3 operator*(const Vector3 &p, const Vector3 &q)
{
Vector3 res(p.x() * q.x(), p.y() * q.y(), p.z() * q.z());
return res;
}

Vector3 operator/(const Vector3 &p, const Vector3 &q)
{
Vector3 res(p.x() / q.x(), p.y() / q.y(), p.z() / q.z());
return res;
}

bool operator==(const Vector3 &p, const Vector3 &q)
{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the clang-format formatter, and if you're using vscode, you can add a plugin to use it from the interface to autoformat everything.

bool res = (p.x() == q.x()) && (p.y() == q.y()) && (p.z() == q.z());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can sue auto instead of bool.

return res;
}

bool operator==(const Vector3 &p, const std::initializer_list<double> &list){
bool res = (p.x() == *(list.begin() + 0)) && (p.y() == *(list.begin() + 1)) && (p.z() == *(list.begin() + 2));
return res;
}

bool operator!=(const Vector3 &p, const Vector3 &q)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better leave line in between.

{

return (!(p == q));
}

bool operator!=(const Vector3 &p, const std::initializer_list<double> &list){
Vector3 q(*(list.begin() + 0),*(list.begin() + 1),*(list.begin() + 2));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To do this you need to call begin() three times. Better call it only once and reuse the same iterator.

return (!(p == q));

}

std::ostream &operator<<(std::ostream &os, const Vector3 &p)
{
os << "(x: " << p.x() << ", y: " << p.y() << ", z: " << p.z() << ")";
return os;
}

const double &Vector3::operator[](const int index) const
{
try
{
return values[index];
}
catch (const std::exception &ex)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test for this case in the test file.

{
std::cerr << "Error occurred: " << ex.what() << std::endl;
}
}

double &Vector3::operator[](const int index)
{
try
{
return values[index];
}
catch (const std::exception &ex)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add a test for this case.

{
std::cerr << "Error occurred: " << ex.what() << std::endl;
}
}

const Vector3 Vector3::kUnitX = Vector3(1., 0., 0.);
const Vector3 Vector3::kUnitY = Vector3(0., 1., 0.);
const Vector3 Vector3::kUnitZ = Vector3(0., 0., 1.);
const Vector3 Vector3::kZero = Vector3(0., 0., 0.);

} // namespace ekumen
2 changes: 1 addition & 1 deletion course/test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ macro (cppcourse_build_tests)
add_executable(${BINARY_NAME} ${GTEST_SOURCE_file})

add_dependencies(${BINARY_NAME}
foo
foo
gtest
gtest_main
)
Expand Down
1 change: 1 addition & 0 deletions course/test/src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ include_directories(

# Test sources.
set (GTEST_SOURCES
isometry_TEST.cc
foo_TEST.cc
)

Expand Down
23 changes: 14 additions & 9 deletions course/test/src/isometry_TEST.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,26 +3,27 @@
// needed to implement an isometry.

// Consider including other header files if needed.
#include "isometry.h"

//#include "isometry.h"
#include "Vector3.h"
#include <cmath>
#include <sstream>
#include <string>
#include <initializer_list>

#include "gtest/gtest.h"

namespace ekumen {
namespace math {
namespace cppcourse {
namespace test {
namespace {

GTEST_TEST(Vector3Test, Vector3Operations) {
const double kTolerance{1e-12};
const Vector3 p{1., 2., 3.};
const Vector3 q{4., 5., 6.};

EXPECT_EQ(p + q, {5., 7., 9.});
EXPECT_EQ(p - q, {-3., -3., -3.});
EXPECT_EQ(p + q, std::initializer_list<double>({5., 7., 9.}));
EXPECT_EQ(p - q, std::initializer_list<double>({-3., -3., -3.}));
EXPECT_EQ(p * 2., Vector3(2., 4., 6));
EXPECT_EQ(2 * q, Vector3(8., 10., 12.));
EXPECT_EQ(p * q, Vector3(4., 10., 18.));
Expand All @@ -40,8 +41,8 @@ GTEST_TEST(Vector3Test, Vector3Operations) {
EXPECT_EQ(ss.str(), "(x: 1, y: 2, z: 3)");

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to add this line.

EXPECT_TRUE(Vector3::kUnitX == Vector3(1., 0., 0));
EXPECT_TRUE(Vector3::kUnitX != {1., 1., 0});
EXPECT_TRUE(Vector3::kUnitY == {0., 1., 0});
EXPECT_TRUE(Vector3::kUnitX != std::initializer_list<double>({1., 1., 0}));
EXPECT_TRUE(Vector3::kUnitY == std::initializer_list<double>({0., 1., 0}));
EXPECT_TRUE(Vector3::kUnitZ == Vector3::kUnitX.cross(Vector3::kUnitY));
EXPECT_NEAR(Vector3::kUnitX.dot(Vector3::kUnitZ), 0., kTolerance);

Expand All @@ -53,6 +54,7 @@ GTEST_TEST(Vector3Test, Vector3Operations) {
EXPECT_EQ(t, p);
}

/*
GTEST_TEST(Matrix3Test, Matrix3Operations) {
const double kTolerance{1e-12};
Matrix3 m1{{1., 2., 3.}, {4., 5., 6.}, {7., 8., 9.}};
Expand Down Expand Up @@ -96,7 +98,9 @@ GTEST_TEST(Matrix3Test, Matrix3Operations) {
ASSERT_TRUE(found);
}
}
*/

/*
GTEST_TEST(IsometryTest, IsometryOperations) {
const double kTolerance{1e-12};
const Isometry t1 = Isometry::FromTranslation({1., 2., 3.});
Expand Down Expand Up @@ -129,9 +133,10 @@ GTEST_TEST(IsometryTest, IsometryOperations) {
ss << t5;
EXPECT_EQ(ss.str(), "[T: (x: 0, y: 0, z: 0), R:[[0.923879533, -0.382683432, 0], [0.382683432, 0.923879533, 0], [0, 0, 1]]]");
}

} // namespace
*/
}
} // namespace test
} // namespace cppcourse
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we don't need another namespace layer, but I'll leave it up to you.

} // namespace math
} // namespace ekumen

Expand Down