shell c ++ pour 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;

}

J'ai un shell que j'ai codé en c ++ sur une distribution Fedora Linux. Je souhaiterais des améliorations générales sur la façon d'améliorer le code, mais je serais particulièrement heureux de recevoir des commentaires sur la lisibilité du code

Answer 1

Il existe plusieurs améliorations que vous pouvez apporter à ce code en utilisant uniquement des classes et des fonctions de bibliothèque standard c ++.

1. N'utilisez pas #include <bits/stdc++.h>

Il n'est pas garanti que ce fichier d'en-tête existe et qu'il soit un interne spécifique au compilateur. Leur utilisation rendra votre code moins portable.
Seuls les en- #includetêtes fournis pour les classes et les fonctions que vous souhaitez utiliser à partir de la bibliothèque standard c ++.
Vous pouvez en savoir plus sur les conséquences et les problèmes possibles ici: Pourquoi ne devrais-je pas #include ?

De même, ne #includesupprimez pas les fichiers d'en-tête dont vous n'utilisez rien (par exemple #include <filesystem>).

2. N'utilisez pas les fonctions de la bibliothèque c pour les manipulations de chaînes

Par exemple, votre code pour construire la promptvariable peut être considérablement simplifié en utilisant simplement std::stringau lieu de char*:

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

Vous pouvez aussi simplement écrire

if(command == "quit"){

supposé que vous utilisez const std::string&comme type pour le commandparamètre.

3. Vous n'avez pas besoin d'allouer des tableaux de char*variables pour les transmettre aux execxy()fonctions

Juste construit un std::vector<const char*>au lieu de votre conv()fonction:

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);
  }
}

Dans ce cas où vous utilisez des pointeurs de données brutes obtenus par exemple std::string::data(), assurez-vous que les durées de vie des variables sous-jacentes durent tout au long de leur utilisation dans les fonctions de la bibliothèque C.

En règle générale:
évitez de gérer vous-même la mémoire en utilisant newet deleteexplicitement. Utilisez plutôt un conteneur standard C ++ ou au moins des pointeurs intelligents .

4. Vous n'avez pas besoin de comparaison explicite pour les boolvaleurs

Changement

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

à

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

Utilisez également falseet à la trueplace des conversions implicites de int 0et 1littéraux.

5. Utiliser constet passer par référence pour les paramètres chaque fois que possible

À utiliser constsi vous n'avez pas besoin de modifier le paramètre.
Utilisez pass by reference ( &) si vous voulez éviter des copies inutiles faites pour des types non triviaux.

Vous pouvez voir comment dans l'exemple Execute()que j'ai donné ci-dessus.

Il en va de même par exemple

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

ce devrait être

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

Answer 2

Mise en page.

Ceci est un grand mur de texte. Vous devez diviser les choses en sections logiques pour rendre cela plus facile à lire. Ajoutez un espace vertical entre les sections pour faciliter la lecture.

---

Vous avez un tas de #include. C'est bien de les commander. Vous pouvez choisir n'importe quel moyen de le commander, à condition que cela soit logique et que les gens puissent le consulter facilement.

Je fais du plus spécifique au plus général.

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

D'autres les énumèrent par ordre alphabétique.

Je ne sais pas ce qui est le mieux, mais une certaine logique à la commande serait bien.

---

C'est vraiment difficile à lire. Je ne vois pas les noms des fonctions dans la mer de texte.

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);

Avec une utilisation judicieuse usinget un peu de rangement, vous pouvez rendre cela vraiment facile à utiliser.

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);

Code

Les «variables» globales sont une mauvaise idée.

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

Configurez ceci dans main(). Vous pouvez ensuite les transmettre en tant que paramètres ou les ajouter à un objet.

Vous pouvez avoir un état immuable statique dans la portée globale. C'est pour des choses comme les constantes.

---

Rendez-le facile à lire.

  while (1) {

Ce serait mieux car:

  while(true) {

---

N'utilisez pas de tampons de taille fixe où l'utilisateur pourrait saisir des chaînes de longueur arbitraire. C ++ a le std::stringpour gérer ce genre de situation.

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

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

---

N'utilisez pas de nombres magiques dans votre code:

    char prompt[110] = "$[";

Pourquoi un 110? Mettez les nombres magiques dans des constantes nommées

    // 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];

---

Devrait utiliser std :: string ici

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

Les anciennes fonctions de chaîne C ne sont pas sûres.

---