Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

I see a number of things that may help you improve your program.

Include all required headers

The template has calls to acos and sin but doesn't include the <cmath> header which is required. The code should have this line:

#include <cmath>

Use namespaces

The previously mentioned acos and sin should use the namespace prefix as std::acos and std::sin.

Don't use anonymous structs

While they are standard in C11, they are not allowed in C++. There are compiler extensions that can enable them but they don't conform to the standard they don't conform to the standard. In this case, you could either name it or use the next suggestion.

Eliminate the union

Within the array is never used. This suggests that the union could simply be eliminated. If you think that users of the code are going to want to refer to the elements as an array, you could provide an operator[] implementation for that with cleaner syntax anyway.

Eliminate vec2

The vec3 class does not seem to rely on a vec2 for existence or functionality, so it should not be a part of the class definition. Right now, it's not possible to use the class unless one has a vec2 also available, but that dependency is also not #included per the first suggestion.

Don't use #pragma once

Although it is supported by some compilers, code which is intended to be reused should avoid non-standard extensions. By definition, all #pragma are non-standard. For portable code, you should use the standard include guards use the standard include guards. Even if you are only ever using one compiler at the moment, you will want to know the portable way of accomplishing this.

Think about unsigned numbers

It may be that you intended for this class to use exclusively floating point numbers, but there isn't any checking of that, so the result is that this line:

std::cout << vec3<unsigned>::left.toString() << '\n';

compiles and runs just fine, but gives a peculiar result:

[x: 4294967295, y: 0, z: 0]

I would recommend adding a static_assert to the class:

static_assert(std::is_signed<T>{}, "Error: vec3 type must be signed");

Use appropriate floating point types

If I am using a vec3<double>, I might not be happy that angle is internally using only a float instead. Consider using either T or double where float is currently used within the class member functions.

Wrap it in a namespace

To make sure your vec3 doesn't collide with another in some other library, you might want to wrap the whole thing up in your own namespace.

I see a number of things that may help you improve your program.

Include all required headers

The template has calls to acos and sin but doesn't include the <cmath> header which is required. The code should have this line:

#include <cmath>

Use namespaces

The previously mentioned acos and sin should use the namespace prefix as std::acos and std::sin.

Don't use anonymous structs

While they are standard in C11, they are not allowed in C++. There are compiler extensions that can enable them but they don't conform to the standard. In this case, you could either name it or use the next suggestion.

Eliminate the union

Within the array is never used. This suggests that the union could simply be eliminated. If you think that users of the code are going to want to refer to the elements as an array, you could provide an operator[] implementation for that with cleaner syntax anyway.

Eliminate vec2

The vec3 class does not seem to rely on a vec2 for existence or functionality, so it should not be a part of the class definition. Right now, it's not possible to use the class unless one has a vec2 also available, but that dependency is also not #included per the first suggestion.

Don't use #pragma once

Although it is supported by some compilers, code which is intended to be reused should avoid non-standard extensions. By definition, all #pragma are non-standard. For portable code, you should use the standard include guards. Even if you are only ever using one compiler at the moment, you will want to know the portable way of accomplishing this.

Think about unsigned numbers

It may be that you intended for this class to use exclusively floating point numbers, but there isn't any checking of that, so the result is that this line:

std::cout << vec3<unsigned>::left.toString() << '\n';

compiles and runs just fine, but gives a peculiar result:

[x: 4294967295, y: 0, z: 0]

I would recommend adding a static_assert to the class:

static_assert(std::is_signed<T>{}, "Error: vec3 type must be signed");

Use appropriate floating point types

If I am using a vec3<double>, I might not be happy that angle is internally using only a float instead. Consider using either T or double where float is currently used within the class member functions.

Wrap it in a namespace

To make sure your vec3 doesn't collide with another in some other library, you might want to wrap the whole thing up in your own namespace.

I see a number of things that may help you improve your program.

Include all required headers

The template has calls to acos and sin but doesn't include the <cmath> header which is required. The code should have this line:

#include <cmath>

Use namespaces

The previously mentioned acos and sin should use the namespace prefix as std::acos and std::sin.

Don't use anonymous structs

While they are standard in C11, they are not allowed in C++. There are compiler extensions that can enable them but they don't conform to the standard. In this case, you could either name it or use the next suggestion.

Eliminate the union

Within the array is never used. This suggests that the union could simply be eliminated. If you think that users of the code are going to want to refer to the elements as an array, you could provide an operator[] implementation for that with cleaner syntax anyway.

Eliminate vec2

The vec3 class does not seem to rely on a vec2 for existence or functionality, so it should not be a part of the class definition. Right now, it's not possible to use the class unless one has a vec2 also available, but that dependency is also not #included per the first suggestion.

Don't use #pragma once

Although it is supported by some compilers, code which is intended to be reused should avoid non-standard extensions. By definition, all #pragma are non-standard. For portable code, you should use the standard include guards. Even if you are only ever using one compiler at the moment, you will want to know the portable way of accomplishing this.

Think about unsigned numbers

It may be that you intended for this class to use exclusively floating point numbers, but there isn't any checking of that, so the result is that this line:

std::cout << vec3<unsigned>::left.toString() << '\n';

compiles and runs just fine, but gives a peculiar result:

[x: 4294967295, y: 0, z: 0]

I would recommend adding a static_assert to the class:

static_assert(std::is_signed<T>{}, "Error: vec3 type must be signed");

Use appropriate floating point types

If I am using a vec3<double>, I might not be happy that angle is internally using only a float instead. Consider using either T or double where float is currently used within the class member functions.

Wrap it in a namespace

To make sure your vec3 doesn't collide with another in some other library, you might want to wrap the whole thing up in your own namespace.

Source Link
Edward
  • 67.2k
  • 4
  • 120
  • 284

I see a number of things that may help you improve your program.

Include all required headers

The template has calls to acos and sin but doesn't include the <cmath> header which is required. The code should have this line:

#include <cmath>

Use namespaces

The previously mentioned acos and sin should use the namespace prefix as std::acos and std::sin.

Don't use anonymous structs

While they are standard in C11, they are not allowed in C++. There are compiler extensions that can enable them but they don't conform to the standard. In this case, you could either name it or use the next suggestion.

Eliminate the union

Within the array is never used. This suggests that the union could simply be eliminated. If you think that users of the code are going to want to refer to the elements as an array, you could provide an operator[] implementation for that with cleaner syntax anyway.

Eliminate vec2

The vec3 class does not seem to rely on a vec2 for existence or functionality, so it should not be a part of the class definition. Right now, it's not possible to use the class unless one has a vec2 also available, but that dependency is also not #included per the first suggestion.

Don't use #pragma once

Although it is supported by some compilers, code which is intended to be reused should avoid non-standard extensions. By definition, all #pragma are non-standard. For portable code, you should use the standard include guards. Even if you are only ever using one compiler at the moment, you will want to know the portable way of accomplishing this.

Think about unsigned numbers

It may be that you intended for this class to use exclusively floating point numbers, but there isn't any checking of that, so the result is that this line:

std::cout << vec3<unsigned>::left.toString() << '\n';

compiles and runs just fine, but gives a peculiar result:

[x: 4294967295, y: 0, z: 0]

I would recommend adding a static_assert to the class:

static_assert(std::is_signed<T>{}, "Error: vec3 type must be signed");

Use appropriate floating point types

If I am using a vec3<double>, I might not be happy that angle is internally using only a float instead. Consider using either T or double where float is currently used within the class member functions.

Wrap it in a namespace

To make sure your vec3 doesn't collide with another in some other library, you might want to wrap the whole thing up in your own namespace.

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /