Re: Another Understanding Pointers Question
From: Darrell Grainger (darrell_at_NOMORESPAMcs.utoronto.ca.com)
Date: 05/10/04
- Next message: Darrell Grainger: "Re: File name gets truncated"
- Previous message: Case: "Re: memcpy() and endianness"
- In reply to: Materialised: "Another Understanding Pointers Question"
- Next in thread: Materialised: "Re: Another Understanding Pointers Question"
- Reply: Materialised: "Re: Another Understanding Pointers Question"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Date: 10 May 2004 15:18:35 GMT
On Mon, 10 May 2004, Materialised wrote:
> Hi everyone,
> I seen the post by Rob Morris, and thought that I would double check
> that I was using pointers in the correct way. So I written the following
> string functions to test. I know soem can be iumplimented using the
> standard libary, but I just wanted to test writing my own functions.
> They work ok, but I would like some feed back on any issues you can see
> with them etc
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
> char *left(char *string, int count)
> {
> char *p;
> int i;
It is a good habit to check your inputs. What if string == NULL? what if
count > strlen(string)? You can actually check this as you copy the string
over to p.
> p = malloc((count * sizeof(char)+1));
The value of sizeof(char) is always 1. This can be simplified to:
p = malloc(count+1);
> if(!p){
> printf("Cannot allocate memory\n");
> exit(1);
> }
I would be less likely to use a function that exits on failure. You have
taken away all control from the calling function. What if I had created a
lot of data then called your function? I'd be very annoyed to lose all
that data. If you returned NULL instead then I have the option to
continue, save and quit or just quit.
> for(i = 0; i <= count -1; i++) {
I just like to see:
for(i = 0; i < count; i++) {
it seems more natural. Additionally, what if count > strlen(string)? The
p[i] is safe but is the string[i]?
> p[i] = string[i];
> }
> p[i++] = '\0';
Why increment i? This could have just as easily been:
p[i] = '\0';
> return(p);
> }
As far as pointer usage goes, this to good. Some comments on the code
would be good. When I saw the count+1 in the malloc I was wondering why
the +1. I assumed it was for the null character but without comments I
couldn't be sure. As someone using your function I'd have to read and
understand the code to be sure I was using it correctly. Having a
commented at the top explaining usage would be good.
> char *right(char *string, int count)
> {
> char *p;
> int len, i, j = 0;
Same as previous function. Check your inputs.
> p = malloc((count * sizeof(char)+1));
Same as previous function.
> if(!p){
> printf("Cannot allocate memory\n");
> exit(1);
> }
Same as previous function.
> len = strlen(string);
> for(i = (len - count); i <= len; i++){
> p[j] = string[i];
> j++;
> }
Good. Might be better using:
for(j = 0; i = (len - count); j < count; i++, j++)
p[j] = string[i];
First, I initialize j here. This way I'm sure I initialized it and no code
between the top of the function and here changed it.
Second, using the j index to exit the loop just seems right to me. This is
just a personal preference though.
Third, use the comma operator and make the code a little shorter. I like
to put as much on one screen as I can without losing readability.
Finally, we are sure that p[j] is safe. What about string[i]? You might
have an array out of bounds there as well.
> p[j++] = '\0';
Again, why increment j?
> return(p);
> }
All-in-all, a good use of pointers.
> char *chreplace(char *string, int count, char rep)
> {
> char *p;
Check the inputs.
> p = malloc((sizeof(string)+1));
First serious mistake. The sizeof(string) will be the sizeof(char*). You
don't want the size of a pointer. You want the size of the string being
pointed to. This should be:
p = malloc(strlen(string)+1);
> if(!p){
> printf("Cannot allocate memory\n");
> exit(1);
> }
Same thing as previous functions.
> count--;
Not immediately obvious why you are decrementing count. I can read your
code and figure out that the count parameter is 1-indexed and you are
decrementing it to make it 0-indexed but I should have to read your code
to figure that out.
> strcpy(p, string);
> p[count] = rep;
You could actually combine two lines and have:
p[--count] = rep;
> return(p);
> }
I like the fact that even of the user passing a literal string this
function still works.
> char *section(char *string, int from, int to)
> {
> char *p;
> int i, j = 0;
Check the inputs.
> p = malloc(((to - from) * sizeof(char)+1));
The sizeof(char) is redundant. What if to-from < 0? It could happen. Check
the inputs.
> if(!p){
> printf("Cannot allocate memory\n");
> exit(1);
> }
Same as previous functions.
> for( i = from; i <= to; i++) {
> p[j] = string[i];
> j++;
> }
for(j = 0, i = from; i <= to; i++, j++)
p[j] = string[i];
> p[j++] = '\0';
Why increment j?
> return(p);
> }
Not bad but still had one serious mistake.
> int main(void)
> {
> char blah[] = "abcdefghijklm";
> char *test;
> char *test2;
> char *test3;
> char *test4;
>
> test = left(blah, 10);
> test2 = right(blah, 10);
> test3 = chreplace(blah, 2, 'Q');
> test4 = section(blah, 4, 10);
> puts(test);
> puts(test2);
> puts(test3);
> puts(test4);
Lots of allocating of memory in the functions but absolutely not call to
free(). That would be a memory leak.
free(test4);
free(test3);
free(test2);
free(test);
> return 0;
> }
> Comments and improvements are welcome, flames to if appropriate.
> --
> ------
> Materialised
>
> perl -e 'printf "%silto%c%sck%ccodegurus%corg%c", "ma", 58, "mi", 64,
> 46, 10;'
>
-- Send e-mail to: darrell at cs dot toronto dot edu Don't send e-mail to vice.president@whitehouse.gov
- Next message: Darrell Grainger: "Re: File name gets truncated"
- Previous message: Case: "Re: memcpy() and endianness"
- In reply to: Materialised: "Another Understanding Pointers Question"
- Next in thread: Materialised: "Re: Another Understanding Pointers Question"
- Reply: Materialised: "Re: Another Understanding Pointers Question"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Relevant Pages
|
