Skip to content

Commit 8fa4f1f

Browse files
RyanJFieldRyanJField
committed
Fix code smells + more tests
1 parent f96214e commit 8fa4f1f

File tree

6 files changed

+190
-105
lines changed

6 files changed

+190
-105
lines changed

‎include/fdp/exceptions.hxx

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,37 +7,37 @@
77
namespace FairDataPipeline {
88
class config_parsing_error : public std::runtime_error {
99
public:
10-
explicit config_parsing_error(const std::string& message):std::runtime_error(message){};
10+
using std::runtime_error::runtime_error;
1111
};
1212

1313
class rest_apiquery_error : public std::runtime_error {
1414
public:
15-
explicit rest_apiquery_error(const std::string& message):std::runtime_error(message){};
15+
using std::runtime_error::runtime_error;
1616
};
1717

1818
class json_parse_error : public std::runtime_error {
1919
public:
20-
explicit json_parse_error(const std::string& message):std::runtime_error(message){};
20+
using std::runtime_error::runtime_error;
2121
};
2222

2323
class validation_error : public std::runtime_error {
2424
public:
25-
explicit validation_error(const std::string& message):std::runtime_error(message){};
25+
using std::runtime_error::runtime_error;
2626
};
2727

2828
class sync_error : public std::runtime_error {
2929
public:
30-
explicit sync_error(const std::string& message):std::runtime_error(message){};
30+
using std::runtime_error::runtime_error;
3131
};
3232

3333
class write_error : public std::runtime_error {
3434
public:
35-
explicit write_error(const std::string& message):std::runtime_error(message){};
35+
using std::runtime_error::runtime_error;
3636
};
3737

3838
class toml_error : public std::runtime_error {
3939
public:
40-
explicit toml_error(const std::string& message):std::runtime_error(message){};
40+
using std::runtime_error::runtime_error;
4141
};
4242

4343
}; // namespace FairDataPipeline

‎include/fdp/objects/distribution.hxx

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,22 +12,23 @@ namespace FairDataPipeline {
1212
class Distribution {
1313

1414
public:
15-
Distribution(std::string &name, double mu, double sigma):
15+
Distribution(const std::string &name, double mu, double sigma):
1616
_name(name),
1717
_mu(mu),
1818
_sigma(sigma),
1919
_component("0"){};
20-
Distribution(std::string &name, double mu, double sigma, std::string &component):
20+
Distribution(const std::string &name, double mu, double sigma, const std::string &component):
2121
_name(name),
2222
_mu(mu),
2323
_sigma(sigma),
2424
_component(component){};
25-
Distribution(std::string file_name);
26-
Distribution(std::string file_name, std::string component);
27-
std::string get_name(){return _name;}
28-
double get_mu(){return _mu;}
29-
double get_sigma(){return _sigma;}
30-
std::string get_component(){return _component;}
25+
explicit Distribution(const std::string &file_name);
26+
Distribution(const std::string &file_name, const std::string &component);
27+
virtual ~Distribution() = default;
28+
std::string get_name() const {return _name;}
29+
double get_mu() const {return _mu;}
30+
double get_sigma() const {return _sigma;}
31+
std::string get_component() const {return _component;}
3132

3233
std::string write_to_toml(std::string &file_name);
3334
std::string write_to_toml(std::string &component, std::string &file_name);
@@ -39,7 +40,7 @@ namespace FairDataPipeline {
3940
std::string _component;
4041

4142
protected:
42-
void read_from_toml(ghc::filesystem::path file_path, std::string component);
43+
void read_from_toml(const ghc::filesystem::path &file_path, const std::string &component);
4344
virtual bool isEqual(const Distribution& dist) const;
4445
friend bool operator==(const Distribution& lhs, const Distribution& rhs){
4546
return typeid(lhs) == typeid(rhs) && lhs.isEqual(rhs);

‎include/fdp/utilities/data_io.hxx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,9 +89,9 @@ write_point_estimate(T &value, const std::string &component,
8989
component, file_path);
9090
}
9191

92-
std::string get_first_component(ghc::filesystem::path file_path);
92+
std::string get_first_component(const ghc::filesystem::path &file_path);
9393

94-
bool component_exists(ghc::filesystem::path file_path, const std::string &component);
94+
bool component_exists(const ghc::filesystem::path &file_path, const std::string &component);
9595

9696
}; // namespace FairDataPipeline
9797

‎src/objects/distribution.cxx

Lines changed: 75 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -1,86 +1,82 @@
11
#include "fdp/objects/distribution.hxx"
2-
#include "fdp/utilities/data_io.hxx"
32
#include "fdp/exceptions.hxx"
3+
#include "fdp/utilities/data_io.hxx"
44

55
#include <vector>
66

77
namespace FairDataPipeline {
8-
Distribution::Distribution(std::string file_name){
9-
ghc::filesystem::path file_name_(file_name);
10-
_component = get_first_component(ghc::filesystem::path(file_name_));
11-
read_from_toml(file_name_, _component);
12-
}
13-
Distribution::Distribution(std::string file_name, std::string component){
14-
ghc::filesystem::path file_name_(file_name);
15-
this->_component = component;
16-
read_from_toml(file_name_, _component);
8+
Distribution::Distribution(const std::string &file_name) {
9+
ghc::filesystem::path file_name_(file_name);
10+
_component = get_first_component(ghc::filesystem::path(file_name_));
11+
read_from_toml(file_name_, _component);
12+
}
13+
Distribution::Distribution(const std::string &file_name,
14+
const std::string &component)
15+
: _component(component) {
16+
ghc::filesystem::path file_name_(file_name);
17+
read_from_toml(file_name_, _component);
18+
}
19+
20+
void Distribution::read_from_toml(const ghc::filesystem::path &file_path,
21+
const std::string &component) {
22+
auto data_ = read_component_from_toml(file_path, component);
23+
if (!data_.contains("type")) {
24+
throw toml_error("Error Toml file: " + file_path.string() +
25+
" does not contain a type tag");
26+
}
27+
28+
if (data_.at("type").as_string() != "distribution") {
29+
throw toml_error("Error component: " + component +
30+
" does not contain a distribution");
31+
}
32+
33+
std::vector<std::string> required_keys_{"distribution", "mu", "sigma"};
34+
35+
for (const auto &key : required_keys_) {
36+
if (!data_.contains(key)) {
37+
throw toml_error("Error component: " + component +
38+
" does not contain a " + key);
1739
}
18-
19-
void Distribution::read_from_toml(ghc::filesystem::path file_path, std::string component){
20-
auto data_ = read_component_from_toml(file_path, component);
21-
if(!data_.contains("type")){
22-
throw toml_error("Error Toml file: " + file_path.string() + " does not contain a type tag" );
23-
}
24-
25-
if(data_.at("type").as_string() != "distribution"){
26-
throw toml_error("Error component: " + component + " does not contain a distribution");
27-
}
28-
29-
std::vector<std::string> required_keys_{"distribution", "mu", "sigma"};
30-
31-
for(auto &key : required_keys_)
32-
{
33-
if(!data_.contains(key)){
34-
throw toml_error("Error component: " + component + " does not contain a " + key);
35-
}
36-
}
37-
38-
_name = data_.at("distribution").as_string();
39-
40-
41-
if(data_.at("mu").is_floating()){
42-
_mu = data_.at("mu").as_floating();
43-
}
44-
else if(data_.is_integer()){
45-
_mu = static_cast<double>(data_.at("mu").as_integer());
46-
}
47-
else{
48-
throw toml_error("Error mu value is not a number");
49-
}
50-
51-
if(data_.at("sigma").is_floating()){
52-
_sigma = data_.at("sigma").as_floating();
53-
}
54-
else if(data_.is_integer()){
55-
_sigma = static_cast<double>(data_.at("sigma").as_integer());
56-
}
57-
else{
58-
throw toml_error("Error sigma value is not a number");
59-
}
60-
61-
62-
}
63-
64-
std::string Distribution::write_to_toml(std::string &file_name){
65-
return write_to_toml(_component, file_name);
66-
}
67-
68-
std::string Distribution::write_to_toml(std::string &component, std::string &file_name){
69-
const toml::value data_{
70-
{component, {{"type", "distribution"},
71-
{"distribution", _name},
72-
{"mu", _mu},
73-
{"sigma", _sigma}}}};
74-
75-
return write_toml_data(ghc::filesystem::path(file_name), component, data_).string();
76-
77-
}
78-
79-
bool Distribution::isEqual(const Distribution& dist) const{
80-
return this->_component == dist._component &&
81-
this->_mu == dist._mu &&
82-
this->_name == dist._name &&
83-
this->_sigma == dist._sigma;
84-
}
85-
86-
};
40+
}
41+
42+
_name = data_.at("distribution").as_string();
43+
44+
if (data_.at("mu").is_floating()) {
45+
_mu = data_.at("mu").as_floating();
46+
} else if (data_.is_integer()) {
47+
_mu = static_cast<double>(data_.at("mu").as_integer());
48+
} else {
49+
throw toml_error("Error mu value is not a number");
50+
}
51+
52+
if (data_.at("sigma").is_floating()) {
53+
_sigma = data_.at("sigma").as_floating();
54+
} else if (data_.is_integer()) {
55+
_sigma = static_cast<double>(data_.at("sigma").as_integer());
56+
} else {
57+
throw toml_error("Error sigma value is not a number");
58+
}
59+
}
60+
61+
std::string Distribution::write_to_toml(std::string &file_name) {
62+
return write_to_toml(_component, file_name);
63+
}
64+
65+
std::string Distribution::write_to_toml(std::string &component,
66+
std::string &file_name) {
67+
const toml::value data_{{component,
68+
{{"type", "distribution"},
69+
{"distribution", _name},
70+
{"mu", _mu},
71+
{"sigma", _sigma}}}};
72+
73+
return write_toml_data(ghc::filesystem::path(file_name), component, data_)
74+
.string();
75+
}
76+
77+
bool Distribution::isEqual(const Distribution &dist) const {
78+
return this->_component == dist._component && this->_mu == dist._mu &&
79+
this->_name == dist._name && this->_sigma == dist._sigma;
80+
}
81+
82+
}; // namespace FairDataPipeline

‎src/utilities/data_io.cxx

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ toml::value read_parameter_from_toml(const ghc::filesystem::path file_path, cons
2020

2121
toml::value read_data_from_toml(const ghc::filesystem::path file_path){
2222
if (!ghc::filesystem::exists(file_path)) {
23-
throw std::runtime_error("File '" + file_path.string() +
23+
throw toml_error("File '" + file_path.string() +
2424
"' could not be opened as it does not exist");
2525
}
2626

@@ -36,10 +36,14 @@ toml::value read_component_from_toml(const ghc::filesystem::path file_path, cons
3636

3737

3838
toml::value read_parameter_from_toml(const ghc::filesystem::path file_path, const std::string &parameter, const std::string &component) {
39-
39+
4040
toml::value toml_data_ = read_data_from_toml(file_path);
4141

42-
if (!toml::get<toml::table>(toml_data_).at(component).contains("type")) {
42+
if(!component_exists(file_path, component)){
43+
throw toml_error("Component " + component + "does not exist");
44+
}
45+
46+
if (!toml_data_.at(component).contains("type")) {
4347
throw toml_error("Expected 'type' tag but none found");
4448
}
4549

@@ -48,7 +52,7 @@ toml::value read_parameter_from_toml(const ghc::filesystem::path file_path, cons
4852
parameter) {
4953
throw toml_error(
5054
"Expected "+ parameter +" for type but got '" +
51-
static_cast<std::string>(toml_data_.at("type").as_string()) + "'");
55+
static_cast<std::string>(toml_data_.at(component).at("type").as_string()) + "'");
5256
}
5357

5458
return toml_data_.at(component).at("value");
@@ -73,10 +77,9 @@ ghc::filesystem::path write_toml_data(ghc::filesystem::path file_path,
7377
toml_out_.open(file_path.string(), std::ios_base::app);
7478

7579
if (!toml_out_) {
76-
throw std::runtime_error("Failed to open TOML file for writing");
80+
throw toml_error("Failed to open TOML file for writing");
7781
}
7882

79-
//toml_out_ << toml::format(data_);
8083
toml_out_ << data;
8184

8285
toml_out_.close();
@@ -87,7 +90,7 @@ ghc::filesystem::path write_toml_data(ghc::filesystem::path file_path,
8790
return file_path;
8891
}
8992

90-
std::string get_first_component(ghc::filesystem::path file_path){
93+
std::string get_first_component(const ghc::filesystem::path &file_path){
9194
if (!ghc::filesystem::exists(file_path)) {
9295
throw toml_error("File '" + file_path.string() +
9396
"' could not be opened as it does not exist");
@@ -97,7 +100,7 @@ std::string get_first_component(ghc::filesystem::path file_path){
97100
return toml_data_.as_table().begin()->first;
98101
}
99102

100-
bool component_exists(ghc::filesystem::path file_path, const std::string &component){
103+
bool component_exists(const ghc::filesystem::path &file_path, const std::string &component){
101104
if(!ghc::filesystem::exists(file_path))
102105
{
103106
return false;

0 commit comments

Comments
 (0)