Linux用の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;

}

FedoraLinuxディストリビューションでc ++でコーディングしたシェルがあります。コードを改善する方法に関する一般的な改善を歓迎しますが、コードの読みやすさに関するコメントを特に歓迎します

Answer 1

C ++標準ライブラリのクラスと関数だけを使用して、このコードに対して実行できるいくつかの改善点があります。

1.使用しないでください #include <bits/stdc++.h>

このヘッダーファイルが存在することは保証されておらず、コンパイラ固有の内部ファイルです。これを使用すると、コードの移植性が低下します。c ++標準ライブラリから使用するクラスと関数に提供されるヘッダー
のみ#include。
考えられる結果と問題について詳しくは、こちらをご覧ください。＃include を使用しないのはなぜですか？

また、#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します。trueint 01

5.const可能な場合はいつでも、パラメータを使用して参照渡しします

constパラメータを変更する必要がない場合に使用します。自明でないタイプに対して作成された不要なコピーを回避したい場合は
、参照渡し（&）を使用してください。

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

フォーマット。

これは1つの大きなテキストの壁です。これを読みやすくするために、物事を論理セクションに分割する必要があります。これを読みやすくするために、セクション間に垂直方向のスペースを追加します。

---

たくさんの#includeがあります。それらを注文するのはいいことです。論理的で、人々が見やすくする限り、任意の注文方法を選択できます。

私は最も一般的なものに最も具体的にします。

 #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文字列関数は安全ではありません。

---