c ++ Shell für 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;

}

Ich habe eine Shell, die ich in c ++ auf einer Fedora Linux-Distribution codiert habe. Ich würde allgemeine Verbesserungen bei der Verbesserung des Codes begrüßen, aber ich würde besonders Kommentare zur Lesbarkeit des Codes begrüßen

Answer 1

Es gibt verschiedene Verbesserungen, die Sie für diesen Code mit nur C ++ - Standardbibliotheksklassen und -funktionen vornehmen können.

1. Nicht verwenden #include <bits/stdc++.h>

Es kann nicht garantiert werden, dass diese Header-Datei vorhanden ist und eine compilerspezifische interne Datei ist. Wenn Sie solche verwenden, wird Ihr Code weniger portabel.
Es werden nur #includeHeader für die Klassen und Funktionen bereitgestellt, die Sie aus der c ++ - Standardbibliothek verwenden möchten.
Weitere Informationen zu möglichen Konsequenzen und Problemen finden Sie hier: Warum sollte ich nicht einschließen?

Neben nicht - #includeHeader - Dateien , wo Sie nichts von ihnen (zB #include <filesystem>).

2. Verwenden Sie keine c-Bibliotheksfunktionen für Zeichenfolgenmanipulationen

Zum Beispiel kann Ihr Code zum Erstellen der promptVariablen drastisch vereinfacht werden, indem Sie einfach Folgendes std::stringverwenden char*:

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

Sie können auch einfach schreiben

if(command == "quit"){

Angenommen, Sie verwenden const std::string&als Typ für den commandParameter.

3. Sie müssen keine Arrays von char*Variablen zuweisen , um sie an execxy()Funktionen zu übergeben

Bauen Sie einfach eine std::vector<const char*>anstelle Ihrer conv()Funktion:

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

Stellen Sie in einem solchen Fall, in dem Sie Rohdatenzeiger verwenden, die z. B. von erhalten std::string::data()wurden, sicher, dass die Lebensdauer der zugrunde liegenden Variablen während ihrer Verwendung in z. B. C-Bibliotheksfunktionen gültig ist.

Als allgemeine Faustregel gilt:
Vermeiden Sie es, die Speicherverwaltung selbst newund deleteexplizit durchzuführen. Verwenden Sie lieber einen C ++ - Standardcontainer oder zumindest intelligente Zeiger .

4. Sie benötigen keinen expliziten Vergleich für boolWerte

Veränderung

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

zu

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

Verwenden Sie auch falseund trueanstelle der impliziten Konvertierungen von int 0und 1Literalen.

5. Verwenden constund übergeben Sie die Parameter nach Möglichkeit als Referenz

Verwenden constSie diese Option, wenn Sie den Parameter nicht ändern müssen.
Verwenden Sie pass by reference ( &), wenn Sie unnötige Kopien für nicht triviale Typen vermeiden möchten.

Sie können sehen, wie in dem Beispiel von Execute()ich oben gegeben habe.

Gleiches gilt zum Beispiel

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

das sollte sein

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

Answer 2

Formatierung.

Dies ist eine große Textwand. Sie müssen die Dinge in logische Abschnitte aufteilen, um das Lesen zu vereinfachen. Fügen Sie zwischen den Abschnitten einen vertikalen Abstand hinzu, um die Lesbarkeit zu verbessern.

---

Du hast ein paar #include. Es ist schön, sie zu bestellen. Sie können eine beliebige Art der Bestellung wählen, solange dies logisch ist und das Durchsuchen erleichtert wird.

Ich mache am spezifischsten für die allgemeinsten.

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

Andere listen sie alphabetisch auf.

Ich bin mir nicht sicher, was am besten ist, aber eine Logik für die Bestellung wäre schön.

---

Das ist wirklich schwer zu lesen. Ich kann die Funktionsnamen im Textmeer nicht sehen.

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

Mit etwas umsichtigem Gebrauch usingund etwas Aufräumen können Sie das wirklich einfach machen.

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

Globale "Variablen" sind eine schlechte Idee.

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

Richten Sie dies in ein main(). Sie können diese dann entweder als Parameter weitergeben oder einem Objekt hinzufügen.

Sie können einen statischen unveränderlichen Status im globalen Bereich haben. Dies ist für Dinge wie Konstanten.

---

Machen Sie es einfach zu lesen.

  while (1) {

Dies wäre besser als:

  while(true) {

---

Verwenden Sie keine Puffer mit fester Größe, in die der Benutzer Zeichenfolgen beliebiger Länge eingeben kann. C ++ muss std::stringmit solchen Situationen umgehen.

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

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

---

Verwenden Sie keine magischen Zahlen in Ihrem Code:

    char prompt[110] = "$[";

Warum eine 110? Setzen Sie die magischen Zahlen in benannte Konstanten

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

---

Sollte hier std :: string verwenden

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

Die alten C-String-Funktionen sind nicht sicher.

---