6
\$\begingroup\$

I'm making a program where I need to check if files in the directory are correct (46 files and all 11 chars).

I've got two methods to do it but I don't know which one is better.

Using opendir/readdir:

int checkfiles()
{
    DIR             *dir;
    struct dirent   *dp;
    int             count = 0;

    if ((dir = opendir("img/")) != NULL)
    {
        while ((dp = readdir(dir)) != NULL)
            if (dp->d_name[0] != '.')
            {
                count++;
                if (strlen(dp->d_name) != 11)
                    error(4);
            }
            if (count != 46)
                error(3);
        if (closedir(dir) == -1)
            error(2);
    }
    else
        error(1);

    return (0);
}

or using scandir:

int checkfiles()
{
    struct dirent   **namelist;
    int             i = 0;
    int             n;

    n = scandir("img/", &namelist, 0, alphasort);
    if (n < 0)
        error(1);
    else if (n != 48)
        error(3);
    else
    {
        while (i++ < n - 1)
        {
            if (namelist[i]->d_name[0] == '.')
                continue;
            if (strlen(namelist[i]->d_name) != 11)
                error(4);
            free(namelist[i]);
        }
        free(namelist);
    }

    return (0);
}

The second is more readable with less indentation, but uses more resources by sorting all files.

\$\endgroup\$
0

2 Answers 2

Reset to default
5
\$\begingroup\$

...which one is better (?)

6.001 of one, 1/2 dozen of the other - not much difference.

  1. Weakness: method 1: Fix indent. if (count != 46) should be indented left. This excessive indentation gives the false impression of when the the test occurs - as part of the while loop or after it? Better to use {}.

    while ((dp = readdir(dir)) != NULL)
    if (dp->d_name[0] != '.')
    {
        count++;
        if (strlen(dp->d_name) != 11)
            error(4);
    }
    //if (count != 46)
    //    error(3);
if (count != 46)
    error(3);

  2. Weakness: method 2: else if (n != 48) error(3); make the usually good assumption the directory contains 2 entries . and ... This is not always correct given a root may not have a .. entry.

  3. Weakness: method 2: Reliance on successful malloc(). namelist[i] may have the value of NULL and if (namelist[i]->d_name[0] == '.') is UB.

  4. Weakness: method 2: Code does not free namelist in (n != 48) case.

\$\endgroup\$
2
  • \$\begingroup\$ I've improve my code with the point you said. Just the second point I don't understand because if I check in a directory, ./img/, there allways a .., no ? \$\endgroup\$ Commented Aug 31, 2016 at 10:41
  • \$\begingroup\$ @Raihli Some systems do not have a .. in the root directory. ./img/ may link (symbolic or hard link) to the root. Not likely - yet robust code would not assume the existance of ... \$\endgroup\$ Commented Aug 31, 2016 at 14:17
0
\$\begingroup\$

You should let your code be indented automatically. One of the if statements is indented too far.

I prefer the first variant, because you don't need to allocate the memory for the whole directory at once. And, as soon as the count is larger than 46, you can break out of the loop and return immediately.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ free what ? The struct dirent *dp ? The man of readdir says "On success, readdir() returns a pointer to a dirent structure. (This structure may be statically allocated; do not attempt to free(3) it.)". Yeah one indent too far, going to rework it. \$\endgroup\$ Commented Aug 31, 2016 at 0:48
  • \$\begingroup\$ I removed the free mention. I must have overlooked the closedir call. \$\endgroup\$ Commented Aug 31, 2016 at 7:03

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.