Constructing a simple shell from scratch

I'm actually doing my whole shell in C from scratch from a Linux computer.
The thing is that I think we all do our best from creating the simplest things that can be explained easily. And I'm not sure my shell has these qualities.

All command I tried worked but command 1 | command 2 | command 3 or command 1 | command 2 > command 3. I would be glad if you had any ideas but I'm okay with it. My point is that this code it a bit heavy and I'm sure it could have been done easier.

If you have any idea to make it simpler, I would be glad to hear it.

This shell is constructed this way:

The main method calls a command function which calls a parsing function which reads the function written by the user, fills an array with the function and returns a number for each delimiter. The command is constructed by case and does an execvp on the array with the command. The thing that I'm not allowed to modify is the parsing function.

```
#include
#include
#include
#include
#include
#include
#include
#include
#include

char *delimiteurs = ";&<>|";
char *respP[10]; // tableau contenant la commande a executer
char *fout[10];// tableau qui affiche le resultat de l'istruction shell lue dans fin
char *fin[10]; // la méthode commande va exécuter une instruction shell lue dans le tableau fin elements par element grace a parsing
char *tube[10]; // tube qui assusre la communication entre deux processus
char mot[50];
int symboleP, status, c;
void parsing(){
int i=0;
int cmot=0;
while(1){
c = getchar();
if (c == '\n') {symboleP = 0;return;}
else if (c == ';') {symboleP = 1;return;}
else if (c == '&') {symboleP = 2;return;}
else if (c == '') {symboleP = 4;return;}
else if (c == '|') {symboleP = 5;return;}
else if (c == EOF) {symboleP = 7;return;}
else if (c != ' ') {
symboleP = 10;
while(c != '\n' && !strchr(delimiteurs,c)){
i=0;
whil

As a heads up, I write much more C++ than C now so stylistic comments may not meet what is consider correct for C code. I'll get those out of the way now

Style

The absolute first thing you need to do is fix your indentation. As is, I find it almost impossible to read. Pick a tab-width (I like 4), set your IDE/text editor to replace tabs with spaces, and go from there. Indent inside scopes (i.e. curly braces) or wherever there could be curly braces (i.e. one-line for-loops).

After that, add spaces around operators, before the conditions of while loops, before curly braces, etc. This makes it easier to grok what any given line is saying.

This is a pet peeve, and many people feel differently, but I can't stand code like this:

short_name                   = value;
super_ridiculously_long_name = value;

I don't find it more readable or visually pleasing to have my equals signs (or parentheses, or whatever) line up in this way.

I also almost never like putting multiple statements on a single line, with the possible exception of if (condition) return; Especially don't do something like this

respP[i++] =0;                                  int keyboard;

Just put them on separate lines.

You should also use consistent brace style - don't mix up these two

while (condition) {
    // do stuff
}

while (condition)
{
    // do stuff
}

I find the first one easier to read, and much more common in C code.

I absolutely detest this aspect of C:

void commande () {
    pid_t pid, fid;
    int background = 0;
    int status;
    char car;
    int i, j, k, l;
    int p, p2;
    int execute=1;
    int output=0;
    int input=0;
    int tube=0;
    int fd[2];
    int fich, fo, screen;

But I understand that it is pretty common. If you must do it, please make sure to give every variable a good, easy to understand name. I generally dislike Hungarian notation but I also usually declare my variables right before I use them and can see their type. When declaring a variable far away from its use, I find Hungarian notation can make it more clear what that variable's type is.

Now we can start on actually understanding your code.

Reduce global state

You don't use the variables c or delimiteurs anywhere except in parsing, so you should move them there (delimiteurs is also a constant, so mark it as such).

Both fin and fout are only used in commande, so move them there.

You never use tube (well, you declare int tube in commande) so you can delete it. Same for status.

You can remove symboleP from global scope as well by doing something like this (I removed a ton from this for clarity):

int main() {
    int symboleP;
    commande(&symboleP);
}
...
int parsing() {
    return symboleP;
}

switch vs if/else if/else

You already use switch statements in your code, but you could use them in more places. This may give a speedup to your program, and for long chains I find them more readable.

parsing function (could use a better name)

I'm not totally clear on the specifics of what is happening in this section, but I'd recommend maybe using fewer character and integer literals, and used named constants/enums instead. I'd also recommend putting the final case into its own helper function for readability. All in all, I think I'd rewrite this section to look something like this:

bool handle_default_case(int c, const char* delimiteurs) {    
    int i = 0;
    int cmot = 0;
    while (c != '\n' && !strchr(delimiteurs, c)) {
        i = 0;
        while (c != ' ' && c != '\n' && !strchr(delimiteurs, c)) {
            mot[i++] = c;
            c = getchar();
        }
        break;
    }
    while(c == ' ') {
        c = getchar();
    }
    ungetc(c, stdin);
    mot[i] = 0;
    respP[cmot++] = strdup(mot); // fonction strdup renvoie l'adresse de la nouvelle chaine de caractère qui vient dêtre duppliquer ou sinon NULL --> alloue de l'espace necessaire pour stoker au mot
    fflush(stdout);
    if (c == '\n' || strchr(delimiteurs, c)) {
        respP[cmot] = 0;
        return true;
    }
    return false;
}

typedef enum {
    NEWLINE,
    SEMICOLON,
    AMPERSAND,
    LESS_THAN,
    GREATER_THAN,
    VERTICAL_BAR,
    END_OF_FILE,
    DEFAULT
} parsed_character;

parsed_character parsing() {
    int c = getchar();
    const char *delimiteurs = ";&<>|";
    while (1) {
        switch (c) {
            case '\n':
                return NEWLINE;
            case ';':
                return SEMICOLON;
            case '&':
                return AMPERSAND;
            case '':
                return GREATER_THAN;
            case '|':
                return VERTICAL_BAR;
            case EOF:
                return END_OF_FILE;
            case ' ':
                break;
            default:
                if (handle_default_case(c, delimiteurs)) {
                    return DEFAULT;
                }
        }
        c = getchar();
    }
}

`comm

StackExchange Code Review Q#116603, answer score: 3