Skip to main content
Code Review

Return to Answer

deleted 19 characters in body
Source Link
uli
uli
  • 1.7k
  • 9
  • 14

Range Limits

Your function's description says "1024B -> 1KB", but that doesn't work, you have an off-by-one error in your implementation. This also suggests that you haven't unit-tested your code. BTW: I think you have a one-off error as well, because size is not the number of elements in the array, but one less. The number of elements is idiomatically computed by (sizeof array)/(sizeof array[0]).

Another issue with those range limits in your code is that int is a signed type and you're completely ignoring negative values. Additionally, you're converting the number to a floating-point type, which causes a loss of precision.

Scaling Factor

One thing you've not gotten right is the prefixes for the different sizes. The correct ones should be ki, Mi, Gi etc. Alternatively, use the scaling factors of 1000, not 1024. In any case, the prefixesprefix k (1000) and ki (1024) areis not written with capital letters! More in-depth info can be found in Wikipedia.

Non-Standard Exception Constructor

If you look at std::exception docs, you will not find a constructor taking a char const*. Your code only works because your compiler ships with a non-standard extension. Instead, use std::runtime_error, which has a constructor taking a string. Then, you can also drop the .c_str() from the expression it's initialized with.

Namespaced Macros

...aren't. Your code

namespace error_handler {
    #define CATCH(send_to_developers_function) \
    ...
}

looks like it's isolating the macro in a namespace, but it doesn't. Macros just don't work like that, so that part is just misleading. Avoid macros in general (I guess you know that), but in this case add a check that this macro isn't already defined. Especially with common identifiers like THROW, collisions with other code are likely.

Magic Numbers

This code

char message[128];
snprintf(message, 128, text, args...);

has two problems. One is that it's repeating the size of the array. Using above way to represent the size would avoid that. The other is probably some kind of cargo cult programming. Using powers of two isn't going to make your code better, faster or more correct. Why not use 100 or 200? How did you pick that number at all? This decision is what costs most time during development, so it is definitely worth documenting! In this case, it's probably just "I guess this is enough", but even that would be helpful to understand the code.

Range Limits

Your function's description says "1024B -> 1KB", but that doesn't work, you have an off-by-one error in your implementation. This also suggests that you haven't unit-tested your code. BTW: I think you have a one-off error as well, because size is not the number of elements in the array, but one less. The number of elements is idiomatically computed by (sizeof array)/(sizeof array[0]).

Another issue with those range limits in your code is that int is a signed type and you're completely ignoring negative values. Additionally, you're converting the number to a floating-point type, which causes a loss of precision.

Scaling Factor

One thing you've not gotten right is the prefixes for the different sizes. The correct ones should be ki, Mi, Gi etc. Alternatively, use the scaling factors of 1000, not 1024. In any case, the prefixes k (1000) and ki (1024) are not written with capital letters! More in-depth info can be found in Wikipedia.

Non-Standard Exception Constructor

If you look at std::exception docs, you will not find a constructor taking a char const*. Your code only works because your compiler ships with a non-standard extension. Instead, use std::runtime_error, which has a constructor taking a string. Then, you can also drop the .c_str() from the expression it's initialized with.

Namespaced Macros

...aren't. Your code

namespace error_handler {
    #define CATCH(send_to_developers_function) \
    ...
}

looks like it's isolating the macro in a namespace, but it doesn't. Macros just don't work like that, so that part is just misleading. Avoid macros in general (I guess you know that), but in this case add a check that this macro isn't already defined. Especially with common identifiers like THROW, collisions with other code are likely.

Magic Numbers

This code

char message[128];
snprintf(message, 128, text, args...);

has two problems. One is that it's repeating the size of the array. Using above way to represent the size would avoid that. The other is probably some kind of cargo cult programming. Using powers of two isn't going to make your code better, faster or more correct. Why not use 100 or 200? How did you pick that number at all? This decision is what costs most time during development, so it is definitely worth documenting! In this case, it's probably just "I guess this is enough", but even that would be helpful to understand the code.

Range Limits

Your function's description says "1024B -> 1KB", but that doesn't work, you have an off-by-one error in your implementation. This also suggests that you haven't unit-tested your code. BTW: I think you have a one-off error as well, because size is not the number of elements in the array, but one less. The number of elements is idiomatically computed by (sizeof array)/(sizeof array[0]).

Another issue with those range limits in your code is that int is a signed type and you're completely ignoring negative values. Additionally, you're converting the number to a floating-point type, which causes a loss of precision.

Scaling Factor

One thing you've not gotten right is the prefixes for the different sizes. The correct ones should be ki, Mi, Gi etc. Alternatively, use the scaling factors of 1000, not 1024. In any case, the prefix k (1000) is not written with capital letters! More in-depth info can be found in Wikipedia.

Non-Standard Exception Constructor

If you look at std::exception docs, you will not find a constructor taking a char const*. Your code only works because your compiler ships with a non-standard extension. Instead, use std::runtime_error, which has a constructor taking a string. Then, you can also drop the .c_str() from the expression it's initialized with.

Namespaced Macros

...aren't. Your code

namespace error_handler {
    #define CATCH(send_to_developers_function) \
    ...
}

looks like it's isolating the macro in a namespace, but it doesn't. Macros just don't work like that, so that part is just misleading. Avoid macros in general (I guess you know that), but in this case add a check that this macro isn't already defined. Especially with common identifiers like THROW, collisions with other code are likely.

Magic Numbers

This code

char message[128];
snprintf(message, 128, text, args...);

has two problems. One is that it's repeating the size of the array. Using above way to represent the size would avoid that. The other is probably some kind of cargo cult programming. Using powers of two isn't going to make your code better, faster or more correct. Why not use 100 or 200? How did you pick that number at all? This decision is what costs most time during development, so it is definitely worth documenting! In this case, it's probably just "I guess this is enough", but even that would be helpful to understand the code.

Source Link
uli
uli
  • 1.7k
  • 9
  • 14

Range Limits

Your function's description says "1024B -> 1KB", but that doesn't work, you have an off-by-one error in your implementation. This also suggests that you haven't unit-tested your code. BTW: I think you have a one-off error as well, because size is not the number of elements in the array, but one less. The number of elements is idiomatically computed by (sizeof array)/(sizeof array[0]).

Another issue with those range limits in your code is that int is a signed type and you're completely ignoring negative values. Additionally, you're converting the number to a floating-point type, which causes a loss of precision.

Scaling Factor

One thing you've not gotten right is the prefixes for the different sizes. The correct ones should be ki, Mi, Gi etc. Alternatively, use the scaling factors of 1000, not 1024. In any case, the prefixes k (1000) and ki (1024) are not written with capital letters! More in-depth info can be found in Wikipedia.

Non-Standard Exception Constructor

If you look at std::exception docs, you will not find a constructor taking a char const*. Your code only works because your compiler ships with a non-standard extension. Instead, use std::runtime_error, which has a constructor taking a string. Then, you can also drop the .c_str() from the expression it's initialized with.

Namespaced Macros

...aren't. Your code

namespace error_handler {
    #define CATCH(send_to_developers_function) \
    ...
}

looks like it's isolating the macro in a namespace, but it doesn't. Macros just don't work like that, so that part is just misleading. Avoid macros in general (I guess you know that), but in this case add a check that this macro isn't already defined. Especially with common identifiers like THROW, collisions with other code are likely.

Magic Numbers

This code

char message[128];
snprintf(message, 128, text, args...);

has two problems. One is that it's repeating the size of the array. Using above way to represent the size would avoid that. The other is probably some kind of cargo cult programming. Using powers of two isn't going to make your code better, faster or more correct. Why not use 100 or 200? How did you pick that number at all? This decision is what costs most time during development, so it is definitely worth documenting! In this case, it's probably just "I guess this is enough", but even that would be helpful to understand the code.