c ++ เชลล์สำหรับลินุกซ์

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;

}

ฉันมีเชลล์ที่ฉันได้เขียนโค้ด c ++ บน Fedora Linux distro ฉันยินดีต้อนรับการปรับปรุงทั่วไปเกี่ยวกับวิธีทำให้โค้ดดีขึ้น แต่โดยเฉพาะอย่างยิ่งฉันยินดีรับความคิดเห็นเกี่ยวกับความสามารถในการอ่านของโค้ด

Answer 1

มีการปรับปรุงหลายอย่างที่คุณสามารถทำได้สำหรับโค้ดนี้โดยใช้คลาสและฟังก์ชันไลบรารีมาตรฐาน c ++ เท่านั้น

1. อย่าใช้ #include <bits/stdc++.h>

ไม่รับประกันว่าไฟล์ส่วนหัวนี้มีอยู่และเป็นคอมไพเลอร์เฉพาะภายใน การใช้เช่นนี้จะทำให้โค้ดของคุณพกพาได้น้อยลง
เฉพาะ#includeส่วนหัวที่จัดเตรียมไว้สำหรับคลาสและฟังก์ชันที่คุณต้องการใช้จากไลบรารีมาตรฐาน c ++
คุณสามารถอ่านเพิ่มเติมเกี่ยวกับผลที่ตามมาและปัญหาได้ที่นี่ทำไมฉันจึงไม่ # รวม

และอย่าใช้#includeไฟล์ส่วนหัวที่คุณไม่ได้ใช้อะไรเลย (เช่น#include <filesystem>)

2. อย่าใช้ฟังก์ชันไลบรารี c สำหรับการปรับแต่งสตริง

เช่นโค้ดของคุณในการสร้างpromptตัวแปรสามารถทำให้ง่ายขึ้นอย่างมากโดยใช้std::stringแทนchar*:

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

นอกจากนี้คุณสามารถเขียน

if(command == "quit"){

คุณควรใช้const std::string&เป็นประเภทสำหรับcommandพารามิเตอร์

3. คุณไม่จำเป็นต้องจัดสรรอาร์เรย์ของchar*ตัวแปรเพื่อส่งผ่านไปยังexecxy()ฟังก์ชัน

เพิ่งสร้างstd::vector<const char*>แทนconv()ฟังก์ชันของคุณ:

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

ในกรณีเช่นนี้ที่คุณใช้พอยน์เตอร์ข้อมูลดิบที่ได้รับจากเช่นstd::string::data()ตรวจสอบให้แน่ใจว่าอายุการใช้งานของตัวแปรที่อยู่ภายใต้นั้นคงอยู่ตลอดการใช้งานเช่นฟังก์ชันไลบรารี C

ตามหลักการทั่วไป:
หลีกเลี่ยงการจัดการหน่วยความจำด้วยตัวเองโดยใช้newและdeleteอย่างชัดเจน แทนที่จะใช้ C ++ ภาชนะมาตรฐานหรืออย่างน้อยก็ชี้สมาร์ท

4. คุณไม่จำเป็นต้องมีการเปรียบเทียบboolค่าอย่างชัดเจน

เปลี่ยน

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

ถึง

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

ใช้falseและtrueแทนการแปลงโดยนัยจากint 0และตาม1ตัวอักษร

5. ใช้constและส่งต่อโดยอ้างอิงสำหรับพารามิเตอร์ทุกครั้งที่ทำได้

ใช้constหากคุณไม่จำเป็นต้องเปลี่ยนพารามิเตอร์
ใช้ pass by reference ( &) หากคุณต้องการหลีกเลี่ยงการทำสำเนาที่ไม่จำเป็นสำหรับประเภทที่ไม่สำคัญ

คุณสามารถดูได้จากตัวอย่างที่Execute()ฉันให้ไว้ข้างต้น

เช่นเดียวกัน

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

สิ่งนี้ควรจะเป็น

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

Answer 2

การจัดรูปแบบ

นี่คือกำแพงข้อความขนาดใหญ่หนึ่งแผ่น คุณต้องแยกสิ่งต่างๆออกเป็นส่วนที่เป็นตรรกะเพื่อให้อ่านง่ายขึ้น เพิ่มช่องว่างแนวตั้งระหว่างส่วนเพื่อให้อ่านง่ายขึ้น

---

คุณมี # รวมอยู่มากมาย ยินดีที่ได้สั่งซื้อ คุณสามารถเลือกวิธีใดก็ได้ในการสั่งซื้อตราบเท่าที่มีเหตุผลและทำให้ผู้คนมองผ่านได้ง่าย

ฉันเจาะจงมากที่สุดสำหรับคนทั่วไปส่วนใหญ่

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

คนอื่น ๆ จะแสดงรายการตามตัวอักษร

ไม่แน่ใจว่าอะไรดีที่สุด แต่ตรรกะบางอย่างในการสั่งซื้อจะดี

---

นี่อ่านยากจริงๆ ฉันไม่เห็นชื่อฟังก์ชันในส่วนของข้อความ

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

ด้วยการใช้อย่างรอบคอบusingและการจัดระเบียบบางอย่างคุณสามารถทำให้ใช้งานได้ง่ายมาก

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

รหัส

"ตัวแปร" ทั่วโลกเป็นความคิดที่ไม่ดี

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

ตั้งค่านี้ในmain(). จากนั้นคุณสามารถส่งผ่านสิ่งเหล่านี้เป็นพารามิเตอร์หรือเพิ่มลงในวัตถุ

คุณสามารถมีสถานะคงที่ไม่เปลี่ยนรูปได้ในขอบเขตส่วนกลาง สำหรับสิ่งต่างๆเช่นค่าคงที่

---

ทำให้อ่านง่าย

  while (1) {

สิ่งนี้จะดีกว่าเมื่อ:

  while(true) {

---

อย่าใช้บัฟเฟอร์ขนาดคงที่ซึ่งผู้ใช้สามารถป้อนสตริงความยาวโดยพลการ C ++ std::stringสามารถจัดการกับสถานการณ์แบบนี้ได้

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

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

---

อย่าใช้เลขวิเศษในรหัสของคุณ:

    char prompt[110] = "$[";

ทำไมต้องเป็น 110? ใส่ตัวเลขมหัศจรรย์ลงในค่าคงที่ที่ตั้งชื่อ

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

---

ควรใช้ std :: string ที่นี่

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

ฟังก์ชันสตริง C แบบเก่าไม่ปลอดภัย

---