Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am writing this program to try and get some practice at C++ and correct and proper styling.

This code is meant to take user input from a pip and then run it through the program. The user enters a pattern and that pattern is turned into regex which is compared against the input, such that it could look like this:

cat input.txt |./app "pattern %{0} end of pattern"

I was wondering if I could get some pointers as to what I am doing wrong here (what is not best practice and anything else you may see as being incorrect).

#include <iostream>
#include <sstream>
#include <string>
#include <vector>
#include "build/pcrecpp.h"

void ReadFile(std::vector<string> &lines_in_file);
bool ValidateArguments(int arguments);
std::string RegexConverter(std::string &user_specified_pattern);
void SplitInputLine(std::string &user_specified_pattern, std::vector<string> &users_pattern_split);
void TokenValidator(std::vector<string>& users_pattern_split, std::string& users_converted_pattern);
std::string TokenToRegex(std::string word_in_user_specified_pattern);
bool FindMatchingLines(std::vector<string> lines_in_file, std::string user_specified_pattern);


int main(int argc, char *argv[]) {
  if (!ValidateArguments(argc)) return 1;

  std::string user_specified_pattern = argv[1];
  std::vector<string> lines_in_file;  // 2 diminsional vector containing [lines][words]

  ReadFile(lines_in_file);
  std::string users_converted_pattern = RegexConverter(user_specified_pattern);

  FindMatchingLines(lines_in_file, users_converted_pattern);
  return 0;
}

void ReadFile(std::vector<string> &lines_in_file) {
  std::stringstream input_file;
  input_file << std::cin.rdbuf();
  std::string text_from_input;

    while (std::getline(input_file, text_from_input, '\n')) {
      lines_in_file.push_back(text_from_input);
    }
}

bool ValidateArguments(int arguments) {
  if ( arguments != 2 ) {
    std::cerr << "Usage - ./alertlogic_test \"Patern\" \n";
    return false;
  }
return true;
}

std::string RegexConverter(std::string &user_specified_pattern) {
  std::vector<string> users_pattern_split;
  std::string users_converted_pattern;

  SplitInputLine(user_specified_pattern, users_pattern_split);
  TokenValidator(users_pattern_split, users_converted_pattern);

  return users_converted_pattern;
}

void SplitInputLine(std::string &user_specified_pattern, std::vector<string> &users_pattern_split) {
  char sep = ' ';

    for (size_t p = 0, q = 0; p != user_specified_pattern.npos; p = q)
      users_pattern_split.push_back(user_specified_pattern.substr(p+(p != 0),
      (q = user_specified_pattern.find(sep, p+1))-p-(p != 0)));
}

void TokenValidator(std::vector<string> &users_pattern_split, std::string &users_converted_pattern) {
  pcrecpp::RE   re("%{(.)*}");

          users_converted_pattern.append("(");              // worked tomorrow. look

    for (int i = 0; i < users_pattern_split.size(); i++) {
      if (re.PartialMatch(users_pattern_split[i])) {
        users_converted_pattern.append(TokenToRegex(users_pattern_split[i]) + " ");
      } else {
        users_converted_pattern.append(users_pattern_split[i] + " ");
      }
    }
  users_converted_pattern.erase(users_converted_pattern.find_last_not_of(" \n\r\t")+1);
            users_converted_pattern.append(")");            // llook at thi tomorrow

// std::cout << new_user_specified_pattern << "\n";
}

std::string TokenToRegex(std::string word_in_user_specified_pattern) {
  std::string converted_word;

    switch (word_in_user_specified_pattern.length()) {
      case 4:
        converted_word = "(.*)";                            // %{n}
      break;
      case 5:
        converted_word = "(.*)";                            // %{nG}
      break;
      case 6:
        converted_word = std::string("{.}*{")+word_in_user_specified_pattern[4]+"}";
      break;
      default:
        std::cerr <<  "ERROR UNKNOWN TOKEN!\n";
        exit(3);
    }
return converted_word;
}

bool FindMatchingLines(std::vector<string> lines_in_file, std::string user_specified_pattern) {
  std::string phrase;
  pcrecpp::RE re(user_specified_pattern);
  bool is_there_a_match = false;  
    for (int i = 0; i < lines_in_file.size(); ++i) {
      if (re.PartialMatch(lines_in_file[i], &phrase)) {
        std::cout << "\nA matching phrase \"" << phrase << "\" found at line " <<  i+1 <<  "\n" << 
    lines_in_file[i] << "\n";
      is_there_a_match = true;
      }
    }
    if (is_there_a_match == false) {
      std::cout << "no match found\n";
    return false;
    }
return true;
}
share|improve this question
    
You know that C++11 has regular expressions –  Loki Astari Jul 20 at 17:51
    
Yep, my practice questions ask to use PCRECPP.H though –  andrew Jul 20 at 18:31
    
    
Please do not remove the code. It has to stay intact alongside the answers. –  Jamal Jul 21 at 3:15
    
Posts with upvoted answers should not be deleted. Doing so will take away the work done by these answers to help you, which is extremely disrespectful. These posts should also be here to benefit others, not just the asker. This can be unlocked after a while, but not until you've come to understand this. –  Jamal Jul 21 at 5:39

3 Answers 3

up vote 4 down vote accepted

General Style

My main problem with the code as it stands is that it is messy (and thus hard to read). The contributing factors are:

Inconsistent Indentation:

  std::string text_from_input;

    while (std::getline(input_file, text_from_input, '\n')) {
      lines_in_file.push_back(text_from_input);
    }

Inconsistent use of braces '{' and '}'

// One liner braced here.
    while (std::getline(input_file, text_from_input, '\n')) {
      lines_in_file.push_back(text_from_input);
    }
// But not here
    for (size_t p = 0, q = 0; p != user_specified_pattern.npos; p = q)
      users_pattern_split.push_back(user_specified_pattern.substr(p+(p != 0),
      (q = user_specified_pattern.find(sep, p+1))-p-(p != 0)));

Inconsistent use of white space to separate logical chunks.

// Two variables nothing to do with each other one initialized the other not. 
  std::string user_specified_pattern = argv[1];
  std::vector<string> lines_in_file;  // 2 diminsional vector containing [lines]


// Followed by calls (in unexpected order.
  ReadFile(lines_in_file);
  std::string users_converted_pattern = RegexConverter(user_specified_pattern);

Your code should be self commenting (variable and function names)
But your code seems to not always do what the name describes (sometimes it does).

Comments on code

Header file inclusion. Go from most specific to most general. This helps you avoid situations where you include header files in one header files that hide the fact that you missed the inclusion in another header file.

// Thus you have these in reverse order.
// The build/pcrecpp.h should be first as it is application specific.

#include <iostream>
#include <sstream>
#include <string>
#include <vector>
#include "build/pcrecpp.h"

Though there is nothing wrong here. It is a bit messy. Source is designed to be human readable. So make it easy to read.

void ReadFile(std::vector<string> &lines_in_file);
bool ValidateArguments(int arguments);
std::string RegexConverter(std::string &user_specified_pattern);
void SplitInputLine(std::string &user_specified_pattern, std::vector<string> &users_pattern_split);
void TokenValidator(std::vector<string>& users_pattern_split, std::string& users_converted_pattern);
std::string TokenToRegex(std::string word_in_user_specified_pattern);
bool FindMatchingLines(std::vector<string> lines_in_file, std::string user_specified_pattern);

Declare variables as close to the point of use as you can. Don't be scared of return by value. The optimizer is going to eliminate the copy out of the function for you (look up RVO NRVO). Also out parameters are a pain when maintaining the code. Its easy to spot when writing but noticing out values at the call point several months later is a pain (much easier to use return values).

  // So this 

  std::string user_specified_pattern = argv[1];
  std::vector<string> lines_in_file;  // 2 diminsional vector containing [lines][words]

  ReadFile(lines_in_file);
  std::string users_converted_pattern = RegexConverter(user_specified_pattern);


  // Is a lot easier to read as:
  std::vector<string> lines_in_file   = ReadFile(std::cin);     // 2 diminsional vector containing [lines][words]    
  std::string users_converted_pattern = RegexConverter(argv[1]);

OK you are reading a file. But that sorts of limits reuse. Why not read from a stream (then you can pass std::cin as the stream but also any other type of stream.

void ReadFile(std::vector<string> &lines_in_file) {

Copy the whole stream to another stream. Seems like a bit of a waste. Then read a line at a time. Why not just read a line at a time from the original stream std::cin?

std::vector<std::string> ReadFile(std::istream& input) {
{
    std::vector<std::string>   result;
    std::string                text_from_input;

    while (std::getline(input, text_from_input, '\n')) {
      lines_in_file.push_back(text_from_input);
    }
    return result;
}

But now looking at that we see, a loop over a stream. There must be an algorithm for that. We could just replace the while() with std::copy() (I'll define Line in a second).

std::vector<std::string> ReadFile(std::istream& input) {
{
    std::vector<std::string>   result;

    std::copy(std::istream_iterator<Line>(input),  // begin
              std::istream_iterator<Line>(),       // end
              std::back_inserter(result));         // 
    return result;
}

But we can take this a step further as the vector can take two iterators as part of its constructor.

std::vector<std::string> ReadFile(std::istream& input) {
{
    std::vector<std::string>   result(std::istream_iterator<Line>(input),  // begin
                                      std::istream_iterator<Line>());      // end
    return result;
}

Now we just need a definition for a Line. This is basically an object that is convertable to std::string and when used with operator>> reads a line of text.

struct Line
{
    std::string    line;
    operator std::string const&  {return line;}

    friend std::istream& operator>>(std::istream& s, Line& data)
    {
         return std::getline(s, data.line);
    }
};

Badly named function.

bool ValidateArguments(int arguments) {

Does not seem to validate the arguments. Also it prints out help. I would have two different functions for this. Especially since you may want to print out the help under other situations.

Looks like the first arguments to this function is not modified. So why is it passed by reference. Best to pass parameters by const reference to avoid the copy and prevent accidental modification. Prefer not to use out parameters return the result.

void SplitInputLine(std::string &user_specified_pattern, std::vector<string> &users_pattern_split) {
  char sep = ' ';

This is OK but unusual: user_specified_pattern.npos. I would normally expect std::string::npos.

That is a most complicated expression you are trying to evaluate in one line.

users_pattern_split.push_back(user_specified_pattern.substr(p+(p != 0),
      (q = user_specified_pattern.find(sep, p+1))-p-(p != 0)));

That is just horrible to read split it up into multiple lines. SO it is readable.

std::size_t start = p + (p != 0);
q                 = user_specified_pattern.find(sep, p+1)
std::size_t end   = q - p - (p != 0);
std::string sp    = user_specified_pattern.substr(start, end);
users_pattern_split.emplace_back(std::move(sp));

Don't worry. With RVO/NRVO and move semantics. This will become just as efficient as the original. But now is much more readable. Which is nice as we can now spot mistakes easier.

Seems like we are special casing p in two sub expressions but not the third. Which is odd looking. Why not modify the meaning of p (it always point to the start of the next section (or end).

share|improve this answer

The big thing that jumps out from reading your code is the lack of const. Just about every reference parameter that is passed should be passed by const &. This is very valuable - it means the compiler can enforce that only methods that will not change the object state will be called. Further, don't pass parameters by reference and write to them, returning void. Some people do this because they think it is more efficient (it almost always isn't), and it's not good practice. For example:

void SplitInputLine(std::string &user_specified_pattern, std::vector<string> &users_pattern_split)

Should be:

std::vector<std::string> SplitInputLine(const std::string& user_specified_pattern)

Likewise for:

void TokenValidator(std::vector<string> &users_pattern_split, std::string &users_converted_pattern)

which should be:

std::string TokenValidator(const std::vector<std::string>& users_pattern_split)

(Also, watch out for using string instead of std::string. If this is compiling for you, it suggests that a using namespace std is lurking around somewhere).

Speaking of SplitInputLine, how this function works is entirely non-obvious.

for (size_t p = 0, q = 0; p != user_specified_pattern.npos; p = q)
  users_pattern_split.push_back(user_specified_pattern.substr(p+(p != 0),
  (q = user_specified_pattern.find(sep, p+1))-p-(p != 0)));

This desperately needs to be split up into a few intermediate calls to actually show what is going on.This is "golfed" code - code written in the shortest possible way, readability be damned.

If you're using C++11, you can simplify a the "index" based looping and instead just use a range-based for:

for (int i = 0; i < users_pattern_split.size(); i++)

becomes

for(char c : users_pattern_split)

Be careful of passing parameters by value; this is introducing unnecessary overhead.

bool FindMatchingLines(std::vector<string> lines_in_file, std::string user_specified_pattern)

Here, full copies of lines_in_file and user_specified_patterns are created and passed into the function. Both should be passed by const & instead. Inside this function, using a ranged-based for will again make the code much easier to read:

for (int i = 0; i < lines_in_file.size(); ++i)

can become:

for(const auto& line : lines_in_file)

Be careful on going overboard on variable names. You've generally done a good job; everything is named with descriptive names, but things like is_there_a_match are overkill; match (or even match_exists) would both be fine.

As a final note, you might want to swap to the standard regex implementation (in <regex>) if it is available (again, this is a C++11 feature).

share|improve this answer

I'll approach this as another programmer that wants to reuse some of your code.

why is ReadFile declared to modify some vector? Based on what it does, it should return a fresh vector, by value (in case you hear that this may involve unnecessary copying, it doesn't, unless you're using Oracle Studio, and even that will change in 12.4). Btw, '\n' is the default for getline. Also, I can't reuse ReadFile if my file name comes from elsewhere because your function actually does two independent things: reading the standard input and reading lines from a file.

ValidateArguments doesn't actually validate the arguments, it doesn't even look at them: it 1) checks the number of the arguments and 2) prints the usage information for your program. Again, two things. Make usage printing a separate function, and either rename this one to indicate what it does, or actually validate the argv.

RegexConverter: is declared as if it modifies the user_specified_pattern, surely not what I want. As already correctly pointed out, the parameter must be a reference to const std::string. A comment as to what this does would help. Btw, if you rewrite the functions to actually return what they create, it would be simply return TokenValidator(SplitInputLine(arg));

SplitInputLine: once again a function that's modifying an existing vector instead of returning it, and again pretending to be modifying its user_specified_pattern parameter. Make it std::vector<std::string> SplitInputLine(const std::string& user_specified_pattern). And as already noted, it's incomprehensible: I would be worried about reusing it even if you provide unit tests. There are many ways to split a string using spaces as delimiters.

For the functions that are the meat of this program, comments about what they do what help a lot. What kind of tokens does TokenToRegex expect? (I could step back and look at what you're feeding re.PartialMatch, but it's not helping someone who would see this function called in my code.. which I wouldn't anyway because it has a surprise call to exit())

And yes, FindMatchingLines shouldn't be taking its parameters by value. It is also doing two things again: finding and printing. Either find and return what was found, or at least rename it "PrintMatchingLines".

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.