Skip to main content
8 of 10
added 2 characters in body
Oliver Schönrock
  • 2.5k
  • 1
  • 10
  • 23

##General

Please ensure code compiles when posting it here. Or state a specific justification why it doesn't. (e.g. Print() was missing a <<)

##Putting it all together

There are quite a few specific comments under your question, which I have incorporated and added a few more in a proposed alternative solution below. It is quite different in approach, technique and style. None of these are "must have", but you can take your pick.

##Parsing - how flexible

The most challenging part of any parsing is the question, how flexible / tolerant does it have to be? How consistent is the file format?

Probably the simplest way to parse anything in C++ is to use std::getline. You were already using it, but were you aware of the second version? If your parsing requirements are not very pedantic or in need of lightspeed performance you could use the overloaded version of std::getline which accepts a delimiter. I have shown below how that can be used.

It makes the code much simpler, but it's slightly less tolerant. e.g. double spaced gap is not good.

##Printing

The common idiom use making objects "printable" in C++ is to implement the << operator. This is best done using a friend function inside the class as shown.

You should almost always use '\n' and not std::endl, because the latter flushes the stream buffer, and that is often not needed. The '\n' line ending will also adapt to the local machine.

##Return "nothing"

In case of parsing failures, you were previously returning an "empty" ConfigUnit (correct term is value initialized). I have shown a, probably cleaner alternative, using C++17 std::optional.

##Use of exceptions

We can have a long debate about exceptions, there are several schools of thought. However, something I think most people agree on is that exceptions shouldn't be thrown during the "normal, expected case". They should be, well... "exceptional".

Your code was throwing and catching the eof flag. i.e. every invocation of the programme was throwing. This is probably bad.

I double checked the "file bits" and they didn't seem very useful to me, e.g. bad_bit doesn't get set if file is not found (perhaps the most common mode of failure), only fail_bit gets set. But that also gets set for eof. So I did it differently without exceptions.

##RAII

Look it up: "Resource acquisition is initialisation" (and what is not stated, is that when objects go out of scope they clean up after themselves). It is a key C++ feature. It means you don't have to call file.close(). When the object goes out of scope at end of function its destructor will be called, which will clean up.

This also means we don't have to have "one common exit point". Maybe that's what you were trying to achieve with your try .. catch block? It's not required. The right stuff just happens (because the std:: classes are well written).

This was my test file "music.txt":

CH_THRESHOLD 100
ba pram 314
SOME-PARAM2 3000
Too_many_spaces  999

And here is the suggested code:

#include <functional>
#include <iostream>
#include <fstream>
#include <sstream>
#include <string>
#include <vector>

struct config_unit {
  std::string parameter;
  std::string value;

  friend std::ostream& operator<<(std::ostream& ostream, const config_unit& cu) {
    return ostream << "Parameter: '" << cu.parameter << "'\t"
                   << "Value: '" << cu.value << "'" << '\n';
  }
};

std::optional<config_unit> get_config_unit(const std::string& line) {
  std::istringstream       ss(line);
  std::string              field;
  std::vector<std::string> fields;
  while (getline(ss, field, ' ')) fields.push_back(field);
  if (fields.size() != 2) return std::nullopt;
  return config_unit{fields[0], fields[1]};
}

int main() {
  std::string filename{"music.txt"};
  std::ifstream fstream(filename);
  if (fstream.fail()) {
    std::cerr << "couldn't open file '" << filename << "\n";
    return EXIT_FAILURE;
  }
  std::string line;
  while (std::getline(fstream, line)) {
    auto maybe_unit = get_config_unit(line);
    if (maybe_unit) std::cout << *maybe_unit;
  }
  return EXIT_SUCCESS;
}

And here is the output produced given the above input:

Parameter: 'CH_THRESHOLD'   Value: '100'
Parameter: 'SOME-PARAM2'    Value: '3000'
Oliver Schönrock
  • 2.5k
  • 1
  • 10
  • 23