Skip to main content
added 6 characters in body
Source Link
Jerry Coffin
  • 33.6k
  • 4
  • 75
  • 143

Note that there's a difference in the size you specify though. With fgets, you specify the size of the buffer, but with scanf you specify the sizenumber of stringcharacters it's allowed to read, which is one less than the size of the buffer itself. Also note that fgets normally retains the \n at the end of what was entered though (if you get data without a \n on the end, it means the buffer you supplied wasn't large enough to hold all the data that was entered).

wchar_t* reversed = malloc(sizeof(*reversed) * (wcslen(str) + 1));

Note that there's a difference in the size you specify though. With fgets, you specify the size of the buffer, but with scanf you specify the size of string it's allowed to read, which is one less than the size of the buffer itself. Also note that fgets normally retains the \n at the end of what was entered though (if you get data without a \n on the end, it means the buffer you supplied wasn't large enough to hold all the data that was entered).

wchar_t* reversed = malloc(sizeof(*reversed) * wcslen(str) + 1);

Note that there's a difference in the size you specify though. With fgets, you specify the size of the buffer, but with scanf you specify the number of characters it's allowed to read, which is one less than the size of the buffer itself. Also note that fgets normally retains the \n at the end of what was entered though (if you get data without a \n on the end, it means the buffer you supplied wasn't large enough to hold all the data that was entered).

wchar_t* reversed = malloc(sizeof(*reversed) * (wcslen(str) + 1));
added 1 character in body
Source Link
Jerry Coffin
  • 33.6k
  • 4
  • 75
  • 143

Unnecessary Cast

When you allocate your memory:

char* reversed = (char*) malloc(sizeof(char) * strlen(str) + 1);

...you're currently casting the result of malloc. This is generally frowned upon by C programmers--it's unnecessary and it can cover up a bug of having failed to include the right header to declare/prototype malloc correctly.

char* reversed = malloc(sizeof(char) * strlen(str) + 1);

The cast is only necessary if you decide to write C that can also compile as C++. This tends to give the worst of both worlds, and should generally be avoided--if you're going to compile this as C++, use std::string and std::reverse.

Size Computation

When you allocate memory, you multiply the length by sizeof(char), but sizeof(char) is guaranteed to be 1, so the multiplication accomplishes nothing.

Even if you decide to leave the multiplication in (since it is necessary for types other than char), I prefer to use code like this:

char* reversed = malloc(sizeof(*reversed) * strlen(str) + 1);

This has the advantage that when/if you (for example) decide to support wide characters, you can change it to something like:

wchar_t* reversed = malloc(sizeof(*reversed) * wcslen(str) + 1);

...so the argument to sizeof doesn't need to change at all (and believe me--if you decide to do something like this, you have enough headaches to deal with, so even a small help will be welcome).

Unnecessary Cast

When you allocate your memory:

char* reversed = (char*) malloc(sizeof(char) * strlen(str) + 1);

...you're currently casting the result of malloc. This is generally frowned upon by C programmers--it's unnecessary and it can cover up a bug of having failed to include the right header to declare/prototype malloc correctly.

char* reversed = malloc(sizeof(char) * strlen(str) + 1);

The cast is only necessary if you decide to write C that can also compile as C++. This tends to give the worst of both worlds, and should generally be avoided--if you're going to compile this as C++, use std::string and std::reverse.

Size Computation

When you allocate memory, you multiply the length by sizeof(char), but sizeof(char) is guaranteed to be 1, so the multiplication accomplishes nothing.

Even if you decide to leave the multiplication in (since it is necessary for types other than char), I prefer to use code like this:

char* reversed = malloc(sizeof(*reversed) * strlen(str) + 1);

This has the advantage that when/if you (for example) decide to support wide characters, you can change it to something like:

wchar_t* reversed = malloc(sizeof(*reversed) * wcslen(str) + 1);

...so the argument to sizeof doesn't need to change at all (and believe me--if you decide to do something like this, you have enough headaches to deal with, so even a small help will be welcome).

added 1 character in body
Source Link
Jerry Coffin
  • 33.6k
  • 4
  • 75
  • 143

Memory management

Right now, your code leaks memory--every time you call reverse, it allocates some memory, and none of your other code frees it again.

If all you're doing is reversing one string, then exiting, that's of little consequence--but if you try to use this in real code, leaking memory like this is generally unacceptable.

const correctness

Since you're not modifying the input string, you might as well use const in the function's signature:

char* reverse(char const * str);

Buffer overrun protection

Right now you have:

scanf("%s", word);

This is essentially equivalent to gets(word);. That is to say, it provides absolutely no protection against the user entering a string longer than you've provided space to store. When using %s with scanf (or cousins like fscanf, sscanf, etc.) you need to specify the maximum length:

scanf(%49s""%49s", word);

Alternatively, consider using fgets, which also requires you to specify the buffer size.

Note that there's a difference in the size you specify though. With fgets, you specify the size of the buffer, but with scanf you specify the size of string it's allowed to read, which is one less than the size of the buffer itself. Also note that fgets normally retains the \n at the end of what was entered though (if you get data without a \n on the end, it means the buffer you supplied wasn't large enough to hold all the data that was entered).

Memory management

Right now, your code leaks memory--every time you call reverse, it allocates some memory, and none of your other code frees it again.

If all you're doing is reversing one string, then exiting, that's of little consequence--but if you try to use this in real code, leaking memory like this is generally unacceptable.

const correctness

Since you're not modifying the input string, you might as well use const in the function's signature:

char* reverse(char const * str);

Buffer overrun protection

Right now you have:

scanf("%s", word);

This is essentially equivalent to gets(word);. That is to say, it provides absolutely no protection against the user entering a string longer than you've provided space to store. When using %s with scanf (or cousins like fscanf, sscanf, etc.) you need to specify the maximum length:

scanf(%49s", word);

Alternatively, consider using fgets, which also requires you to specify the buffer size.

Note that there's a difference in the size you specify though. With fgets, you specify the size of the buffer, but with scanf you specify the size of string it's allowed to read, which is one less than the size of the buffer itself. Also note that fgets normally retains the \n at the end of what was entered though (if you get data without a \n on the end, it means the buffer you supplied wasn't large enough to hold all the data that was entered).

Memory management

Right now, your code leaks memory--every time you call reverse, it allocates some memory, and none of your other code frees it again.

If all you're doing is reversing one string, then exiting, that's of little consequence--but if you try to use this in real code, leaking memory like this is generally unacceptable.

const correctness

Since you're not modifying the input string, you might as well use const in the function's signature:

char* reverse(char const * str);

Buffer overrun protection

Right now you have:

scanf("%s", word);

This is essentially equivalent to gets(word);. That is to say, it provides absolutely no protection against the user entering a string longer than you've provided space to store. When using %s with scanf (or cousins like fscanf, sscanf, etc.) you need to specify the maximum length:

scanf("%49s", word);

Alternatively, consider using fgets, which also requires you to specify the buffer size.

Note that there's a difference in the size you specify though. With fgets, you specify the size of the buffer, but with scanf you specify the size of string it's allowed to read, which is one less than the size of the buffer itself. Also note that fgets normally retains the \n at the end of what was entered though (if you get data without a \n on the end, it means the buffer you supplied wasn't large enough to hold all the data that was entered).

Source Link
Jerry Coffin
  • 33.6k
  • 4
  • 75
  • 143
Loading