Skip to main content
updated info on null term and corrected some variable names
Source Link
UpAndAdam
  • 1.7k
  • 1
  • 3
  • 20
  1. Your is_digit function is fundamentally broken. Even though there is a loop in place to try to check all the characters in the input string it will only check the first character in the string for being a digit and then it returns immediately. See if you can figure out the problem here. For example, it would think the string "1FOO" is only digits, but it is not. See if you can figure this out yourself.

  2. You need to do something with the returned result from your call to only_digits.

    • Its signature specifies it returns a bool which is correct. However if it's returning a bool you should really return true or false to be proper instead of 0 and 1.
    • That aside, I would stick to having cohesive functions of singular responsibility. Let that function only tell you if there are only digits or not. Move the business logic ( printing ) back into main.
    • In the main function what do you need to do if only_digits indicates that the key is not "only_digits" so that you exit the program? Currently you do not examine the result of the function so flow continues to the next line of code.
  3. Minor Issue

    • You have an issue with the size you use for cipher_textciphertext; it is incorrect. You need to remember that strlen function doesn't give you the size you need to store the string, it gives you the length of the string, you need room for a null terminator. You should make the size text_length + 1 instead and remember to set the null terminator at cipher_text[text_length]ciphertext[text_length] either before or after your population of cipher_textciphertext. If you don't do this, you will run into undefined value issues and your code will behave inconsistently. That's bad. Here I am spelling out a fix for you. UPDATE to set the null terminator you just have to say ciphertext[text_length] = 0; or ciphertext[text_length] = '\0';
  4. Problem with your construction of cipher_textciphertext

  • Your rotate function, takes in a char and rotation number as input. Because you pass the input char by value it does not alter plain_textplaintext. As a result in the next line when you say cipher_text[i]ciphertext[i] = plain_text[i]plaintext[i] effectively nothing happens, it just copies over the original value of plain_text[i]plaintext[i]. The return of your call to rotate is not used and discarded, but it does contain the correctly rotated letter, what could you do to make use of it?
  1. Side Notes
    • I would suggest not declaring cipher_textciphertext the way you have; yes it is valid in C99 but in general that is not C style. If you don't know array size at compile time from a constant you should generally use dynamic allocation. ( which means a call to malloc and free ) Yes C99 lets you do this but you should get used to that idiom.
    • No reason to create variable j, just use text_length
    • You should avoid using 'magic' numbers and make them proper constants instead. Namely in your rotate function, yes I know why you are using 65 and 97 but you should make them constants. Or see if you can figure out a way to do this without having to know the values 65 or 97 directly. Even 26 you should probably make a constant but that number you is reasonable for you to 'know' so to speak.
  1. Your is_digit function is fundamentally broken. Even though there is a loop in place to try to check all the characters in the input string it will only check the first character in the string for being a digit and then it returns immediately. See if you can figure out the problem here. For example, it would think the string "1FOO" is only digits, but it is not. See if you can figure this out yourself.

  2. You need to do something with the returned result from your call to only_digits.

    • Its signature specifies it returns a bool which is correct. However if it's returning a bool you should really return true or false to be proper instead of 0 and 1.
    • That aside, I would stick to having cohesive functions of singular responsibility. Let that function only tell you if there are only digits or not. Move the business logic ( printing ) back into main.
    • In the main function what do you need to do if only_digits indicates that the key is not "only_digits" so that you exit the program? Currently you do not examine the result of the function so flow continues to the next line of code.
  3. Minor Issue

    • You have an issue with the size you use for cipher_text; it is incorrect. You need to remember that strlen function doesn't give you the size you need to store the string, it gives you the length of the string, you need room for a null terminator. You should make the size text_length + 1 instead and remember to set the null terminator at cipher_text[text_length] either before or after your population of cipher_text. If you don't do this, you will run into undefined value issues and your code will behave inconsistently. That's bad. Here I am spelling out a fix for you.
  4. Problem with your construction of cipher_text

  • Your rotate function, takes in a char and rotation number as input. Because you pass the input char by value it does not alter plain_text. As a result in the next line when you say cipher_text[i] = plain_text[i] effectively nothing happens, it just copies over the original value of plain_text[i]. The return of your call to rotate is not used and discarded, but it does contain the correctly rotated letter, what could you do to make use of it?
  1. Side Notes
    • I would suggest not declaring cipher_text the way you have; yes it is valid in C99 but in general that is not C style. If you don't know array size at compile time from a constant you should generally use dynamic allocation. ( which means a call to malloc and free ) Yes C99 lets you do this but you should get used to that idiom.
    • No reason to create variable j, just use text_length
    • You should avoid using 'magic' numbers and make them proper constants instead. Namely in your rotate function, yes I know why you are using 65 and 97 but you should make them constants. Or see if you can figure out a way to do this without having to know the values 65 or 97 directly. Even 26 you should probably make a constant but that number you is reasonable for you to 'know' so to speak.
  1. Your is_digit function is fundamentally broken. Even though there is a loop in place to try to check all the characters in the input string it will only check the first character in the string for being a digit and then it returns immediately. See if you can figure out the problem here. For example, it would think the string "1FOO" is only digits, but it is not. See if you can figure this out yourself.

  2. You need to do something with the returned result from your call to only_digits.

    • Its signature specifies it returns a bool which is correct. However if it's returning a bool you should really return true or false to be proper instead of 0 and 1.
    • That aside, I would stick to having cohesive functions of singular responsibility. Let that function only tell you if there are only digits or not. Move the business logic ( printing ) back into main.
    • In the main function what do you need to do if only_digits indicates that the key is not "only_digits" so that you exit the program? Currently you do not examine the result of the function so flow continues to the next line of code.
  3. Minor Issue

    • You have an issue with the size you use for ciphertext; it is incorrect. You need to remember that strlen function doesn't give you the size you need to store the string, it gives you the length of the string, you need room for a null terminator. You should make the size text_length + 1 instead and remember to set the null terminator at ciphertext[text_length] either before or after your population of ciphertext. If you don't do this, you will run into undefined value issues and your code will behave inconsistently. That's bad. Here I am spelling out a fix for you. UPDATE to set the null terminator you just have to say ciphertext[text_length] = 0; or ciphertext[text_length] = '\0';
  4. Problem with your construction of ciphertext

  • Your rotate function, takes in a char and rotation number as input. Because you pass the input char by value it does not alter plaintext. As a result in the next line when you say ciphertext[i] = plaintext[i] effectively nothing happens, it just copies over the original value of plaintext[i]. The return of your call to rotate is not used and discarded, but it does contain the correctly rotated letter, what could you do to make use of it?
  1. Side Notes
    • I would suggest not declaring ciphertext the way you have; yes it is valid in C99 but in general that is not C style. If you don't know array size at compile time from a constant you should generally use dynamic allocation. ( which means a call to malloc and free ) Yes C99 lets you do this but you should get used to that idiom.
    • No reason to create variable j, just use text_length
    • You should avoid using 'magic' numbers and make them proper constants instead. Namely in your rotate function, yes I know why you are using 65 and 97 but you should make them constants. Or see if you can figure out a way to do this without having to know the values 65 or 97 directly. Even 26 you should probably make a constant but that number you is reasonable for you to 'know' so to speak.
added 374 characters in body
Source Link
UpAndAdam
  • 1.7k
  • 1
  • 3
  • 20

I'm mostly not going to spell out exact solutions, but leave you with hints that should enable you to solve it. Ask follow ups in comment if you need more.

  1. You need to do something with the returned result from your call to only_digits. It returns a bool which is correct, but if its returning a bool you should return true or false to be proper instead of 0 and 1. That aside, I would stick to having cohesive functions of singular responsibility. Let that function only tell you if there are only digits or not. Move the business logic ( printing ) back into main. Back to my main point, you need to check the result of the call and if the result is false you need to exit the program (return 1 possibly). Because you don't do this the only thing the call achieves is a printout, and then the code continues ostensibly into an infinite loop somewhere.

    Your is_digit function is fundamentally broken. Even though there is a loop in place to try to check all the characters in the input string it will only check the first character in the string for being a digit and then it returns immediately. See if you can figure out the problem here. For example, it would think the string "1FOO" is only digits, but it is not. See if you can figure this out yourself.

    For example:
  2. You need to do something with the returned result from your call to only_digits.

    • Its signature specifies it returns a bool which is correct. However if it's returning a bool you should really return true or false to be proper instead of 0 and 1.
    • That aside, I would stick to having cohesive functions of singular responsibility. Let that function only tell you if there are only digits or not. Move the business logic ( printing ) back into main.
    • In the main function what do you need to do if only_digits indicates that the key is not "only_digits" so that you exit the program? Currently you do not examine the result of the function so flow continues to the next line of code.
  3. Minor Issue

    • You have an issue with the size you use for cipher_text; it is incorrect. You need to remember that strlen function doesn't give you the size you need to store the string, it gives you the length of the string, you need room for a null terminator. You should make the size text_length + 1 instead and remember to set the null terminator at cipher_text[text_length] either before or after your population of cipher_text. If you don't do this, you will run into undefined value issues and your code will behave inconsistently. That's bad. Here I am spelling out a fix for you.
  4. Problem with your construction of cipher_text

if (!only_digits(argv[1]))
{
    printf("Usage: ./caesar key\n"); //suggest pulling this from only digits and keeping in main
    return 1;
}

// rest of your code 
  • Your rotate function, takes in a char and rotation number as input. Because you pass the input char by value it does not alter plain_text. As a result in the next line when you say cipher_text[i] = plain_text[i] effectively nothing happens, it just copies over the original value of plain_text[i]. The return of your call to rotate is not used and discarded, but it does contain the correctly rotated letter, what could you do to make use of it?
  1. Minor Issue

    • You have an issue with the size you use; it is incorrect. You need to remember that strlen function doesn't give you the exact size you need to store the string, it gives you the length of the string, you need room for a null terminator. You should make the size text_length + 1 instead and remember to set the null terminator at cipher_text[text_length] either before or after your population of cipher_text
  2. Major problem
    Your rotate function, takes in a char and rotation number as input. Because you pass the input char by value it does not alter plain_text. As a result in the next line when you say cipher_text[i] = plain_text[i] effectively nothing happens, it just copies over the original value of plain_text[i]. You just have a string copy with extra work. Once again you need to do something with your return value. What you should be doing is assigning cipher_text[i] = rotate(plain_text[i], key); so that the returned rotated character is assigned into cipher_text.
    There are many alternatives to this, but for your rotate signature this is what you need to do.

  3. Side Notes

    • I would suggest not declaring cipher_text the way you have; yes it is valid in C99 but in general that is not C style. If you don't know array size at compile time from a constant you should generally use dynamic allocation. ( which means a call to malloc and free ) Yes C99 lets you do this but you should get used to that idiom.
    • No reason to create variable j, just use text_length
    • You should avoid using 'magic' numbers and make them proper constants instead. Namely in your rotate function, yes I know why you are using 65 and 97 but you should make them constants. Or see if you can figure out a way to do this without having to know the values 65 or 97 directly. Even 26 you should probably make a constant but that number you is reasonable for you to 'know' so to speak.
  1. Side Notes
    • I would suggest not declaring cipher_text the way you have; yes it is valid in C99 but in general that is not C style. If you don't know array size at compile time from a constant you should generally use dynamic allocation. ( which means a call to malloc and free ) Yes C99 lets you do this but you should get used to that idiom.
    • No reason to create variable j, just use text_length
    • You should avoid using 'magic' numbers and make them proper constants instead. Namely in your rotate function, yes I know why you are using 65 and 97 but you should make them constants. Or see if you can figure out a way to do this without having to know the values 65 or 97 directly. Even 26 you should probably make a constant but that number you is reasonable for you to 'know' so to speak.
  1. You need to do something with the returned result from your call to only_digits. It returns a bool which is correct, but if its returning a bool you should return true or false to be proper instead of 0 and 1. That aside, I would stick to having cohesive functions of singular responsibility. Let that function only tell you if there are only digits or not. Move the business logic ( printing ) back into main. Back to my main point, you need to check the result of the call and if the result is false you need to exit the program (return 1 possibly). Because you don't do this the only thing the call achieves is a printout, and then the code continues ostensibly into an infinite loop somewhere. For example:
if (!only_digits(argv[1]))
{
    printf("Usage: ./caesar key\n"); //suggest pulling this from only digits and keeping in main
    return 1;
}

// rest of your code 
  1. Minor Issue

    • You have an issue with the size you use; it is incorrect. You need to remember that strlen function doesn't give you the exact size you need to store the string, it gives you the length of the string, you need room for a null terminator. You should make the size text_length + 1 instead and remember to set the null terminator at cipher_text[text_length] either before or after your population of cipher_text
  2. Major problem
    Your rotate function, takes in a char and rotation number as input. Because you pass the input char by value it does not alter plain_text. As a result in the next line when you say cipher_text[i] = plain_text[i] effectively nothing happens, it just copies over the original value of plain_text[i]. You just have a string copy with extra work. Once again you need to do something with your return value. What you should be doing is assigning cipher_text[i] = rotate(plain_text[i], key); so that the returned rotated character is assigned into cipher_text.
    There are many alternatives to this, but for your rotate signature this is what you need to do.

  3. Side Notes

    • I would suggest not declaring cipher_text the way you have; yes it is valid in C99 but in general that is not C style. If you don't know array size at compile time from a constant you should generally use dynamic allocation. ( which means a call to malloc and free ) Yes C99 lets you do this but you should get used to that idiom.
    • No reason to create variable j, just use text_length
    • You should avoid using 'magic' numbers and make them proper constants instead. Namely in your rotate function, yes I know why you are using 65 and 97 but you should make them constants. Or see if you can figure out a way to do this without having to know the values 65 or 97 directly. Even 26 you should probably make a constant but that number you is reasonable for you to 'know' so to speak.

I'm mostly not going to spell out exact solutions, but leave you with hints that should enable you to solve it. Ask follow ups in comment if you need more.

  1. Your is_digit function is fundamentally broken. Even though there is a loop in place to try to check all the characters in the input string it will only check the first character in the string for being a digit and then it returns immediately. See if you can figure out the problem here. For example, it would think the string "1FOO" is only digits, but it is not. See if you can figure this out yourself.

  2. You need to do something with the returned result from your call to only_digits.

    • Its signature specifies it returns a bool which is correct. However if it's returning a bool you should really return true or false to be proper instead of 0 and 1.
    • That aside, I would stick to having cohesive functions of singular responsibility. Let that function only tell you if there are only digits or not. Move the business logic ( printing ) back into main.
    • In the main function what do you need to do if only_digits indicates that the key is not "only_digits" so that you exit the program? Currently you do not examine the result of the function so flow continues to the next line of code.
  3. Minor Issue

    • You have an issue with the size you use for cipher_text; it is incorrect. You need to remember that strlen function doesn't give you the size you need to store the string, it gives you the length of the string, you need room for a null terminator. You should make the size text_length + 1 instead and remember to set the null terminator at cipher_text[text_length] either before or after your population of cipher_text. If you don't do this, you will run into undefined value issues and your code will behave inconsistently. That's bad. Here I am spelling out a fix for you.
  4. Problem with your construction of cipher_text

  • Your rotate function, takes in a char and rotation number as input. Because you pass the input char by value it does not alter plain_text. As a result in the next line when you say cipher_text[i] = plain_text[i] effectively nothing happens, it just copies over the original value of plain_text[i]. The return of your call to rotate is not used and discarded, but it does contain the correctly rotated letter, what could you do to make use of it?
  1. Side Notes
    • I would suggest not declaring cipher_text the way you have; yes it is valid in C99 but in general that is not C style. If you don't know array size at compile time from a constant you should generally use dynamic allocation. ( which means a call to malloc and free ) Yes C99 lets you do this but you should get used to that idiom.
    • No reason to create variable j, just use text_length
    • You should avoid using 'magic' numbers and make them proper constants instead. Namely in your rotate function, yes I know why you are using 65 and 97 but you should make them constants. Or see if you can figure out a way to do this without having to know the values 65 or 97 directly. Even 26 you should probably make a constant but that number you is reasonable for you to 'know' so to speak.
Source Link
UpAndAdam
  • 1.7k
  • 1
  • 3
  • 20

Overall the common theme is that you need to remember to do something with the return values from your function calls that return a value.

  1. You need to do something with the returned result from your call to only_digits. It returns a bool which is correct, but if its returning a bool you should return true or false to be proper instead of 0 and 1. That aside, I would stick to having cohesive functions of singular responsibility. Let that function only tell you if there are only digits or not. Move the business logic ( printing ) back into main. Back to my main point, you need to check the result of the call and if the result is false you need to exit the program (return 1 possibly). Because you don't do this the only thing the call achieves is a printout, and then the code continues ostensibly into an infinite loop somewhere. For example:
if (!only_digits(argv[1]))
{
    printf("Usage: ./caesar key\n"); //suggest pulling this from only digits and keeping in main
    return 1;
}

// rest of your code 
  1. Minor Issue

    • You have an issue with the size you use; it is incorrect. You need to remember that strlen function doesn't give you the exact size you need to store the string, it gives you the length of the string, you need room for a null terminator. You should make the size text_length + 1 instead and remember to set the null terminator at cipher_text[text_length] either before or after your population of cipher_text
  2. Major problem
    Your rotate function, takes in a char and rotation number as input. Because you pass the input char by value it does not alter plain_text. As a result in the next line when you say cipher_text[i] = plain_text[i] effectively nothing happens, it just copies over the original value of plain_text[i]. You just have a string copy with extra work. Once again you need to do something with your return value. What you should be doing is assigning cipher_text[i] = rotate(plain_text[i], key); so that the returned rotated character is assigned into cipher_text.
    There are many alternatives to this, but for your rotate signature this is what you need to do.

  3. Side Notes

    • I would suggest not declaring cipher_text the way you have; yes it is valid in C99 but in general that is not C style. If you don't know array size at compile time from a constant you should generally use dynamic allocation. ( which means a call to malloc and free ) Yes C99 lets you do this but you should get used to that idiom.
    • No reason to create variable j, just use text_length
    • You should avoid using 'magic' numbers and make them proper constants instead. Namely in your rotate function, yes I know why you are using 65 and 97 but you should make them constants. Or see if you can figure out a way to do this without having to know the values 65 or 97 directly. Even 26 you should probably make a constant but that number you is reasonable for you to 'know' so to speak.