HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Bash command helper in C++

Submitted by: @import:stackexchange-codereview··
0
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 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 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 practical

One 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 classes

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 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.