Skip to content

Add reader class #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jun 27, 2021
Merged

Add reader class #1

merged 8 commits into from
Jun 27, 2021

Conversation

NikolaJelic
Copy link
Collaborator

Added reader class that can can handle both file and string inputs.

class reader {
  std::vector<std::string> file_lines;
  std::unordered_map<std::string, std::string> key_value_pairs;
public:
  std::unordered_map<std::string, std::string> get_pairs();
  bool read_file(std::filesystem::path const &filename);
  bool read_file(std::string const &raw_string);
}

This class is currently working and able to extract, save and pass the stored unordered_map to other variables and objects.
Due to read_file() being and overloaded function and path and string being very similar when passed as literals, they first need to be initialized as variables of their respective type and passed to the function.
When passing the data as a string to the function, the key-value pairs need to be divided with the char /, as shown in the code below:

  pn::reader r;
  std::string raw_input = {"a=5/b=10/23=c"};
  std::filesystem::path filename{"test/test.ini"};
  r.read_file(filename); // r.read_file(raw_input);

For future implementation there is one more problem I noticed, that needs to be solved : the read_file() function doesn't take care of excessive white space at the start or end of the string which could cause problems during search.

@NikolaJelic NikolaJelic self-assigned this Jun 15, 2021
@karnkaul
Copy link
Member

karnkaul commented Jun 15, 2021

I'd suggest not having a special character like / for text vs file input: both should converge on exactly the same code paths. You could simply use \n to simulate new lines in code.

@@ -2,7 +2,8 @@ cmake_minimum_required(VERSION 3.19)

project(pini)
add_executable(${PROJECT_NAME} src/main.cpp

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line can be simplified, especially if you plan to add more files in the future it is not feasible to keep this list of files growing

src/main.cpp Outdated
@@ -1,10 +1,14 @@
#include "pini.hpp"
#include "util.hpp"
#include <iostream>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort your includes:

#include <iostream>   # std first
#include "pini.hpp"   # then your headers
#include "utils.hpp"

This is my personal preference, but there are also other otions.

// std::vector<std::string> separate_substrings(const std::string &str);
// extract key-value pairs from given vector of strings and insert them in an
// unordered_map
std::unordered_map<std::string, std::string>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird lines breaks, I suggest you add a clang-format file to clean up your formatting everywhere.

src/util.cpp Outdated
std::string value;

for (const auto &str : file_lines) {
if (auto delimeter_pos = str.find('=');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

confusing line break, see also this mail from Linus as to why you don't have to adhere to the 80-column philosophy - in this case specifically, your code is harder to read because you make these unexpected line breaks

@NikolaJelic NikolaJelic requested a review from karnkaul June 27, 2021 21:01
@NikolaJelic NikolaJelic merged commit 9e1bfe8 into main Jun 27, 2021
@NikolaJelic NikolaJelic deleted the dev branch July 12, 2021 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants