patterncppMinor
Bash command helper in C++
Viewed 0 times
helpercommandbash
Problem
I'm writing a program to help with remembering complex bash commands. On invoking the program, it asks for a description of the desired operation, e.g., "increase volume" or "find orphaned packages", and displays the commands matching the input ordered by closest match.
Matching is determined by splitting the command text into a string vector, then the description, and combining these with a list of additional keywords to compare with the input via
This is my first C++ program so I've probably made plenty of mistakes. A couple things I'm still unclear on: when to pass arguments by reference, when to use
```
#include
#include
#include
#include
#include
#include
struct Command {
std::string text;
std::string description;
std::vector addl_keywords;
};
struct FoundCommand {
std::vector::size_type keywords_found;
Command command;
FoundCommand(std::vector::size_type keywords_found, Command command)
: keywords_found {keywords_found}, command {command} {}
bool operator other.keywords_found;
}
bool operator > (FoundCommand other) {
return keywords_found Split(const std::string& s, char delim = ' ') {
std::stringstream ss {s};
std::string part;
std::vector parts;
while (getline(ss, part, delim))
parts.push_back(part);
return parts;
}
std::vector Union(std::vector v1,
std::vector v2) {
std::vector result;
result.reserve(v1.size() + v2.size());
result.insert(result.end(), v1.begin(), v1.end());
result.insert(result.end(), v2.begin(), v2.end());
return result;
}
std::vector Union(std::vector v1,
std::vector v2,
std::vector v3) {
return Union(Union(v1, v2), v3);
}
std::vector Intersect(std::vector& v1,
std::vector& v2) {
std::sort(v1.begin(), v1.end());
Matching is determined by splitting the command text into a string vector, then the description, and combining these with a list of additional keywords to compare with the input via
std::set_intersection. This is currently case sensitive, which I plan to change.This is my first C++ program so I've probably made plenty of mistakes. A couple things I'm still unclear on: when to pass arguments by reference, when to use
const and static, and when to use pointers.```
#include
#include
#include
#include
#include
#include
struct Command {
std::string text;
std::string description;
std::vector addl_keywords;
};
struct FoundCommand {
std::vector::size_type keywords_found;
Command command;
FoundCommand(std::vector::size_type keywords_found, Command command)
: keywords_found {keywords_found}, command {command} {}
bool operator other.keywords_found;
}
bool operator > (FoundCommand other) {
return keywords_found Split(const std::string& s, char delim = ' ') {
std::stringstream ss {s};
std::string part;
std::vector parts;
while (getline(ss, part, delim))
parts.push_back(part);
return parts;
}
std::vector Union(std::vector v1,
std::vector v2) {
std::vector result;
result.reserve(v1.size() + v2.size());
result.insert(result.end(), v1.begin(), v1.end());
result.insert(result.end(), v2.begin(), v2.end());
return result;
}
std::vector Union(std::vector v1,
std::vector v2,
std::vector v3) {
return Union(Union(v1, v2), v3);
}
std::vector Intersect(std::vector& v1,
std::vector& v2) {
std::sort(v1.begin(), v1.end());
Solution
First, let me say that this is not a bad program from somebody who is new to C++. You seem to have successfully avoided many of the common pitfalls, so good job on that! To help you further develop, here are some things that may help you improve your code, starting with answers to your specific questions.
Understand when to use references
The primary difference between a reference and a pointer is that a reference cannot be
Use
One way to remember how to use
Use existing library functions
As @mrm observed, you're using
However, we don't really need
Note, too, that I'm using
Use objects more effectively
The program requires the list of commands and keywords to operate, but they're constructed in a rather inefficient way. First, there is the
This uses the command line input to create a vector of strings, rather than prompting for input, but that's a minor point. The significant change is that there is presumed to be a
Eliminate global variables
There is really no compelling reason here to have global variables, even when they're as relatively benign as your
Promote your
Right now, there's only loose association between the functions and the data structures on which they operate. This could be made more clear by the use of objects, and more safe by making those objects
Right now, within the various functions, they reach in directly into the member data items and directly manipulate them. This is not good object design because it places no limits on how the objects are used. Really, we could make it into a
First, we can make all data items
Next, a simple constructor:
Next, a simple way to convert a particular command into a string for output:
Finally, a function that takes a single word as input, and return
```
bool find(const std::string &word) const {
if (text.find(word) != std::string::npos ||
description.find(word) != std::string::npos
Understand when to use references
The primary difference between a reference and a pointer is that a reference cannot be
nullptr. So if the function you're calling couldn't work with a nullptr (or doesn't check for one), as with your Intersect() routine, then it should be a reference as you've correctly done. Use
const where practicalOne way to remember how to use
const effectively is to think of it this way: try to use const everywhere, and then only omit it where you must. So, for example, your commands structure is never changed and is therefore const as you have correctly done it. However, with some additional changes, you can both make your program more efficient and use const more often. The following suggestions help with that.Use existing library functions
As @mrm observed, you're using
set_intersection already, so why not use set_union as well instead of defining your own Union? Also, in main, we currently have this:std::string input;
getline(std::cin, input);
std::vector input_keywords = Split(input);However, we don't really need
Split in this case. I would do it like this instead:using namespace std;
vector input_keywords;
copy(istream_iterator(cin), istream_iterator(),
back_inserter(input_keywords));Note, too, that I'm using
using namespace std within main() (and therefore limiting it to within that scope) rather than at the top of the file outside main which is a poor practice.Use objects more effectively
The program requires the list of commands and keywords to operate, but they're constructed in a rather inefficient way. First, there is the
Command structure, and then the commands[] simple array, but then to actually use this within FindCommands, each entry is individually converted into an array of words every time the function is called, which is not very efficient. Further, each string vector is then sorted within the call to Intersect and then discarded. That's a lot of redundant processing! Instead, I'd suggest creating a single class of Commands and creating member functions for that class. With a re-engineered class, the main function might look like this:int main(int argc, char *argv[]) {
for (const auto &item: commands.find(&argv[0], &argv[argc])) {
std::cout << item << std::endl;
}
}This uses the command line input to create a vector of strings, rather than prompting for input, but that's a minor point. The significant change is that there is presumed to be a
commands object that has a find member function that returns something that has an iterator. How we might go about writing this is described in the following items.Eliminate global variables
There is really no compelling reason here to have global variables, even when they're as relatively benign as your
static const commands array. I'd be inclined instead to either put it as an automatic variable within main or, if you really must have a global variable, at least wrap it inside a usable object or namespace.Promote your
structs to classesRight now, there's only loose association between the functions and the data structures on which they operate. This could be made more clear by the use of objects, and more safe by making those objects
class rather than struct types. Let's first look at your existing Command struct:struct Command {
std::string text;
std::string description;
std::vector addl_keywords;
};Right now, within the various functions, they reach in directly into the member data items and directly manipulate them. This is not good object design because it places no limits on how the objects are used. Really, we could make it into a
class and write just three relatively simple member functions. First, we can make all data items
private:class Command {
private:
std::string text;
std::string description;
std::vector addl_keywords;Next, a simple constructor:
public:
Command(const std::string &mytext,
const std::string &mydesc,
const std::vector kwds)
: text{mytext},
description{mydesc},
addl_keywords{kwds}
{}Next, a simple way to convert a particular command into a string for output:
std::string toString() const {
return description + ": " + text;
}Finally, a function that takes a single word as input, and return
true if that word is somewhere within the data items for this command:```
bool find(const std::string &word) const {
if (text.find(word) != std::string::npos ||
description.find(word) != std::string::npos
Code Snippets
std::string input;
getline(std::cin, input);
std::vector<std::string> input_keywords = Split(input);using namespace std;
vector<string> input_keywords;
copy(istream_iterator<string>(cin), istream_iterator<string>(),
back_inserter(input_keywords));int main(int argc, char *argv[]) {
for (const auto &item: commands.find(&argv[0], &argv[argc])) {
std::cout << item << std::endl;
}
}struct Command {
std::string text;
std::string description;
std::vector<std::string> addl_keywords;
};class Command {
private:
std::string text;
std::string description;
std::vector <std::string> addl_keywords;Context
StackExchange Code Review Q#110776, answer score: 5
Revisions (0)
No revisions yet.