c code emulating grep

Question

    #include<stdio.h>
    #include<stdlib.h>
    #include<string.h>
    #include<ctype.h>
    #define MAX_FILE_LENGTH  500

    // Gets the next line from the text file, returns the pointer to that line
    char* getLine(char* loc, FILE* fileStream);
    // Returns whether the given string contains the specified search pattern
    int checkStringMatch(char* toSearch, char* pattern);
    // Returns a lowercase copy of the string
    char* switchToLower(char* string);
    // Prints the given line to stdout, with lineNum if nonzero
    void printLine(int lineNum, char* fileName, char* text);
    // Processes a file, printing any lines in that file that match the given search pattern
    void processfile(char* filename, char* pattern );
    // Checks for possible options and returns the number of enabled options
    int checkoptions(char** argv);

    // Global variables hold option state
    int optionn = 0;
    int optioni = 0;

    int main(int argc, char**argv)
    {
            // Make sure the minimum number of arguments is present
            if(argc < 3)
            {
                    fputs("Must provide a search pattern and at least 1 file.\n", stderr);
                    return 1;
            }

            // Number of enabled options, used to detemine where search pattern is in argv
            int numoptions = checkoptions(argv);

            // Holds the search pattern
            char* pattern = argv[numoptions+ 1];

            // If "-i" is used, change the pattern to lowercase
            if(optioni)
            {
                    pattern = switchToLower(pattern);
            }

            int i;
            for(i=numoptions + 2; i < argc; i++)
            {
                    char* filename = argv[i];
                    processfile(filename, pattern);         
            }
            // the switchToLower function resulted in pattern being malloc'd
            if(optioni)
            {
                    free(pattern);
            }
            return 0;
    }

    int checkoptions(char** argv)
    {
            int numOptions = 0;
            if(checkStringMatch(argv[1], "-n") || checkStringMatch(argv[2], "-n"))
            {
                    optionn = 1;
                    numOptions++;
            }
            if(checkStringMatch(argv[1], "-i") || checkStringMatch(argv[2], "-i"))
            {
                    optioni = 1;
                    numOptions++;
            }       
            return numOptions;
    }

    void processfile(char* filename, char* pattern)
    {
            FILE* f = fopen(filename, "r");

            // If the file does not exist print an error message and return, otherwise process it
            if(!f)
            {
                    fprintf(stderr, "File %s unopenable.\n", filename);
                    return;
            }

            // Will hold each line scanned in from the file
            char* line = (char*)malloc(MAX_FILE_LENGTH*sizeof(char)); 
            int lineNum = 1;                         
            while(!feof(f))
            {                       
                    line = getLine(line, f);

                    // Only proceed if the line is non-null
                    if(line)
                    {
                            // If the "-i" option is used compare the string after stripping case
                            if(optioni)
                            {
                                    char* lowerLine = switchToLower(line);
                                    if(checkStringMatch(lowerLine, pattern))
                                    {
                                            printLine(lineNum*optionn, filename, line);
                                    }
                                    free(lowerLine);                                        
                            }
                            else
                            {
                                    if(checkStringMatch(line, pattern))
                                    {
                                            printLine(lineNum*optionn, filename, line);
                                    }
                            }               
                            lineNum++;
                    }
            }

            // Close file and free line
            fclose(f);      
            free(line);
    }

    char* getLine(char* loc, FILE* fileStream)
    {
            return fgets(loc, MAX_FILE_LENGTH, fileStream);
    }

    int checkStringMatch(char* toSearch, char* pattern)
    {       char* exists = strstr(toSearch, pattern);
            if(exists)
            {
                    return 1;
            }
            return 0;
    }

    char* switchToLower(char* str)
    {       
            char* newString = (char*)malloc(sizeof(char)*strlen(str));

            //Iterate through charaters in str, switch each letter to lowercase
            int i;
            for(i = 0; str[i]; i++)
            {
                    newString[i]=tolower(str[i]);           
            }

            return newString;
    }

    void printLine(int lineNum, char* fileName, char* text)
    {
            if(lineNum)
            {               
                    printf("%d %s %s", lineNum, fileName, text);
            }
            else
            {
                    printf("%s %s", fileName, text);
            }
    }

Answer 1

Use of Malloc:

Where you do:

char* line = (char*)malloc(MAX_FILE_LENGTH*sizeof(char));

MAX_FILE_LENGTH is known at compile time, and the scope of line doesn't outlive the function, thus there is no reason to utilize malloc - better to simply automatically allocate and allow it to be cleaned up at the end of the function instead of having to free the storage at the end.

char line[MAX_FILE_LENGTH];

You can also simplify this code:

line = getLine(line, f);

// Only proceed if the line is non-null
if(line)
{ ... }

to

if(getline(line, f) != NULL) { ... }

Problems with switchToLower:

Firstly, you malloc and return a new string - this places the onus on the caller to free it later. Generally, unless there is a really good reason for this, you should prefer the caller to already have allocated the buffer which you then utilize:

void switchToLower(const char *str, char* buf, size_t bufflen);

Have the caller pass in a string buffer which can then be written to. The bufflen is there make sure you only copy that many characters - this will help stop potential buffer overflows, which can be dangerous. Finally, it uses const to show that you're not modifying the original string at all.

There's another problem with this function too - the string you return isn't null-terminated. You've got an off by one error here:

char* newString = (char*)malloc(sizeof(char)*strlen(str));

This allocates enough storage for the characters but does not include storage for the null terminator. Every time you see a malloc with a strlen, always remember it should be malloc(strlen(str) + 1). Also, per the C standard, sizeof(char) is always 1, hence it is unnecessary. The cast at the front is also unnecessary in C (although it is needed in C++). I'd write this as:

char* newString = malloc(strlen(str) + 1);

In fact, in this function, is there any reason you're not simply modifying the original string that gets passed in? From the rest of the code, it seems as though it isn't used anywhere else, so simply modifying the original line might be the best idea.

Usage help:

Something of a minor problem, but your usage string could be a bit better. For one thing, it doesn't mention the order of arguments. Secondly, it doesn't mention anything about options (someone using this would need to look at the actual code to know there was a case-insensitive comparison option). All these options and argument orders should be incorporated into your usage string. Remember, you want to make it as clear as possible how to use this; if the user has to look at your code to figure it out, then your usage help isn't doing its job.

Indentation:

It looks as though you're using 8 space indentation - this is a bit of a personal thing, but I consider this too whitespace heavy (and I imagine a lot of other programmers do as well). 4 spaces looks much nicer to my eyes.

Answer 2

Yuushi covered pretty much everything, but a few more minor things stood out to me.

---

I'm not a fan of the overuse of MAX_FILE_LENGTH.

When you're setting up automatic duration buffers, sure, use MAX_FILE_LENGTH. You're making your functions significantly less flexible though when you assume that things are MAX_FILE_LENGTH.

For example:

char* getLine(char* loc, FILE* fileStream)
{
        return fgets(loc, MAX_FILE_LENGTH, fileStream);
}

Should probably be:

char* getLine(char* loc, FILE* fileStream, size_t bufSize)
{
        return fgets(loc, bufSize, fileStream);
}

Then you'd call it like (assuming a stack allocated array):

char buf[MAX_FILE_LENGTH];
getline(buf, fh, sizeof(buf));

Note that getline can now be used in situations where you have differing buffer sizes. There's no need to constrain your function to one very specific use.

---

That brings me to my next point: getLine shouldn't exist. It's just a (very) thin wrapper around fgets. Just use fgets inline. It's basically a lazy way to avoid typing MAX_FILE_LENGTH again, but as I said, you should avoid hard coding things when (reasonably) possible. (And yes, using a #defined constant is in a lot of ways essentially hard coding -- centralized hard coding yes, but hard coding nonetheless.)

---

I'm not a fan of your fgets loop. It's typically better to just loop directory on fgets:

char line[MAX_FILE_LENGTH];
while (fgets(line, sizeof(line), fh)) {
    //Do something with line
    //Note that if the line is longer than MAX_FILE_LENGTH - 1, line will not
    //actually be a line. You could check line for that though.
}

---

I'm not a fan of the global option variables. In this situation you could easily make the case that they don't matter, but on a site that specializes in pedantic code criticisms, I'm obligated to point out that they're unnecessary and couple the functionality of some of some of your functions to magical hidden state. If you passed the options around, you could would be more flexible (it would also be marginally more cumbersome to write, but such is the woe of high quality code :)).

---

MAX_FILE_LENGTH seems to be a misnomer. Should not it be MAX_LINE_LENGTH (or maybe MAX_FILE_LINE_LENGTH)?

---

Speaking of misnomers, checkStringMatch fits that too. checkStringContains would be better. Or perhaps stringContains. Not a fan of the check prefix. It sounds like the function is responsible for ensuring that the string matches, not that it simply checks (and yes, that sounds like crazy talk since I just said "not that it simply checks -- maybe I just have weird naming habits).

(Note: if you plan on extending checkStringMatch to do more than check substring existence, ignore this comment.)

---

If you're going to comment your functions, you might as well use doc comments. They can be parsed by a lot of existing programs to make prettified documentation (well, API documentation, not true documentation). They also tend to be a bit easier to visually scan:

/**
 * Determines whether the given string contains the specified search pattern.
 * @param toSearch The string in which to search
 * @param pattern The pattern for which to search
 * @return true (non-zero) if toSearch contains pattern, and false (0) otherwise
*/
int checkStringMatch(char* toSearch, char* pattern);

---

A lot of your pointers being passed around could point to constant characters.

It's a lot clearer (and better guaranteed) that int checkStringMatch(const char* toSearch, const char* pattern) doesn't change toSearch or pattern than int checkStringMatch(char* toSearch, char* pattern).

---

if(checkStringMatch(argv[1], "-n") || checkStringMatch(argv[2], "-n"))

That doesn't make sense. You should probably just use strcmp.

You could, however, extend the use of strstr to support situations like fakegrep -ni pattern file. Basically you'd just need to check if the first char is a - (so you know it's not a pattern [though excluding - from allowed patterns seems odd]) and then check for the existence of n and i.

---

Rather than printing straight to stderr in processfile in the case of an error, you could return an error flag. That would allow you to be more flexible with your error handling.

For example, if you wanted to add a -q flag later to suppress "couldn't open file" error messages, you could do something like:

#define PROCFILE_ERR_OK 0
#define PROCFILE_ERR_OPENFAIL 1
//#define PROFILE_ERR_... x
for (i = numoptions + 2; i < argc; ++i) {
    int err;
    char* filename = argv[i];
    err = processfile(filename, pattern);
    if (err != PROCFILE_ERR_OK && !suppressErrors) {
        //Do something
    }
}

---

It would be easy to just use strstr inline anywhere you use checkStringMatch.

When reading through your code, I had to look to see what checkStringMatch is, but I know what strstr is. The level of indirection can cause a bit of confusion (though checkStringContains would not).

Since a NULL pointer is guaranteed to evaluate to false, you could even have the same usage:

For example:

if(checkStringMatch(lowerLine, pattern))

Could be:

if(strstr(lowerLine, pattern))

If you plan to extend or change the functionality of checkStringMatch though, then this should obviously be ignored.

---

This is 100% opinion, but #include<stdio.h> has always looked odd to me. #include <stdio.h> looks cleaner. Once again though, 100% opinion. To each his own.

Answer 3

In addition to comments by others:

• reverse the order of functions to avoid prototypes.
• all functions except main should be static
• use getopt for shell arg handling rather than rolling your own. See the manual page for how to use it.
• argc < 3 is wrong if there are options on the command line

• if there are lines longer than 500 characters, line numbers printed thereafter are wrong and the pattern string will not be found if it straddles the 500 character boundary

• your grep does not read from stdin when there are no file arguments, for example to detect "string" in the output of program

  program | grep -i "string"
  

• if, while, for, etc should be followed by a space

• note that strcasestr can be used to do a case-insensitive search instead of modifying the pattern text and copying each input line.

• inconsistent capitalisation in naming: checkoptions, processfile, but getLine, checkStringMatch

• Some comments are noisy (i.e. useless)

• use perror instead of fprintf(stderr, ...) on printing errors

• optionn and optioni are too similar and do not convey meaning. Try print_lines and ignore_case if they are going to be global. There is little overhead in passing them as parameters in this program, so the globals are unnecessary.

• passing lineNum * optionn to printLine is a hack. Just pass the line number and optionn or use the global in the function.

• you don't check for failure of malloc.

• the real grep doesn't always print the filename. When it does, it adds a colon. It also adds a colon after the line numner.

• your -i does not work because you fail to terminate the string copy - as @Yuushi says, you must allocate an extra char and you must also set it

  `newString[i] = '\0';`
  

• be aware that not everyone likes "doc comments". @Corbin's example could be rewritten more concisely as:

  /* Return non-zero if input string 's' contains pattern 'p'
  */
  static int checkStringMatch(const char *s, const char *p);