4
\$\begingroup\$

I've written this minishell but I'm not sure I'm making a correct control of the errors. Could you give me some advice?

#include <stdio.h>
#include <stdlib.h> 
#include <string.h> 
#include <stdbool.h>

#define LINE_LEN  50
#define MAX_PARTS  50


int main ()
{
char* token;
char str[LINE_LEN];
char* arr[MAX_PARTS];
int i,j;
bool go_on = true;

while (go_on == true){
    printf("Write a line:('quit' to end) \n $:");
    fgets(str, LINE_LEN, stdin);

    if (str==NULL) {
        goto errorfgets;
    } else {
        size_t l=strlen(str);
        if(l && str[l-1]=='\n')
            str[l-1]=0;

        i=0;
        /* split string into words*/
        token = strtok(str, " \t\r\n");
        while( token != NULL ) 
        {
            arr[i] = token;
            i++;
            token = strtok(NULL," \t\r\n");
        }

        fflush(stdin);

        /* check if the first word is quit*/
        if (strcmp(arr[0],"quit")==0)
        {
            printf("Goodbye\n");
            go_on = false;
        } else {

            for (j=0; j < i; j++){
            printf("pajaritos por aqui'%s'\n", arr[j]);     
            }   
        }
    }
}

return 0;
errorfgets:
    printf("fgets didn't work correctly");
    return -1;
}
\$\endgroup\$
4
\$\begingroup\$

I think I haven't really understood the point of your program but I'll try to give you a few hints :

  • The indentation is not really good. I am not sure if this is because of SE but I'd rather point it out just in case.
  • You'll hear everything and its opposite about goto. It surely has drawbacks but it can be quite useful to handle errors and free the ressources that are not required anymore. However, in your case, there's no need for goto so it's probably better to Keep It Simple and just use return.
  • I am not a big fan of a go_on variable if break can do the trick. Also, it makes stdbool pointless.
  • Things will be easier to understand if you declare local variables in the smallest possible scope. Depending on the version of C you are using, you can even declare j in your for statement : for (int j=0; j < i; j++).
  • Because of return/break, you can now remove levels of indentation (again, this is purely personal preference).
  • You shouldn't repeat "quit" in multiple places. It will make it hard to change if it has to change in the future.
  • There is a risk that arr[i] = token; goes to far if you ever change a constant without changing the other. Let's make sure it does not go too far.

At the end, here what the code would be like :

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define LINE_LEN  50
#define MAX_PARTS  50

static const char quit[] = "quit";

int main ()
{
    char str[LINE_LEN];
    char* arr[MAX_PARTS];

    for (;;) {
        printf("Write a line:('%s' to end) \n $:", quit);
        fgets(str, LINE_LEN, stdin);

        if (str==NULL) {
            printf("fgets didn't work correctly");
            return -1;
        }
        size_t l = strlen(str);
        if (l && str[l-1]=='\n')
            str[l-1]=0;

        /* split string into words*/
        int i;
        char* token = strtok(str, " \t\r\n");
        for (i=0; i<MAX_PARTS && token != NULL; i++)
        {
            arr[i] = token;
            token = strtok(NULL," \t\r\n");
        }

        fflush(stdin);

        /* check if the first word is quit*/
        if (strcmp(arr[0], quit)==0)
        {
            printf("Goodbye\n");
            break;
        }
        for (int j=0; j < i; j++){
            printf("pajaritos por aqui '%s'\n", arr[j]);
        }
    }

    return 0;
}
\$\endgroup\$
1
\$\begingroup\$

I would suggest following:

  1. A separate error handler function. Don't exit from shell until user wants to. (most of shells do this)
  2. fgets() is dangerous function (from compiler warnings). you could look into some system level functions to replicate getch() which gets a character and doesn't prints on screen and doesn't require a return for getting the character. It just get the character when you press the key. This way you can have a long input.(you have to print the characters manually if you use getch())
  3. Break down the loop into functions.
  4. If you go for getch() you could even have shortcuts also.(not necessary as its a mini shell).
  5. getch() can be helpful in getting the passwords as well.
\$\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.