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

}

Tengo un shell que he codificado en c ++ en una distribución de Fedora Linux. Agradecería mejoras generales sobre cómo mejorar el código, pero agradecería especialmente los comentarios sobre la legibilidad del código.

Answer 1

Hay varias mejoras que puede hacer para este código usando solo clases y funciones de biblioteca estándar de C ++.

1. No use #include <bits/stdc++.h>

No se garantiza que este archivo de encabezado exista y sea un interno específico del compilador. Usarlo hará que su código sea menos portátil.
Solo se #includeproporcionan encabezados para las clases y funciones que desea utilizar de la biblioteca estándar de c ++.
Puede leer más sobre las posibles consecuencias y problemas aquí: ¿Por qué no debería # incluir ?

Tampoco #includelos archivos de encabezado donde no use nada de ellos (por ejemplo #include <filesystem>).

2. No use las funciones de la biblioteca c para manipulaciones de cadenas

Por ejemplo, su código para construir la promptvariable se puede simplificar drásticamente simplemente usando en std::stringlugar de char*:

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

También puedes simplemente escribir

if(command == "quit"){

se supone que usa const std::string&como tipo para el commandparámetro.

3. No es necesario asignar matrices de char*variables para pasarlas a execxy()funciones

Acabo de construir un en std::vector<const char*>lugar de tu conv()función:

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

En el caso de que utilice punteros de datos brutos obtenidos por ejemplo std::string::data(), asegúrese de que la vida útil de las variables subyacentes dure a lo largo de su uso en, por ejemplo, funciones de la biblioteca C.

Como regla general:
evitar hacer la gestión de memoria por sí mismo utilizando newy deleteexplícitamente. En su lugar, use un contenedor estándar de C ++ o al menos punteros inteligentes .

4. No necesita una comparación explícita de boolvalores

Cambio

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

a

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

También use falsey en truelugar de las conversiones implícitas de int 0y 1literales.

5. Utilice consty pase por referencia los parámetros siempre que sea posible

Úselo constsi no necesita cambiar el parámetro.
Utilice pasar por referencia ( &) si desea evitar copias innecesarias realizadas para tipos no triviales.

Puede ver cómo en el ejemplo Execute()que he dado anteriormente.

Lo mismo ocurre por ejemplo

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

esto debería ser

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

Answer 2

Formateo.

Esta es una gran pared de texto. Necesita dividir las cosas en secciones lógicas para que sea más fácil de leer. Agregue algo de espacio vertical entre las secciones para que sea más fácil de leer.

---

Tienes un montón de #include. Es bueno pedirlos. Puede elegir cualquier forma de ordenarlo siempre que sea lógico y facilite que las personas lo revisen.

Hago de lo más específico a lo más general.

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

Otros los enumeran alfabéticamente.

No estoy seguro de qué es lo mejor, pero sería bueno tener algo de lógica en el pedido.

---

Esto es realmente difícil de leer. No puedo ver los nombres de las funciones en el mar de texto.

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 juicioso usingy un poco de limpieza, puede hacer que sea realmente fácil de usar.

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

Código

Las "variables" globales son una mala idea.

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

Configure esto en main(). A continuación, puede pasarlos como parámetros o agregarlos a un objeto.

Puede tener un estado inmutable estático en el ámbito global. Esto es para cosas como constantes.

---

Haz que sea fácil de leer.

  while (1) {

Esto sería mejor si:

  while(true) {

---

No use búferes de tamaño fijo donde el usuario podría ingresar cadenas de longitud arbitraria. C ++ tiene la std::stringcapacidad para manejar este tipo de situaciones.

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

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

---

No uses números mágicos en tu código:

    char prompt[110] = "$[";

¿Por qué un 110? Pon los números mágicos en constantes nombradas

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

---

Debería estar usando std :: string aquí

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

Las antiguas funciones de cadena C no son seguras.

---