shell c ++ per linux

Question

#include <cstring>
#include <map>
#include <iostream>
#include <fstream>
#include <sstream>
#include <string>
#include <sys/types.h>
#include <sys/wait.h>
#include <unistd.h>
#include <vector>
#include <filesystem>
#include <errno.h>
#include <bits/stdc++.h>
std::string USERDIR = getenv("HOME");
std::string ALIASFILE = USERDIR+"/shell/.alias";
std::vector<std::string> Split(std::string input, char delim);
void Execute(const char *command, char *arglist[]);
std::map<std::string, std::string> alias(std::string file);
bool BuiltInCom(const char *command, char *arglist[],int arglist_size);
char** conv(std::vector<std::string> source);
bool createAlias(std::string first, std::string sec);
std::string replaceAll(std::string data, std::map <std::string, std::string> dict);
int main() {
  while (1) {
    char path[100];
    getcwd(path, 100);
    char prompt[110] = "$[";
    strcat(prompt, path);
    strcat(prompt,"]: ");
    std::cout << prompt;
    // Takes input and splits it by space
    std::string input;
    getline(std::cin, input);
    if(input == "") continue;
    std::map<std::string, std::string> aliasDict = alias(ALIASFILE);
    input = replaceAll(input, aliasDict);
    std::vector<std::string> parsed_string = Split(input, ' ');
    // Splits parsed_string into command and arglist
    const char * com = parsed_string.front().c_str();
    char ** arglist = conv(parsed_string);
    // Checks if it is a built in command and if not, execute it
    if(BuiltInCom(com, arglist, parsed_string.size()) == 0){
        Execute(com, arglist);
    }
    delete[] arglist;
  }
}

std::vector<std::string> Split(std::string input, char delim) {
  std::vector<std::string> ret;
  std::istringstream f(input);
  std::string s;
  while (getline(f, s, delim)) {
    ret.push_back(s);
  }
  return ret;
}

void Execute(const char *command, char *arglist[]) {
  pid_t pid;
  //Creates a new proccess
  if ((pid = fork()) < 0) {
    std::cout << "Error: Cannot create new process" << std::endl;
    exit(-1);
  } else if (pid == 0) {
    //Executes the command
    if (execvp(command, arglist) < 0) {
      std::cout << "Could not execute command" << std::endl;
      exit(-1);
    } else {
      sleep(2);
    }
  }
  //Waits for command to finish
  if (waitpid(pid, NULL, 0) != pid) {
    std::cout << "Error: waitpid()";
    exit(-1);
  }
}

bool BuiltInCom(const char *command, char ** arglist, int arglist_size){
  if(strcmp(command, "quit") == 0){
    delete[] arglist;
    exit(0);
  } else if(strcmp(command, "cd") == 0){
    if(chdir(arglist[1]) < 0){
      switch(errno){
        case EACCES:
          std::cout << "Search permission denied." << std::endl;
          break;
        case EFAULT:
          std::cout << "Path points outside accesable adress space" << std::endl;
          break;
        case EIO:
          std::cout << "IO error" << std::endl;
          break;
        case ELOOP:
          std::cout << "Too many symbolic loops" << std::endl;
          break;
        case ENAMETOOLONG:
          std::cout << "Path is too long" << std::endl;
          break;
        case ENOENT:
          std::cout << "Path doesn't exist" << std::endl;
          break;
        case ENOTDIR:
          std::cout << "Path isn't a dir" << std::endl;
          break;

        default:
            std::cout << "Unknown error" << std::endl;
            break;
      }
      return 1;
    }
    return 1;
  } else if(strcmp(command, "alias") == 0){
    if(arglist_size < 2){
      std::cout << "[USAGE] Alias originalName:substituteName" << std::endl;
      return 1;
    }
    std::string strArg(arglist[1]);
    int numOfSpaces = std::count(strArg.begin(), strArg.end(), ':');
    if(numOfSpaces){
      std::vector<std::string> aliasPair = Split(strArg, ':');
      createAlias(aliasPair.at(0), aliasPair.at(1));
      return 1;
    } else {
      std::cout << "[USAGE] Alias originalName:substituteName" << std::endl;
      return 1;
    }
  }
  return 0;
}

char** conv(std::vector<std::string> source){
  char ** dest = new char*[source.size() + 1];
  for(int i = 0; i < source.size(); i++) dest[i] = (char *)source.at(i).c_str();
  dest[source.size()] = NULL;
  return dest;
}


std::map<std::string, std::string> alias(std::string file){
  std::map<std::string, std::string> aliasPair;
  std::string line;
  std::ifstream aliasFile;
  aliasFile.open(file);
  if(aliasFile.is_open()){
    while(getline(aliasFile, line)){
      auto pair = Split(line, ':');
      aliasPair.insert(std::make_pair(pair.at(0), pair.at(1)));
    }
  } else {
    std::cout << "Error: Cannot open alias file\n";
  }
  return aliasPair;
}
std::string replaceAll(std::string data, std::map <std::string, std::string> dict){
  for(std::pair <std::string, std::string> entry : dict){
      size_t start_pos = data.find(entry.first);
      while(start_pos != std::string::npos){
        data.replace(start_pos, entry.first.length(),entry.second);
        start_pos = data.find(entry.first, start_pos + entry.second.size());
      }

  }
  return data;
}
bool createAlias(std::string first, std::string second){
    std::ofstream aliasFile;
    aliasFile.open(ALIASFILE, std::ios_base::app);
    if(aliasFile.is_open()){
      aliasFile << first << ":"<< second << std::endl;
      return true;
    } else return false;

}

Ho una shell che ho codificato in c ++ su una distribuzione Fedora Linux. Gradirei miglioramenti generali su come migliorare il codice, ma apprezzerei soprattutto commenti sulla leggibilità del codice

Answer 1

Ci sono diversi miglioramenti che puoi fare per questo codice usando solo le classi e le funzioni della libreria standard c ++.

1. Non utilizzare #include <bits/stdc++.h>

Non è garantito che questo file di intestazione esista ed è un interno specifico del compilatore. L'utilizzo di tale renderà il codice meno portabile. Vengono fornite
solo le #includeintestazioni per le classi e le funzioni che si desidera utilizzare dalla libreria standard c ++.
Puoi leggere di più sulle possibili conseguenze e problemi qui: Perché non dovrei #include ?

Inoltre, non #includefile di intestazione in cui non usi nulla (ad esempio #include <filesystem>).

2. Non utilizzare le funzioni della libreria c per la manipolazione delle stringhe

Ad esempio, il tuo codice per costruire la promptvariabile può essere drasticamente semplificato semplicemente usando std::stringinvece di char*:

char path[100];
getcwd(path,100);
std::string prompt = "$[" + std::string(path) + "]:";

Inoltre puoi semplicemente scrivere

if(command == "quit"){

supponiamo che tu usi const std::string&come tipo per il commandparametro.

3. Non è necessario allocare array di char*variabili per passarli alle execxy()funzioni

Ho appena creato un al std::vector<const char*>posto della tua conv()funzione:

void Execute(const std::string& command, const std::vector<std::string>& args) {
  std::vector<const char*> cargs;
  pid_t pid;

  for(auto sarg : args) {
      cargs.append(sarg.data());
  }
  cargs.append(nullptr);

  //Creates a new proccess
  if ((pid = fork()) < 0) {
    std::cout << "Error: Cannot create new process" << std::endl;
    exit(-1);
  } else if (pid == 0) {
    //Executes the command
    if (execvp(command.data(), cargs.data()) < 0) {
      std::cout << "Could not execute command" << std::endl;
      exit(-1);
    } else {
      sleep(2);
    }
  }
  //Waits for command to finish
  if (waitpid(pid, NULL, 0) != pid) {
    std::cout << "Error: waitpid()";
    exit(-1);
  }
}

In tal caso in cui si utilizzano puntatori a dati grezzi ottenuti std::string::data(), ad esempio , da assicurarsi che la durata delle variabili sottostanti duri per tutto il loro utilizzo, ad esempio nelle funzioni di libreria C.

Come regola generale:
evita di gestire personalmente la memoria utilizzando newed deleteesplicitamente. Piuttosto usa un contenitore standard c ++ o almeno puntatori intelligenti .

4. Non è necessario un confronto esplicito per i boolvalori

Modificare

if(BuiltInCom(com, arglist, parsed_string.size()) == 0){

per

if(!BuiltInCom(com, arglist, parsed_string.size())){

Usa anche falsee trueinvece delle conversioni implicite da int 0e 1letterali.

5. Utilizzare conste passare per riferimento per i parametri quando possibile

Utilizzare constse non è necessario modificare il parametro.
Usa pass by reference ( &) se vuoi evitare copie non necessarie fatte per tipi non banali.

Puoi vedere come nell'esempio Execute()che ho dato sopra.

Lo stesso vale per esempio

std::string replaceAll(std::string data, std::map <std::string, std::string> dict);

questo dovrebbe essere

std::string& replaceAll(std::string& data, const std::map <std::string, std::string>& dict);

Answer 2

Formattazione.

Questo è un grande muro di testo. È necessario suddividere le cose in sezioni logiche per renderlo più facile da leggere. Aggiungi un po 'di spazio verticale tra le sezioni per renderlo più facile da leggere.

---

Hai un sacco di #include. È bello ordinarli. Puoi scegliere qualsiasi modo per ordinarlo purché sia ​​logico e faciliti la consultazione delle persone.

Faccio da più specifico a più generale.

 #include "HeaderFileForThisSource.h"
 #include "HeaderFileForOtherClassesInThisProject"
 ...
 #include <C++ Librries>
 ...
 #include <C Librries>
 ...
 #include <Standard C++ Header Files>
 ..
 #include <C standard Libraries>
 ...

Altri li elencano in ordine alfabetico.

Non sono sicuro di cosa sia meglio, ma un po 'di logica nell'ordine sarebbe carino.

---

Questo è davvero difficile da leggere. Non riesco a vedere i nomi delle funzioni nel mare di testo.

std::vector<std::string> Split(std::string input, char delim);
void Execute(const char *command, char *arglist[]);
std::map<std::string, std::string> alias(std::string file);
bool BuiltInCom(const char *command, char *arglist[],int arglist_size);
char** conv(std::vector<std::string> source);
bool createAlias(std::string first, std::string sec);
std::string replaceAll(std::string data, std::map <std::string, std::string> dict);

Con un uso giudizioso usinge un po 'di riordino puoi renderlo davvero facile da usare.

using  Store = std::vector<std::string>;
using  Map   = std::map<std::string, std::string>;
using  CPPtr = char**;

Store       Split(std::string input, char delim);
void        Execute(const char *command, char *arglist[]);
Map         alias(std::string file);
bool        BuiltInCom(const char *command, char *arglist[],int arglist_size);
CPPtr       conv(std::vector<std::string> source);
bool        createAlias(std::string first, std::string sec);
std::string replaceAll(std::string data, std::map <std::string, std::string> dict);

Codice

Le "variabili" globali sono una cattiva idea.

std::string USERDIR = getenv("HOME");
std::string ALIASFILE = USERDIR+"/shell/.alias";

Imposta questo in main(). È quindi possibile passarli come parametri o aggiungerli a un oggetto.

È possibile avere uno stato statico immutabile nell'ambito globale. Questo è per cose come le costanti.

---

Rendilo facile da leggere.

  while (1) {

Questo sarebbe meglio come:

  while(true) {

---

Non utilizzare buffer di dimensioni fisse in cui l'utente potrebbe inserire stringhe di lunghezza arbitraria. Il C ++ ha la capacità std::stringdi gestire questo tipo di situazione.

    char path[100];
    getcwd(path, 100);

    // Rather
    std::string  path = std::filesystem::current_path().string();

---

Non utilizzare numeri magici nel codice:

    char prompt[110] = "$[";

Perché un 110? Metti i numeri magici in costanti con nome

    // Near the top of the programe with all other constants.
    // Then you can tune your program without having to search for the constants.
    static std::size_t constepxr bufferSize = 110;

    .....
    char buffer[bufferSize];

---

Dovrebbe usare std :: string qui

    strcat(prompt, path);
    strcat(prompt,"]: ");

Le vecchie funzioni della stringa C non sono sicure.

---