HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Search a file (simple grep clone)

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
simplefilesearchgrepclone

Problem

For part of a project, I was given this directive:


The grep command will take an exact string and an exact filename as
the 2 arguments and you will need to search through the filename for
the given string and display the "line" and "line number" the string
resides in.
Example output would be:

# grep bob FileWithBob
12 and then bob picked up the phone and called jane 
24 even though they knew that bobby was not going to show up


Testing seems to give identical results to the actual grep program for everything I've tried:

$ ./my_grep "test" grep.c 
1:// test
2:// multi-line test

$ grep -n "test" grep.c 
1:// test
2:// multi-line test


grep.c:

``
// test
// multi-line test
#define _GNU_SOURCE 1 // for getline()
#include
#include
#include
#include
#include
#include
#include
#include

#ifndef streq
#define streq(x, y) (strcmp((x), (y)) == 0)
#endif

#define MSGBUFSIZE 512

regex_t pattern;

int compile_pattern(const char *pat)
{
int flags = REG_NOSUB;
char error[MSGBUFSIZE] = "";

int ret = regcomp(&pattern, pat, flags);
if (ret)
{
regerror(ret, &pattern, error, sizeof(error));
fprintf(stderr, "pattern
%s`: %s\n", pat, error);
return ret;
}

return 0;
}

// read lines of text and match against the pattern
int process(FILE *fp)
{
char *buf = NULL;
size_t size = 0;
char error[MSGBUFSIZE] = "";
int ret = 0;

for (int i = 1; getline(&buf, &size, fp) != -1; i++)
{
ret = regexec(&pattern, buf, 0, NULL, 0);
if (ret)
{
if (ret != REG_NOMATCH)
{
regerror(ret, &pattern, error, sizeof(error));
fprintf(stderr, "%s\n", error);
free(buf);
return ret;
}
}
else
printf("%d:%s", i, buf);
}

free(buf);
return 0;
}

void usage(char* prog_name)
{
fprintf(stderr, "usage: %s [-i] [-E] pattern [ files ... ]

Solution

On the whole your code seems pretty concise and clean. I've just got a couple of side points.

Scope creep

This is a real problem in production code, where developers implement stuff that isn't really needed. Your specification you've posted says 'take an exact string and an exact filename'. You've allowed a regular expression to be passed. The original requirements could have been met by just using strstr which would have made the code quite a bit smaller. You've used extra time and added extra complexity, is there a real benefit?

The scope creep also means that I can't pass the basic string "\n" into your parser and have it find it as the '\n' is interpretted by the regex parser. So, with grep I can do:

grep -F "\n" someFile.h

And get lines with the text \n:

printf("some text\n");

Whereas running your code:

./a.exe "\n" someFile.h

I get every line that ends in a newline (i.e. every line of the file).

-h -i and arguments

If you're going to implement programs that take in arguments, it's a good idea to give the user some way to find out what the arguments mean. When I use grep -i, I'm trying to do a case-insensitive search. Your program advertises a '-i' option, however it doesn't do the same thing as grep. Instead I get told it can't open a file:

$ ./a.exe -i if someFile.h

if: could not open: No such file or directory

Context

StackExchange Code Review Q#149583, answer score: 5

Revisions (0)

No revisions yet.