0
char *substring(char *string, int index, int length)
{
    //int counter = length - index;
    int counter = 0;
    for(;string[index] != string[length];index++, counter++);
    printf("\n%d\n", counter);
    char *array = malloc(sizeof(char) * counter);
    if(array != NULL)
    {
        ///while(string[index] != string[length])
        while(index != length)
            {
                array[index] = string[index];
                index++;
                array++;
            }
    }
    else
        puts("Dynamic allocations failed\n");
    return array;
}   

1 - I've commented out initializing counter with "length - index" because I didn't feel comfortable with it(I also kinda like one line for loops:) ). So, can I count on counter if I used it in this simpler way.

2 - My problem with this code is that It doesn't return anything. I try to printf the result but nothing is printed and when I assign the result of the function to a char *, I get an error saying that I cannot assign a void to char *. But how is it returning void?

3 - Can I write this function with pointer arithmetic and without any array indexing at all?!

4 - Can I mutate the char *array ?!. I'm asking this because I thought char * cannot be mutated, but I've read a code that correctly mutated a char *. Or is it that I'm confusing a regular char * and a string?

Note: I do not want to use string library functions

6
  • string[index] != string[length] what purpose does dis actually server ? Commented Apr 26, 2014 at 17:55
  • How about memcpy? or strncpy? Not sure if those count to you as "string library functions" though they do exactly what you want and generally will be faster. Commented Apr 26, 2014 at 17:57
  • @GoldRoger: It serves the same purpose as the while statement after it. But since the second while loop works just fine and simpler, I've commented it out. Commented Apr 26, 2014 at 18:02
  • @Thomas: It still a library function. I haven't got to studying the library functions properly. That's why I do not want to use them. Commented Apr 26, 2014 at 18:03
  • Posted a working example that doesn't use ANY libraries but malloc and free. Commented Apr 26, 2014 at 18:04

3 Answers 3

3

There are several problems with your code.

for(;string[index] != string[length];index++, counter++);

First, in the above line, you need to subtract one from length since arrays are 0-indexed.

This line will also break if the last character of the substring is repeated somewhere else in the substring. For example aba where index = 0, length = 3. Your loop will immediately stop since string[0] == string[2] even though you did not reach the end of the substring.

However, the entire loop for counting substring length is unnecessary, just use length!

char* array = malloc(length + 1);

Note that you need +1 to include the null terminator that is standard in C-strings.

Next, this line won't work if index is not 0.

array[index] = string[index];

index is the index into string, not the index into array. You should use a different variable for your iterator that starts at index and then increments. Then you can subtract index from the iterator to get the actual index in the substring array.

int i = index;
while(i < length)
{
    array[i - index] = string[i];
}

Also note that this line in the loop is unnecessary and breaks your code.

    array++;

Your iterator is being incremented so you have no need to increment the array pointer. Also, if you increment array directly, then when you return it, where is it pointing? It's pointing to the end of the string, not the beginning, so of course you will get no output.

Finally, don't forget to add the null terminator. After your loop, since your iterator will now conveniently point to the last index, just do

array[i] = '\0';

As extra, since you are specifically creating a new string to hold the substring and not modifying the original string pointer, you should declare the argument for string as const. Ex: const char *string. This is not required however there are a number of reasons that const-correctness is important.

If you make the above changes your code should work just fine. I am not posting completed code as I think making the changes yourself is a valuable exercise.

Sign up to request clarification or add additional context in comments.

7 Comments

I think you've misunderstood the purpose of length; I'm fairly sure it is the length of the substring, not of the string from which the substring is extracted. I agree there are multiple problems, but your analysis of the problems in the for loop is not correct.
That's beautiful. I love answers that corrects my mistakes and misunderstanding line by line and explains where I went wrong. Thank you so much Daniel
To clarify, Length is the length of the substring
No problem, I'm glad it helped. Be sure to read my edit about using a null terminator though. That is a very important part that I initially forgot.
Correct, that is what I assumed from the name you gave it.
|
1

1 - You can use the for loop, but it is a slow way. Simple subtraction does the same job instantly, I see no reason not to feel comfortable with it.

2 - Printf - remove the semicolon at the end of the for loop, about the pointer:

void * is an universal pointer type. You should cast it to char *, like so:

char *array = (char *)malloc(sizeof(char) * counter);

3 - Yes, *(ptr + 3) is equivalent to ptr[3]. In most implementations it is a bit faster to use pointers as well.

4 - Yes, you can modify the memory pointed by a char * pointer. It would modify the original string though, I'm not sure if that's your goal.

7 Comments

In regards to #2, I would argue the cast has more cons than pros.
How else do you want to dynamically allocate memory in standard C? You need to make a cast at one point or another. Also, this is exactly why void * has been introduced.
Yes I did. Still, a warning about types mismatch was mentioned in the question. You can of course tell him to ignore it (which is way worse practice than casting it in my opinion) or tell him to disable some warnings (even worse).
The warning he got about type mismatch was not from malloc(). Even with -Wall, assigning a void* to a char* produces no warnings or error messages. Try it yourself.
@Kelm: that's a C++ error message, not a C error message. In C, the compiler cannot legitimately complain about the absence of a cast from void * to an 'anything else pointer', but a C++ compiler cannot legitimately accept such a conversion without a cast. If you compile C code as C++, you need the casts — but pure C does not.
|
0

Worked 100%

    #include "stdafx.h"
    #include "stdlib.h"
    void substring(char * src, char * dst, int index, int length);
    int _tmain(int argc, _TCHAR* argv[])
    {
        char orig[]="test";
        char * newchar = (char*)malloc(2);//1 char + null

        substring(orig, newchar, 1, 1);

        printf(newchar);

        free(newchar);
        system("pause");
        return 0;
    }

void substring(char * src, char * dst, int index, int length)
{
    // Assign src_index to our initial index
    // Assign a new counter for dst position
    // We want only up to the length hence index+length
    // Increment src_index and dst_index
    for(int src_index = index, dst_index = 0; src_index < index + length; src_index ++, dst_index++)
    {
        dst[dst_index] = src[src_index];
    }
    dst[length++] = '\0'; // Null terminate the string, we already accounted for this above.
}

4 Comments

It wasn't me but perhaps it was lack of explanation. The OP probably wants his questions answered or explanations on what he did wrong, not completely different code written out for him by someone else.
That is understandable, but I don't think it deserves a downvote, a comment sure, but since it does TECHNICALLY answer the question in a concrete way...thats not fair on my end.
Not my down-vote, but you redesigned the interface, and you should probably use printf("%s\n", newchar) in the _tmain(). It would be preferable if you used standard C rather than Microsoft C, but that's not a showstopper. And you should certainly discuss the significant changes you made.
It was just to get the data out thats all.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.