Include (only) what you use
This class uses nothing from <iostream>
, so it's a waste of the compiler's time to include it.
Control the public interface
Is trim()
really a facility that should be a part of SectionlessINIReader
? I don't think it belongs in the public interface, and more likely belongs in a private
or protected
section.
Make it easier to use
The getLong()
member function has an awkward interface, requiring the user to pass an lvalue (worse, a pointer). Consider returning a std::optional<long>
instead, to convey the result in a single value.
Pass strings by const reference
There's no need to make a copy of the key
argument to the accessors - that should be declared as const std::string& key
instead.
Accessors should be const
get()
and getLong()
both modify dict
(they create a new empty mapping for any requested key that doesn't yet exist). We don't want to do that, as it's wasteful. We would have spotted this bug if we'd declared those members const
(so that they could be used by code with only a const reference to the reader - that will be important if we later add writing functions).
We could write:
std::optional<std::string> get(std::string key) const
{
auto const it = dict.find(key);
if (it == dict.end())
return {};
return it->second;
}
And getLong()
could use it:
std::optional<long> getLong(std::string key) const
{
auto const value = get(key);
if (!value.has_value())
return {};
return std::stol(value.value());
}
Open the code to extension
At present, the only way to add more types that can be read is to edit this class or to subclass it. And it's not designed for subclassing (it has a non-virtual destructor), so it's closed to end-users.
One way to make it open for extension is to separate the string-to-value conversion from the string lookup, and allow users to specialize that part; our code could then use the appropriate specialization, or fail to compile if no conversion has been defined.
This would be the starting position:
namespace convert {
// This function must be specialized for any extension types to
// convert from string to value.
template<typename T>
std::optional<T> fromString(const std::string&);
}
It's used in our get<>()
function quite simply:
template<typename T>
std::optional<T> get(const std::string& key) const
{
auto const value = get(key);
if (!value.has_value())
return {};
return convert::fromString<T>(value.value());
}
We could provide some common conversions:
template<>
std::optional<std::string> convert::fromString(const std::string& s)
{ return s; }
template<>
std::optional<long> convert::fromString(const std::string& s)
{ return std::stol(s); }
template<>
std::optional<double> convert::fromString(const std::string& s)
{ return std::stod(s); }
Users can add their own conversions in similar manner.
Variation
Instead of creating a free function to specialize, we could make fromString()
be a private static member function of SectionlessINIReader
. The choice may well depend on whether you might want to use fromString()
from anywhere else.
Think about exceptions
std::stol()
will throw an exception if the value can't be parsed as a long. Do we want to pass this straight to our callers, or do we want to treat malformed values the same as missing values, and simply return an empty std::optional
? (There's no right or wrong answer here; I just want to be sure that we've made a conscious choice.)
Avoid narrowing conversions
int idxEq;
...
if (((idxEq = line.find("=")) != std::string::npos)) {
Here, we assign the result of find()
(a size type) to idxEq
(a signed integer) and then compare idxEq
to std::string::npos
(a size type again). That's a potentially narrowing conversion which could change the sign, and a comparison between types of different signedness. I know we don't expect keys to be long enough to overflow int
, but it would be nice to silence the compiler warning here, and it's very easy to make it consistent:
std::string::size_type idxEq;
Check the result of filesystem operations
In the constructor, we call file.close()
, but never check the result. If we want to be sure that we read the complete file, we need to ensure that close()
has succeeded.
If we really don't care about whether we missed some data, then we could simply omit the close()
and let the file's destructor do that for us - but I don't advise ignoring a failed close.
Inefficient use of search
There's no need to search all the way through a string just to see what its first character is:
if (line.find_first_not_of(";#") == 0)
It's clearer to examine the first character directly (after checking that the line has at least one character, of course).
Check for duplicate keys
When reading, we simply overwrite any key that appears more than once. Consider emitting a warning, or throwing an exception.
Remove dead code
There's a remnant of some old code still hanging around, which should be deleted:
// bool getDouble(...)
Modified code
#include <fstream>
#include <map>
#include <optional>
#include <stdexcept>
#include <string>
class SectionlessINIReader
{
private:
std::map<std::string, std::string> dict = {};
public:
SectionlessINIReader(const std::string& filename)
{
auto file = std::ifstream {filename};
readStream(file);
}
SectionlessINIReader(std::istream& in)
{
readStream(in);
}
std::optional<std::string> get(const std::string& key) const
{
auto const it = dict.find(key);
if (it == dict.end())
return {};
return it->second;
}
template<typename T>
std::optional<T> get(const std::string& key) const
{
auto const value = get(key);
if (!value.has_value())
return {};
return fromString<T>(value.value());
}
private:
// https://stackoverflow.com/a/1798170/3516684
static std::string trim(const std::string& str,
const std::string& whitespace = " \t")
{
const auto strBegin = str.find_first_not_of(whitespace);
if (strBegin == std::string::npos)
return {}; // no content
const auto strEnd = str.find_last_not_of(whitespace);
const auto strRange = strEnd - strBegin + 1;
return str.substr(strBegin, strRange);
}
template<typename T>
static std::optional<T> fromString(const std::string&);
private:
void readStream(std::istream& in)
{
std::string line;
while (std::getline(in, line)) {
if (line.empty() or line[0] == ';' or line[0] == '#') {
continue;
}
auto const idxEq = line.find('=');
if (idxEq == line.npos) {
// consider reporting an error here
continue;
}
auto const key = trim(line.substr(0,idxEq));
auto const value = trim(line.substr(idxEq+1));
if (!dict.emplace(key, value).second) {
throw std::runtime_error("Duplicate key in INI file");
}
}
if (!in.eof()) {
throw std::runtime_error("Failed to read INI file");
}
}
};
// String converters
template<>
std::optional<std::string> SectionlessINIReader::fromString(const std::string& s)
{ return s; }
template<>
std::optional<long> SectionlessINIReader::fromString(const std::string& s)
{ return std::stol(s); }
template<>
std::optional<double> SectionlessINIReader::fromString(const std::string& s)
{ return std::stod(s); }
// we can add more specializations here, and user may also add
// specializations.
#include <iostream>
#include <sstream>
int main()
{
std::istringstream contents("age = 30\n"
"favourite_drink = tea\n"
"name = Bob\n");
const SectionlessINIReader ini(contents);
auto age = ini.get<long>("age");
if (age.has_value())
std::cout << "Name: " << ini.get<std::string>("name").value_or("")
<< "; age: " << age.value()
<< std::endl;
}