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
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\$
    – Raihli
    Aug 31 '16 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\$ Aug 31 '16 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\$
    – Raihli
    Aug 31 '16 at 0:48
  • \$\begingroup\$ I removed the free mention. I must have overlooked the closedir call. \$\endgroup\$ Aug 31 '16 at 7:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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