Can this code be better? Peer code review.
-
I'm coding in C++, but just using basic C++ libraries (ie. not MFC or WTL etc.). I would like to know if there is a more elegant/better way to do what I have coded below, just to learn something. Basically, given a "code" I want to verify that each "digit" is Octal (ie. 0-7), and that the "code" is exactly 4 characters. I'm using this as an opportunity for a "code review" by you peers to see if I can learn something. bool Xpdr::IsValidCode(const wchar_t* code) { // Has 4 numeric digits if (wcslen(code) != 4) { return false; // Code MUST be 4 digits } for (int i=0; i<4; i++) { // Check each digit is octal if (code[i] < 0x30 || code[i] > 0x37) { return false; // Code digits must be 0 - 7 } } return true; } Thanks for your thoughts on how to make this better / more efficient.
-
I'm coding in C++, but just using basic C++ libraries (ie. not MFC or WTL etc.). I would like to know if there is a more elegant/better way to do what I have coded below, just to learn something. Basically, given a "code" I want to verify that each "digit" is Octal (ie. 0-7), and that the "code" is exactly 4 characters. I'm using this as an opportunity for a "code review" by you peers to see if I can learn something. bool Xpdr::IsValidCode(const wchar_t* code) { // Has 4 numeric digits if (wcslen(code) != 4) { return false; // Code MUST be 4 digits } for (int i=0; i<4; i++) { // Check each digit is octal if (code[i] < 0x30 || code[i] > 0x37) { return false; // Code digits must be 0 - 7 } } return true; } Thanks for your thoughts on how to make this better / more efficient.
I think a possible alternative solution is:
//static bool Xpdr::IsValidCode(const wchar_t * code) const { for( int i = 0; i < 4; ++i, ++code) { if( *code < '0' || *code > '7') { return false; } } return *code == 0; }I hope this works. In addition you can "unroll" this short loop and replace it with a series of "if"-s and autoincremented
codepointer. -- modified at 5:47 Tuesday 28th November, 2006 -
I think a possible alternative solution is:
//static bool Xpdr::IsValidCode(const wchar_t * code) const { for( int i = 0; i < 4; ++i, ++code) { if( *code < '0' || *code > '7') { return false; } } return *code == 0; }I hope this works. In addition you can "unroll" this short loop and replace it with a series of "if"-s and autoincremented
codepointer. -- modified at 5:47 Tuesday 28th November, 2006Ah, very interesting. Nice touch verifying that there is a terminating NULL. By unroll are you meaning something like this: bool Xpdr::IsValidCode(const wchar_t * code) const { if( *code < '0' || *code > '7') { return false; } code++; if( *code < '0' || *code > '7') { return false; } code++; if( *code < '0' || *code > '7') { return false; } code++; if( *code < '0' || *code > '7') { return false; } return *code == 0; }
-
I'm coding in C++, but just using basic C++ libraries (ie. not MFC or WTL etc.). I would like to know if there is a more elegant/better way to do what I have coded below, just to learn something. Basically, given a "code" I want to verify that each "digit" is Octal (ie. 0-7), and that the "code" is exactly 4 characters. I'm using this as an opportunity for a "code review" by you peers to see if I can learn something. bool Xpdr::IsValidCode(const wchar_t* code) { // Has 4 numeric digits if (wcslen(code) != 4) { return false; // Code MUST be 4 digits } for (int i=0; i<4; i++) { // Check each digit is octal if (code[i] < 0x30 || code[i] > 0x37) { return false; // Code digits must be 0 - 7 } } return true; } Thanks for your thoughts on how to make this better / more efficient.
Stick^ wrote:
for (int i=0; i<4; i++) { // Check each digit is octal if (code[i] < 0x30 || code[i] > 0x37) { return false; // Code digits must be 0 - 7 } }
Have you considered
wcstol(code, ..., 8)?
"Approved Workmen Are Not Ashamed" - 2 Timothy 2:15
"Judge not by the eye but by the heart." - Native American Proverb
-
Stick^ wrote:
for (int i=0; i<4; i++) { // Check each digit is octal if (code[i] < 0x30 || code[i] > 0x37) { return false; // Code digits must be 0 - 7 } }
Have you considered
wcstol(code, ..., 8)?
"Approved Workmen Are Not Ashamed" - 2 Timothy 2:15
"Judge not by the eye but by the heart." - Native American Proverb
No, but I am thinking that function won't help me, because what I am trying to do is verify that input (a WCHAR*) typed by a user is in the range 0000 - 7777, with only octal (ie 0-7) digits. They could enter anything on their keyboard, so I need to validate it, not just convert it. Thanks guys.
-
Ah, very interesting. Nice touch verifying that there is a terminating NULL. By unroll are you meaning something like this: bool Xpdr::IsValidCode(const wchar_t * code) const { if( *code < '0' || *code > '7') { return false; } code++; if( *code < '0' || *code > '7') { return false; } code++; if( *code < '0' || *code > '7') { return false; } code++; if( *code < '0' || *code > '7') { return false; } return *code == 0; }
Stick^ wrote:
[...]By unroll are you meaning something like this[...]
Yes (actually you need one more
++:return *++code == 0). Or even like this:inline bool Xpdr::IsValidCode(const wchar_t * code) const { return code[0] >= '0' && code[0] <= '7' && code[1] >= '0' && code[1] <= '7' && code[2] >= '0' && code[2] <= '7' && code[3] >= '0' && code[3] <= '7' && code[4] == 0; } -
No, but I am thinking that function won't help me, because what I am trying to do is verify that input (a WCHAR*) typed by a user is in the range 0000 - 7777, with only octal (ie 0-7) digits. They could enter anything on their keyboard, so I need to validate it, not just convert it. Thanks guys.
Stick^ wrote:
No, but I am thinking that function won't help me, because what I am trying to do is verify that input (a WCHAR*) typed by a user is in the range 0000 - 7777...
Which is exactly what it does. If you sent it "7778", it would return an error status.
"Approved Workmen Are Not Ashamed" - 2 Timothy 2:15
"Judge not by the eye but by the heart." - Native American Proverb