Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

Naming

#Naming I'mI'm not fond of your naming. For example, you have a type named px3DArr. Above it you have a comment that says:

// Special class for color pixel RGB.

If this class represents an RGB pixel, then name it that! It should just be RGBPixel or PixelRGB. And while it's possible to think of a pixel as being an array, it's not really an array. It's a data structure with 3 members. In some formats the pixels have a different number of bits, for example. So even if you back it with a std::array, I would leave the word "array" (or any abbreviations of "array") out of the name. A user of this class shouldn't have to know the implementation details to understand what it is.

For function arguments, if you have one named dst, you should probably have one named src.

Data Structures

#Data Structures II really dislike representing a pixel as an array. For one, it leaves you unable to know by looking at the code which color component you're working with. Most of the code here assumes that you need to do the same thing to all the channels of a pixel. While that's sometimes the case, just as often it's not the case. So if I need to treat the green channel differently from the blue channel, how do I know whether to use 0 or 2 for blue? And if it's a 4 channel pixel, how do I know whether it's ARGB or RGBA? (Or BGRA or ABGR?) A pixel is really a data structure and not an array.

When Do I Need This?

#When Do I Need This? ForFor what purpose is a cross product between 2 pixels used? I've used the dot product of 2 pixels before, but never the cross product. I've used the cross product of 2 3D or 4D vectors when dealing with 3D geometry, but not with colors.

Integral Types vs. Floating Point

#Integral Types vs. Floating Point SomeSome of your functions will return unexpected results with integral types. When manipulating pixels, it's frequently the case that you want to treat an integral type as a fixed point type with a value between 0 and 1. So for an unsigned 8-bit value, 0 would represent 0.0, and 255 would represent 1.0. For a function like std::pow(), you need to cast the integral value to a floating point value, divide it by the maximum, perform the operation, multiply by the maximum and convert back. Something like this:

uint_8 x = 127;
uint_8 y = static_cast<uint_8>(pow(static_cast<double>(x) / 255.0, p) * 255.0);

#Naming I'm not fond of your naming. For example, you have a type named px3DArr. Above it you have a comment that says:

// Special class for color pixel RGB.

If this class represents an RGB pixel, then name it that! It should just be RGBPixel or PixelRGB. And while it's possible to think of a pixel as being an array, it's not really an array. It's a data structure with 3 members. In some formats the pixels have a different number of bits, for example. So even if you back it with a std::array, I would leave the word "array" (or any abbreviations of "array") out of the name. A user of this class shouldn't have to know the implementation details to understand what it is.

For function arguments, if you have one named dst, you should probably have one named src.

#Data Structures I really dislike representing a pixel as an array. For one, it leaves you unable to know by looking at the code which color component you're working with. Most of the code here assumes that you need to do the same thing to all the channels of a pixel. While that's sometimes the case, just as often it's not the case. So if I need to treat the green channel differently from the blue channel, how do I know whether to use 0 or 2 for blue? And if it's a 4 channel pixel, how do I know whether it's ARGB or RGBA? (Or BGRA or ABGR?) A pixel is really a data structure and not an array.

#When Do I Need This? For what purpose is a cross product between 2 pixels used? I've used the dot product of 2 pixels before, but never the cross product. I've used the cross product of 2 3D or 4D vectors when dealing with 3D geometry, but not with colors.

#Integral Types vs. Floating Point Some of your functions will return unexpected results with integral types. When manipulating pixels, it's frequently the case that you want to treat an integral type as a fixed point type with a value between 0 and 1. So for an unsigned 8-bit value, 0 would represent 0.0, and 255 would represent 1.0. For a function like std::pow(), you need to cast the integral value to a floating point value, divide it by the maximum, perform the operation, multiply by the maximum and convert back. Something like this:

uint_8 x = 127;
uint_8 y = static_cast<uint_8>(pow(static_cast<double>(x) / 255.0, p) * 255.0);

Naming

I'm not fond of your naming. For example, you have a type named px3DArr. Above it you have a comment that says:

// Special class for color pixel RGB.

If this class represents an RGB pixel, then name it that! It should just be RGBPixel or PixelRGB. And while it's possible to think of a pixel as being an array, it's not really an array. It's a data structure with 3 members. In some formats the pixels have a different number of bits, for example. So even if you back it with a std::array, I would leave the word "array" (or any abbreviations of "array") out of the name. A user of this class shouldn't have to know the implementation details to understand what it is.

For function arguments, if you have one named dst, you should probably have one named src.

Data Structures

I really dislike representing a pixel as an array. For one, it leaves you unable to know by looking at the code which color component you're working with. Most of the code here assumes that you need to do the same thing to all the channels of a pixel. While that's sometimes the case, just as often it's not the case. So if I need to treat the green channel differently from the blue channel, how do I know whether to use 0 or 2 for blue? And if it's a 4 channel pixel, how do I know whether it's ARGB or RGBA? (Or BGRA or ABGR?) A pixel is really a data structure and not an array.

When Do I Need This?

For what purpose is a cross product between 2 pixels used? I've used the dot product of 2 pixels before, but never the cross product. I've used the cross product of 2 3D or 4D vectors when dealing with 3D geometry, but not with colors.

Integral Types vs. Floating Point

Some of your functions will return unexpected results with integral types. When manipulating pixels, it's frequently the case that you want to treat an integral type as a fixed point type with a value between 0 and 1. So for an unsigned 8-bit value, 0 would represent 0.0, and 255 would represent 1.0. For a function like std::pow(), you need to cast the integral value to a floating point value, divide it by the maximum, perform the operation, multiply by the maximum and convert back. Something like this:

uint_8 x = 127;
uint_8 y = static_cast<uint_8>(pow(static_cast<double>(x) / 255.0, p) * 255.0);
Source Link
user1118321
  • 11.9k
  • 1
  • 20
  • 46

#Naming I'm not fond of your naming. For example, you have a type named px3DArr. Above it you have a comment that says:

// Special class for color pixel RGB.

If this class represents an RGB pixel, then name it that! It should just be RGBPixel or PixelRGB. And while it's possible to think of a pixel as being an array, it's not really an array. It's a data structure with 3 members. In some formats the pixels have a different number of bits, for example. So even if you back it with a std::array, I would leave the word "array" (or any abbreviations of "array") out of the name. A user of this class shouldn't have to know the implementation details to understand what it is.

For function arguments, if you have one named dst, you should probably have one named src.

#Data Structures I really dislike representing a pixel as an array. For one, it leaves you unable to know by looking at the code which color component you're working with. Most of the code here assumes that you need to do the same thing to all the channels of a pixel. While that's sometimes the case, just as often it's not the case. So if I need to treat the green channel differently from the blue channel, how do I know whether to use 0 or 2 for blue? And if it's a 4 channel pixel, how do I know whether it's ARGB or RGBA? (Or BGRA or ABGR?) A pixel is really a data structure and not an array.

#When Do I Need This? For what purpose is a cross product between 2 pixels used? I've used the dot product of 2 pixels before, but never the cross product. I've used the cross product of 2 3D or 4D vectors when dealing with 3D geometry, but not with colors.

#Integral Types vs. Floating Point Some of your functions will return unexpected results with integral types. When manipulating pixels, it's frequently the case that you want to treat an integral type as a fixed point type with a value between 0 and 1. So for an unsigned 8-bit value, 0 would represent 0.0, and 255 would represent 1.0. For a function like std::pow(), you need to cast the integral value to a floating point value, divide it by the maximum, perform the operation, multiply by the maximum and convert back. Something like this:

uint_8 x = 127;
uint_8 y = static_cast<uint_8>(pow(static_cast<double>(x) / 255.0, p) * 255.0);
lang-cpp

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