2
\$\begingroup\$

I have the following code, made in C language, which reproduces the functionality of the mv (move) command from linux. The problem is that the code is very inefficient. How I can optimize or make the code more efficient without changing the structure too much?

#include    <limits.h>
#include    <fcntl.h>
#include    "ourhdr.h"
#include    <sys/stat.h>
#include    <dirent.h>
int main(int argc,char *argv[])
{
    if(argc!=3)
    {
        printf("<Sintaxa> <fisier sursa> <fisier destinatie> \n");
        exit(1);
    }
    int ren1,ren2;
    struct stat buf1;
    struct stat buf2;
    lstat(argv[2],&buf1);
    lstat(argv[1],&buf2);
    char src[PATH_MAX];
    strcpy(src,argv[1]);
    char dst[PATH_MAX];
    strcpy(dst,argv[2]);
    int src_size = (int)strlen(src);
    int dst_size = (int)strlen(dst);
    dst[dst_size] = '/';
    dst[dst_size + 1] = '\0';
    dst_size++;
    int i;
    int index=0;
    int index1=src_size;
    while(--index1>0)
    {
        if(src[index1]=='/')
        {
            index1=0;
        }
        else
            index++;
    }
    for (i = src_size-index-1; i < src_size; i++)
    {
        dst[dst_size++] = src[i];
    }
    dst[dst_size] = '\0';
    if (access(src,F_OK)==-1)
    {
        err_ret("%s: cannot stat '%s'", argv[0],argv[1]);
        return -1;
    }
    if(S_ISDIR(buf1.st_mode))
    {
        if(((S_ISDIR(buf2.st_mode))&&(!(opendir(argv[1]))))||(!(opendir(argv[2]))))
        {
            err_quit("%s: cannot move '%s' to '%s': Permission denied \n", argv[0],argv[1],dst);
        }
        ren1=rename(src,dst);
        if(ren1!=0)
        {
            err_quit("Error: unable to move the file");
        }
    }
    else
    {
        ren2=rename(argv[1],argv[2]);
        if(ren2!=0)
        {
            err_quit("Error: unable to rename the file ");
        }
    }
}

The code above works fine but is way to inefficient

\$\endgroup\$
2
\$\begingroup\$

This is a small program, so some of the problems are not likely that big. If this were part of the API, problems may stack up and then make an actual difference.

There are several points which you should work on:

  • opendir leak: opendir opens a file descriptor. You open 2 directories, none of which are closed. Make sure to call closedir. If this is part of an API, those memory leaks will stack up. Otherwise, this might not be a problem, since when the process ends, the descriptors should be cleared automatically.
  • Error checks: If I were to run this code a lot of times, some of which are intended to end in errors, there would be some clatter due to the positioning of your error checks. Checks like does src exist or can dst dir be accessed, should be done first. This will eliminate the need for some operations. Generally, it is always best to perform error checks first.
  • Finding the name: A big part of the code is dedicated to finding the name of the src, and copying it to dst. You might improve it by using built-in functions instead. Use strrchr to find the last / and strcpy to copy the name.

Maybe there are other possible changes, but that should be the gist of it. Most of those problems have a small effect. This is a small program, so you won't notice must performance changes (unless this is an API).

\$\endgroup\$

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.