patterncMinor
Converts a user inputed string into a Integer
Viewed 0 times
convertsuserintoinputedstringinteger
Problem
Just a little exercise to make me more affluent with strings and error handling, any improvements welcome!
#include
#include
#include
#include
#include
#define MAXSIZE 1024
void retreiveStringInput(char *str, size_t buffersize)
{
for (;;) {
if (fgets(str, buffersize, stdin) != NULL) {
str[strcspn(str, "\n")] = 0;
return;
}
else
printf("Touble allocating memory, Please try again...\n");
}
}
bool strHasAllDigits(char *str)
{
size_t strsize = strlen(str);
if (str[0] != 0) {
for (size_t strIndex = 0; strIndex < strsize; ++strIndex) {
if (isdigit(str[strIndex]))
continue;
else
return false;
}
return true;
}
else {
printf("String is empty...\n");
return false;
}
}
int strToInt(char str[])
{
size_t len = strlen(str);
size_t power = len - 1;
int convertedString = 0;
for (size_t strIndex = 0; strIndex < len; ++strIndex, --power) {
convertedString += ((int)(str[strIndex] - '0') * pow(10,power));
}
return convertedString;
}
int main()
{
char number[MAXSIZE];
int intNumber = 0;
for (;;) {
printf("ENTER A WHOLE NUMBER: ");
retreiveStringInput(number, MAXSIZE);
if (strHasAllDigits(number))
intNumber = strToInt(number);
else {
printf("Number must be a whole number containing no characters, please try again...\n\n");
continue;
}
printf("Your number is: %d\n", intNumber);
break;
}
printf("Press any key to continue...");
getchar();
}Solution
Nitpicks
Misspelled.
Not a critical mistake, since you used it consistently. Don't forget to change other uses when you fix it.
This suggests that there is trouble allocating memory, but this code doesn't allocate memory. It reads from input.
It also has a typo in "Trouble".
Keeping it simple
Consider
I find it easier to use positive
There is no point in a
You only used the length as part of the condition check in the loop. But we don't need it. We can just check for the end of the string directly. We know that the end of string marker is
It's true that
I changed
I don't like the single statement form of control structures. I always use the block form as being more robust against editing mistakes and a little easier to read.
Avoid unnecessary floating point operations
Consider
We don't need
We don't convert to and from a floating point type. No danger of precision losses. Everything is an integer type. We probably don't even need the explicit cast to
Simplify
Consider
Now we don't have to declare
void retreiveStringInput(char *str, size_t buffersize)Misspelled.
void retrieveStringInput(char *str, size_t buffersize)Not a critical mistake, since you used it consistently. Don't forget to change other uses when you fix it.
printf("Touble allocating memory, Please try again...\n");This suggests that there is trouble allocating memory, but this code doesn't allocate memory. It reads from input.
It also has a typo in "Trouble".
Keeping it simple
size_t strsize = strlen(str);
if (str[0] != 0) {
for (size_t strIndex = 0; strIndex < strsize; ++strIndex) {
if (isdigit(str[strIndex]))
continue;
else
return false;
}
return true;
}
else {
printf("String is empty...\n");
return false;
}Consider
if (*str == '\0') {
printf("String is empty...\n");
return false;
}
for (; *str != '\0'; ++str) {
if (!isdigit(*str)) {
return false;
}
}
return true;I find it easier to use positive
if conditions with else statements. So the if is the positive case and the else is the negative case. In this situation, we don't need an else then, as we return in the if. There is no point in a
continue when there is no statement to skip. Just let it go. It will continue automatically. You don't need an else case. You only used the length as part of the condition check in the loop. But we don't need it. We can just check for the end of the string directly. We know that the end of string marker is
'\0', so we can just check for that. It's true that
'\0' is 0, but it is more readable to write it out in my opinion. I changed
str[0] to str because I knew that I was going to be saying str in the for loop anyway. Rather than use two different things that return the same value, I picked the one that worked in both cases. It would look kind of odd to repeatedly do str[0] and act as if the values would be different. That's more understandable with *str. I don't like the single statement form of control structures. I always use the block form as being more robust against editing mistakes and a little easier to read.
Avoid unnecessary floating point operations
size_t power = len - 1;
int convertedString = 0;
for (size_t strIndex = 0; strIndex < len; ++strIndex, --power) {
convertedString += ((int)(str[strIndex] - '0') * pow(10,power));
}Consider
int convertedString = 0;
for (size_t strIndex = 0; strIndex < len; ++strIndex) {
convertedString *= 10;
convertedString += (int)(str[strIndex] - '0');
}We don't need
power anymore. We don't convert to and from a floating point type. No danger of precision losses. Everything is an integer type. We probably don't even need the explicit cast to
int. I haven't run the code, so I left it. Simplify
if (strHasAllDigits(number))
intNumber = strToInt(number);
else {
printf("Number must be a whole number containing no characters, please try again...\n\n");
continue;
}
printf("Your number is: %d\n", intNumber);
break;Consider
if (strHasAllDigits(number)) {
int intNumber = strToInt(number);
printf("Your number is: %d\n", intNumber);
break;
}
printf("Number must be a whole number containing no characters, please try again...\n\n");Now we don't have to declare
intNumber outside the loop. And we don't have to continue to skip the break.Code Snippets
void retreiveStringInput(char *str, size_t buffersize)void retrieveStringInput(char *str, size_t buffersize)printf("Touble allocating memory, Please try again...\n");size_t strsize = strlen(str);
if (str[0] != 0) {
for (size_t strIndex = 0; strIndex < strsize; ++strIndex) {
if (isdigit(str[strIndex]))
continue;
else
return false;
}
return true;
}
else {
printf("String is empty...\n");
return false;
}if (*str == '\0') {
printf("String is empty...\n");
return false;
}
for (; *str != '\0'; ++str) {
if (!isdigit(*str)) {
return false;
}
}
return true;Context
StackExchange Code Review Q#141676, answer score: 6
Revisions (0)
No revisions yet.