current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite

I got a pretty large main function that I want to break up in smaller helper functions. Can you help me suggest what to break out into helper functions? The function is part of my own command-line shell and all the code is available on github. I think that a good helper function is for "builtin commands" (e.g. commands that I implement myself) and another good helper method could be "parse shell input" (e.g. the handletoken function). Are there any more possible helper functions that I should consider?

My goal is to make the main function small and readable. 

int main(int argc, char *argv[]) {
    /* int awk = 0; */
    char line2[BUFFER_LEN];
    char linecopy[BUFFER_LEN];
    char *params[100];
    char *cl;
    char *path_value;
    int i = 0;
    int isBackground = 0;
    int built_in_command = 0;
    int fd[2];
    int b;
    long time;
    int status = 0;
    int max = 80;
    struct timeval time_start;
    struct timeval time_end;
    sigset_t my_sig;
    pid_t pid_temp;
    char *pathValue;
    char *path_strdup;
    struct sigaction sa, osa;
    char line[BUFFER_LEN];
    char *input, shell_prompt[BUFFER_LEN];
    size_t length;
    int ret;
    struct sigaction less_sa;
    err_setarg0(argv[argc - argc]);
    pid_temp = 0; /* To please the compiler */

    sa.sa_sigaction = sighandler;
    sa.sa_flags = SA_SIGINFO;
    sigaction(SIGINT, &sa, &osa);

    less_sa.sa_handler = &handle_sigchld;
    sigemptyset(&less_sa.sa_mask);
    less_sa.sa_flags = SA_RESTART | SA_NOCLDSTOP;
    if (sigaction(SIGCHLD, &less_sa, 0) == -1) {
        perror(0);
        exit(1);
    }

    /* get the PATH environment to find if less is installed */
    pathValue = getenv("PATH");
    if (!pathValue) {
        printf("'%s' is not set.\n", "PATH");
    }
    else {
        printf("'%s' is set to %s.\n", "PATH", pathValue);
    }
    path_strdup = strdup(pathValue);
    path_value = strtok(path_strdup, ":");
    ret = find_less_program(path_value);
    free(path_strdup);
    while (1) {

        i = 0;
        Janitor(SIGCHLD);
        /* Create prompt string from user name and current working directory. */
        snprintf(shell_prompt, sizeof(shell_prompt), "%s:%s $ ", getenv("USER"), getcwd(NULL, 1024));
        /* Display prompt and read input (NB: input must be freed after use)...*/
        input = readline(shell_prompt);

        if (!input)
            break;

        add_history(input);

        strncpy(line2, input, BUFFER_LEN);
        strncpy(linecopy, input, BUFFER_LEN);

        length = strlen(input);
        if (input[length - 1] == '\n') {
            input[length - 1] = '\0';
        }
        built_in_command = handleBuiltinCommands(input, ret);
        if (0 == built_in_command) {    /*Not a built in command, so let execute it*/
            /*isBackground = background_check(max, input);*/
            isBackground =0;
            for (b = 0; b < max; b++) {
                if ('&' == input[b]) {
                    printf("is background");
                    isBackground = 1;
                }
            }
            if (isBackground == 1) {    /*If backgroundprocess*/
                if (pipe(fd) == -1) {  /*(two new file descriptors)*/
                    perror("Failed creating pipe\n");
                }
                pid_temp = fork();
            }
            else if (isBackground == 0) {    /*If foreground process*/
                gettimeofday(&time_start, NULL);
                if (1 == isSignal) {    /*If using signaldetection*/
                    sigemptyset(&my_sig); /*empty and initialising a signal set*/
                    sigaddset(&my_sig, SIGCHLD);    /*Adds signal to a signal set (my_sig)*/
                    /*http://pubs.opengroup.org/onlinepubs/7908799/xsh/sigprocmask.html*/
                    sigprocmask(SIG_BLOCK, &my_sig, NULL);
                }
                /*pid_temp = fork();*/
                foreground = pid_temp;    /*Set pid for foreground process*/
            }
            if (0 < pid_temp) {
                /*Parent process*/
            }
            else if (0 > pid_temp) {
                /*Error*/
            }
            else {
                /*Child process*/
                if (1 == isBackground) {    /*Backgroundprocess*/
                    dup2(fd[STDIN_FILENO], STDIN_FILENO);
                    close(fd[0]);
                    close(fd[1]);
                }
                length = strlen(linecopy);
                if (linecopy[length - 1] == '\n')
                    linecopy[length - 1] = '\0';
                /*printf("Command line: %s\n", linecopy);*/
                cl = strtok(linecopy, " ");
                i = 1;
                params[0] = NULL;
                i = handleToken(input, cl, params, i);
                dump_argv("Before"
                                  " exec_arguments", i, params);
                exec_arguments(i, params);
                corpse_collector();
                /*free(input)*/;
            }
            if (0 == isBackground) {    /*Foregroundprocess*/
                waitpid(foreground, &status, 0);    /*Waiting*/
                /*Foregroundprocess terminated*/
                gettimeofday(&time_end, NULL);
                time = (time_end.tv_sec - time_start.tv_sec) * 1000000 +
                       time_end.tv_usec - time_start.tv_usec;
                printf("Execution time %ld.%03ld ms\n", time / 1000, time % 1000);

                if (1 == isSignal) {    /*If using signaldetection*/
                    int a = sigprocmask(SIG_UNBLOCK, &my_sig, NULL);
                    /*http://man7.org/linux/man-pages/man2/sigprocmask.2.html*/
                    if (0 == a) {
                        /*Sigprocmask was successfull*/
                    }
                    else {
                        /*Sigprocmask was not successfull, return=-1*/
                    }
                    Janitor(SIGCHLD);
                }
            }
            else if (1 == isBackground) {
                close(fd[0]);
                close(fd[1]);
            }
        }
        built_in_command = 0;    /*Reset*/
        memset(line, 0, sizeof line); /*Reset*/
        free(input);
    }
    return (0);
}
share|improve this question
up vote 3 down vote accepted

Look for the separate

    sa.sa_sigaction = sighandler;
    sa.sa_flags = SA_SIGINFO;
    sigaction(SIGINT, &sa, &osa);

These three lines use sighandler which is not defined here, sa, and osa. All three are only used here. So you can move them into their own function without impacting the rest of the method. Don't forget the variable declaration.

Since you never use osa again, it's unclear why you have it here. Later you pass a 0 instead. Why not do it here? 

    less_sa.sa_handler = &handle_sigchld;
    sigemptyset(&less_sa.sa_mask);
    less_sa.sa_flags = SA_RESTART | SA_NOCLDSTOP;
    if (sigaction(SIGCHLD, &less_sa, 0) == -1) {
        perror(0);
        exit(1);
    }

The same thing applies here with less_sa. This could be in its own function or in the function with the previous code. 

    change_signal_handlers();

If you want both in one. 

    change_interrupt_handler();
    change_child_handler();

If you want them to be in different functions.

Other names would also be valid. These are just possibilities. 

    /* get the PATH environment to find if less is installed */
    pathValue = getenv("PATH");
    if (!pathValue) {
        printf("'%s' is not set.\n", "PATH");
    }
    else {
        printf("'%s' is set to %s.\n", "PATH", pathValue);
    }
    path_strdup = strdup(pathValue);
    path_value = strtok(path_strdup, ":");
    ret = find_less_program(path_value);
    free(path_strdup);

The only variable used later is ret. So this could be moved into its own function.

I don't know what ret is. Perhaps a more descriptive name?

Beyond that, I'd move the function call to get ret and the entire while loop into its own function. You might further break up that function as well, but that stuff doesn't belong in main.

That would make your main very simple:

int main(int argc, char *argv[]) {
    err_setarg0(argv[argc - argc]);

    change_interrupt_handler();
    change_child_handler();

    respond_to_shell_commands();

    // silence compiler warning with unnecessary return
    return 0;
}

You could actually do without the return as well, but perhaps your compiler squawks. It won't hurt anything either way.

And I'm not convinced that you need argc - argc either, but it doesn't seem to be hurting anything. You might comment in the reason for that though.

Consistency

Your naming standards are all over the place. Sometimes you use snake_case and others you use camelCase. Please pick one and stick to it. If I tried to modify this code, I'd have no idea what standard to use. In C, snake_case is more common. It also is a bit easier for international users to understand, so I would recommend that. Using either consistently would be better than the current state though.

Get rid of unused code

        strncpy(line2, input, BUFFER_LEN);

Never used. 

        memset(line, 0, sizeof line); /*Reset*/

Why reset a variable that you never use? Perhaps this should be the same variable as linecopy.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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