Skip to main content
Code Review

Return to Answer

added 68 characters in body
Source Link
JDługosz
  • 11.7k
  • 19
  • 40

Don't use #define for constants or "functions" (⧺ES.31).


⧺Enum.5 Don't use ALL_CAPS for enumerators and ⧺ES.9 Avoid ALL_CAPS names.


Writing identifiers with a leading underscore is a bad idea, and is generally dissuaded as a style. Note that it is legal for member names if (and only if) the next character is not a capital letter or another underscore.


 ~ADNS2610( ) { };

If you have a destructor that looks like this, don’t say it at all. The compiler knows it is "trivial" if you do not declare one. If you must mention it, you could declare it with =default but that still counts as a declaration and will suppress the generation of move semantics.


Many of the functions named "get..." seem like they are accessors. They should be const members. You have not used const anywhere in the code posted, so there are probably many places where it should be inserted.


address |= ( 1 << 7 );//Specify data direction

That might be clearer using binary literals, now that they are available. 0b10000000.


address & ( 1 << i )

Is the compiler smart enough to figure out that you shift the mask one position each time through the loop? It might generate better code as well as being clearer to state the intent more directly:

for (uint8_t mask= 0b10000000;0b1000'0000; mask; mask>>=1) { ⋯

(or 0x80 or a named constexpr — whatever is easier to read)


static uint8_t frame[ N_PIXELS ];
for( int i = 0; i < N_PIXELS; i++ )//Step 3. in the above algorithm
{
 frame[ i ] = something
}

prefer to use the range-for.

for (auto& x : frame) {
 x = something;
}

Don't use #define for constants or "functions" (⧺ES.31).


⧺Enum.5 Don't use ALL_CAPS for enumerators and ⧺ES.9 Avoid ALL_CAPS names.


Writing identifiers with a leading underscore is a bad idea, and is generally dissuaded as a style. Note that it is legal for member names if (and only if) the next character is not a capital letter or another underscore.


 ~ADNS2610( ) { };

If you have a destructor that looks like this, don’t say it at all. The compiler knows it is "trivial" if you do not declare one. If you must mention it, you could declare it with =default but that still counts as a declaration and will suppress the generation of move semantics.


Many of the functions named "get..." seem like they are accessors. They should be const members. You have not used const anywhere in the code posted, so there are probably many places where it should be inserted.


address |= ( 1 << 7 );//Specify data direction

That might be clearer using binary literals, now that they are available. 0b10000000.


address & ( 1 << i )

Is the compiler smart enough to figure out that you shift the mask one position each time through the loop? It might generate better code as well as being clearer to state the intent more directly:

for (uint8_t mask= 0b10000000; mask; mask>>=1) { ⋯

static uint8_t frame[ N_PIXELS ];
for( int i = 0; i < N_PIXELS; i++ )//Step 3. in the above algorithm
{
 frame[ i ] = something
}

prefer to use the range-for.

for (auto& x : frame) {
 x = something;
}

Don't use #define for constants or "functions" (⧺ES.31).


⧺Enum.5 Don't use ALL_CAPS for enumerators and ⧺ES.9 Avoid ALL_CAPS names.


Writing identifiers with a leading underscore is a bad idea, and is generally dissuaded as a style. Note that it is legal for member names if (and only if) the next character is not a capital letter or another underscore.


 ~ADNS2610( ) { };

If you have a destructor that looks like this, don’t say it at all. The compiler knows it is "trivial" if you do not declare one. If you must mention it, you could declare it with =default but that still counts as a declaration and will suppress the generation of move semantics.


Many of the functions named "get..." seem like they are accessors. They should be const members. You have not used const anywhere in the code posted, so there are probably many places where it should be inserted.


address |= ( 1 << 7 );//Specify data direction

That might be clearer using binary literals, now that they are available. 0b10000000.


address & ( 1 << i )

Is the compiler smart enough to figure out that you shift the mask one position each time through the loop? It might generate better code as well as being clearer to state the intent more directly:

for (uint8_t mask= 0b1000'0000; mask; mask>>=1) { ⋯

(or 0x80 or a named constexpr — whatever is easier to read)


static uint8_t frame[ N_PIXELS ];
for( int i = 0; i < N_PIXELS; i++ )//Step 3. in the above algorithm
{
 frame[ i ] = something
}

prefer to use the range-for.

for (auto& x : frame) {
 x = something;
}
Source Link
JDługosz
  • 11.7k
  • 19
  • 40

Don't use #define for constants or "functions" (⧺ES.31).


⧺Enum.5 Don't use ALL_CAPS for enumerators and ⧺ES.9 Avoid ALL_CAPS names.


Writing identifiers with a leading underscore is a bad idea, and is generally dissuaded as a style. Note that it is legal for member names if (and only if) the next character is not a capital letter or another underscore.


 ~ADNS2610( ) { };

If you have a destructor that looks like this, don’t say it at all. The compiler knows it is "trivial" if you do not declare one. If you must mention it, you could declare it with =default but that still counts as a declaration and will suppress the generation of move semantics.


Many of the functions named "get..." seem like they are accessors. They should be const members. You have not used const anywhere in the code posted, so there are probably many places where it should be inserted.


address |= ( 1 << 7 );//Specify data direction

That might be clearer using binary literals, now that they are available. 0b10000000.


address & ( 1 << i )

Is the compiler smart enough to figure out that you shift the mask one position each time through the loop? It might generate better code as well as being clearer to state the intent more directly:

for (uint8_t mask= 0b10000000; mask; mask>>=1) { ⋯

static uint8_t frame[ N_PIXELS ];
for( int i = 0; i < N_PIXELS; i++ )//Step 3. in the above algorithm
{
 frame[ i ] = something
}

prefer to use the range-for.

for (auto& x : frame) {
 x = something;
}
lang-cpp

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