0
\$\begingroup\$

I've just completed an exercise in which I had to create a program that searches for a string within another bigger string.

If the substring exists then it is outputted to confirm and the starting index is given where the first character can be found.

Aside from printf and scanf, I am not allowed to use any other functions unless I write them myself.

My code works, however I'm concerned that I've written far more code than is necessary. WITHOUT using pointers (something I'll learn next), could I have considerably cut down the amount of code I wrote?

#include <stdio.h>
#include <stdbool.h>

int findString(char mom[], char babe[]);
int stringLength(const char string[]);
bool equalStrings(const char s1[], const char s2[]);

int main(void)
{
    char mommy[80];
    char baby[80];
    int index;

    printf("Which string do you want to search?\n");
    scanf("%s", mommy);

    printf("What do you want to search for?\n");
    scanf("%s", baby);

    index = findString(mommy, baby);

    if(index >= 0)    
        printf("Yup, baby was found in mommy at index %i\n", index);
    else
        printf("Sorry, baby not found\n");

    return 0;
}

int findString(char mom[], char babe[])
{
    int index, i, j;
    int momLength = stringLength(mom);
    int babeLength = stringLength(babe);
    char test[babeLength + 1];

    for(i = 0; i <= babeLength-1; i++)
    {
        for(j = 0; j <= momLength; j++)
        {
            if(babe[i] == mom[j])
            {
                test[i] = babe[i];

                if(babe[0] == mom[j])
                    index = j;
            }
        }
    }

    test[babeLength] = '\0';

    if(equalStrings(test, babe) == true)
        return index;
    else
        return -1;
}

int stringLength(const char string[])
{
    int count = 0;

    while(string[count] != '\0')
        count++;

    return count;
}

bool equalStrings(const char s1[], const char s2[])
{
    int i = 0;
    bool areEqual;

    while(s1[i] == s2[i] && s1[i] != '\0'){
        i++;
        if(s1[i] == '\0' && s2[i] == '\0')
        {
            areEqual = true;
        }
        else
        {
            areEqual = false;
        }
    }
    return areEqual;
}
\$\endgroup\$

closed as off-topic by forsvarir, syb0rg, Dannnno, Pimgd, RobH Jul 26 '16 at 9:52

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – forsvarir, syb0rg, Dannnno, Pimgd, RobH
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 4
    \$\begingroup\$ strstr() source code can be found online, I'd give that a look. \$\endgroup\$ – syb0rg Jul 25 '16 at 12:12
  • 2
    \$\begingroup\$ @syb0rg "Aside from printf and scanf, I am not allowed to use any other functions unless I write them myself." \$\endgroup\$ – Pimgd Jul 25 '16 at 12:13
  • 1
    \$\begingroup\$ I'm pretty sure this code doesn't work the way you're expecting it too. Consider "mommy='abcdefg'" and "baby='fed'". It returns match. You're basically searching to see if all of the letters within baby exist within mommy. The order doesn't matter. \$\endgroup\$ – forsvarir Jul 25 '16 at 12:45
  • \$\begingroup\$ Yes I've JUST now realized this and it is distressing me considerably :( \$\endgroup\$ – gloopit Jul 25 '16 at 12:47
  • 1
    \$\begingroup\$ In references that describe this interface which @syb0rg suggested, the variable names are generally needle and haystack, as in "find the needle in the haystack". Try to improve your variable names so that if you or someone else needs to maintain the code the names mean something. \$\endgroup\$ – pacmaninbw Jul 25 '16 at 13:03
3
\$\begingroup\$

Sorry but your code is an accident waiting to happen.

  1. Using a constant to declare the available space in you char arrays allows your code to be more maintainable.
  2. scanf() can overflow your buffer and corrupt everything after your buffer. Forcibly set the BufferSize-1 character to NULL after you have run scanf() its the best you can do with the limitations you have be given.
  3. Why are you declaring index so far up the code, always declare a variable as near to the point of first use as you can, also it could be const, which will make it clear to the reader that its value will never change.
  4. Unless you compiler won't allow it declare loop variables inside the for statement i.e. for (int i = 0; i < momLength; ++i). The ++i makes no difference for simple types, but when you move on to iterators it does.
  5. Always try and use meaningful variable names, i and j are not very meaningful. You might read this again in 12 months, so make it easy for yourself.
  6. Now the bad news, I think you algorithm is wrong. Why are you creating a copy of a string (test)? I think it would be easier to scan through mum until you find the first character of babe (index). Then match every character of babe to the characters of mom. If you get to the end of babe then you have a match starting at index, if a character doesn't match then start scanning mum again at index + 1.
  7. You need to check for zero length mom and babe values entered by the user.
  8. stringLength() could go on "infinitely" As it stands if you scan a value which overflows the buffer then you could keep going until you run out of memory. You need to impose a maximum length of BufferSize.
  9. And finally, stick some comments in, it will make it easier when you are getting marked and you'll get a better score.
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.