Code Review Asked by The Masked Rebel on February 14, 2021
#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 filen";
}
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;
}
I have a shell that I’ve coded in c++ on a Fedora Linux distro. I would welcome general improvements on how to make the code better, but I’d especially welcome comments on the code’s readability
This is one large wall of text. You need to split things up into logical sections to make this easyier to read. Add some vertical space between section to make this easier to read.
You have a bunch of #include. Its nice to order them. You can choose any way to order it as long as its logical and makes it easy for people to look through.
I do most specific to most general.
#include "HeaderFileForThisSource.h"
#include "HeaderFileForOtherClassesInThisProject"
...
#include <C++ Librries>
...
#include <C Librries>
...
#include <Standard C++ Header Files>
..
#include <C standard Libraries>
...
Others list them alphabetically.
Not sure what is best but some logic to the ordering would be nice.
This is really hard to read. I can't see the function names in the sea of text.
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);
With some judisious use of using
and some tidying you can make that really easy to use.
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);
Global "Variables" are a bad idea.
std::string USERDIR = getenv("HOME");
std::string ALIASFILE = USERDIR+"/shell/.alias";
Set this up in main()
. You can then either pass these around as parameters or add them to an object.
You can have static immutable state in the global scope. This is for things like constants.
Make it easy to read.
while (1) {
This would be better as:
while(true) {
Don't use fixed size buffers where the user could input arbitrary length strings. C++ has the std::string
to handle this kind of situation.
char path[100];
getcwd(path, 100);
// Rather
std::string path = std::filesystem::current_path().string();
Don't use magic numbers in your code:
char prompt[110] = "$[";
Why a 110? Put the magic numbers into named constants
// 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];
Should be using std::string here
strcat(prompt, path);
strcat(prompt,"]: ");
The old C string functions are not safe.
Answered by Martin York on February 14, 2021
There are several improvements you can do for this code using just c++ standard library classes and functions.
#include <bits/stdc++.h>
It's not guaranteed that this header file exists, and is a compiler specific internal. Using such will make your code less portable.
Only #include
headers provided for the classes and functions you want to use from the c++ standard library.
You can read more about the possible consequences and problems here: Why should I not #include <bits/stdc++.h>?
As well don't #include
header files where you don't use anything from them (e.g. #include <filesystem>
).
E.g. your code to build the prompt
variable can be drastically simplified by just using std::string
instead of char*
:
char path[100];
getcwd(path,100);
std::string prompt = "$[" + std::string(path) + "]:";
Also you can simply write
if(command == "quit"){
supposed you use const std::string&
as type for the command
parameter.
char*
variables to pass them to execxy()
functionsJust built a std::vector<const char*>
instead of your conv()
function:
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);
}
}
In such case where you use raw data pointers obtained by e.g. std::string::data()
, be sure that the lifetimes of the underlying variables last throughout their use in e.g. C library functions.
As a general rule of thumb:
Avoid doing memory management yourself using new
and delete
explicitely.
Rather use a c++ standard container or at least smart pointers.
bool
valuesChange
if(BuiltInCom(com, arglist, parsed_string.size()) == 0){
to
if(!BuiltInCom(com, arglist, parsed_string.size())){
Also use false
and true
instead of the implicit conversions from int
0
and 1
literals.
const
and pass by reference for parameters whenever possibleUse const
if you don't need to change the parameter.
Use pass by reference (&
) if you want to avoid unnecessary copies made for non trivial types.
You can see how in the example of Execute()
I've given above.
Same goes for example
std::string replaceAll(std::string data, std::map <std::string, std::string> dict);
this should be
std::string& replaceAll(std::string& data, const std::map <std::string, std::string>& dict);
Answered by πάντα ῥεῖ on February 14, 2021
Get help from others!
Recent Questions
Recent Answers
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP