I'm trying to construct a simple database for use by 2 persons only (myself and other guy), right now, we're using excel table with 4 fields "requisition, origin date, request date and authorized date", but I want to take it to the next level :D. So, I'm building this small database app to emulate that, the file will be shared and I'm not worried about not being able to load the file if it's in use by him ( or viceversa). This database consists of 2 classes "Element" and "Records", element carries each row data, and records handles vectors of elements, it's the connection with the outer world for elements.
I need help on this to know, if this is much code, or a little bit of code, or how can it be improved?, I'm not worried about speed or disk space or access time, this database will contain at much 5,000 records.
Later on, this code will be used on winapi.
This is the code, I'm using visual studio:
Element.h
#pragma once
#include <cstdint>
#include <ctime>
class Element
{
public:
Element(uint32_t requi, time_t originDate,
time_t requestDate, time_t authorizedDate, uint64_t amount, bool hasTax, bool authorized = false);
void SetId(uint32_t id)
{
this->id = id;
}
uint32_t GetId() const
{
return id;
}
bool operator<(const Element& rhs) const
{
return requi < rhs.requi;
}
uint32_t GetRequi() const
{
return requi;
}
time_t GetOriginDate() const
{
return originDate;
}
time_t GetRequestDate() const
{
return requestDate;
}
time_t GetAuthorizedDate() const
{
return authorizedDate;
}
uint64_t GetAmount() const
{
return amount;
}
bool IsTaxed() const
{
return hasTax;
}
bool IsAuthorized() const
{
return authorized;
}
bool operator<(const Element& rhs)
{
return requi < rhs.requi;
}
bool operator>(const Element& rhs)
{
return requi > rhs.requi;
}
bool operator==(const Element& rhs)
{
return requi == rhs.requi;
}
void Authorize();
void Print();
private:
uint32_t id;
uint32_t requi;
time_t originDate;
time_t requestDate;
time_t authorizedDate;
uint64_t amount;
bool hasTax;
bool reviewed = true;
bool authorized = false;
};
Element.cpp
#include "Element.h"
#include <iostream>
Element::Element(uint32_t requi, time_t originDate,
time_t requestDate, time_t authorizedDate, uint64_t amount, bool hasTax, bool authorized)
:
requi(requi),
originDate(originDate),
requestDate(requestDate),
authorizedDate(authorizedDate),
amount(amount),
hasTax(hasTax),
authorized(authorized)
{
}
void Element::Authorize()
{
std::time(&authorizedDate);
authorized = true;
}
void Element::Print() // not used at the moment
{
std::cout << "Id: " << id << ", Requi: " << requi << ", Monto: $ "
<< amount << std::endl;
}
Records.h
#pragma once
#include <string>
#include <vector>
#include "Element.h"
class Records
{
public:
friend std::ostream& operator<<(std::ostream& out, const Element& rhs)
{
out << "Id: " << rhs.GetId() << ", Requi: " << rhs.GetRequi() << ", Monto: $ "
<< int(rhs.GetAmount() / 100) << "." << int(rhs.GetAmount() % 100)
<< (rhs.IsTaxed() ? " mas IVA":" neto (no aplica IVA)");
return out;
}
void Insert(Element& element);
void SaveToFile();
void LoadFromFile();
void ListRequi();
private:
std::vector<Element> elements;
unsigned int nRecords = 0;
};
Records.cpp
#include "Records.h"
#include <cassert>
#include <fstream>
#include <algorithm>
#include <iterator>
#include <iostream>
void Records::Insert(Element & element)
{
//autoincrement record count and use it as indes (id)
element.SetId(++nRecords);
elements.emplace_back(element);
}
void Records::SaveToFile()
{
std::ofstream file("datos.txt", std::ios::binary);
assert(file);
if (file)
{
nRecords = (unsigned int)elements.size();
file.write(reinterpret_cast<char*>(&nRecords), sizeof(nRecords) );
if (nRecords > 0)
{
for (auto& e : elements)
{
uint32_t tmpId = e.GetId();
uint32_t tmpRequi = e.GetRequi();
time_t tmpOrigin = e.GetOriginDate();
time_t tmpRequest = e.GetRequestDate();
time_t tmpAuth = e.GetAuthorizedDate();
uint64_t tmpAmount = e.GetAmount();
bool tmpTaxed = e.IsTaxed();
bool tmoAuthorized = e.IsAuthorized();
file.write(reinterpret_cast<char*>(&tmpId), sizeof(tmpId));
file.write(reinterpret_cast<char*>(&tmpRequi), sizeof(tmpRequi));
file.write(reinterpret_cast<char*>(&tmpOrigin), sizeof(tmpOrigin));
file.write(reinterpret_cast<char*>(&tmpRequest), sizeof(tmpRequest));
file.write(reinterpret_cast<char*>(&tmpAuth), sizeof(tmpAuth));
file.write(reinterpret_cast<char*>(&tmpAmount), sizeof(tmpAmount));
file.write(reinterpret_cast<char*>(&tmpTaxed), sizeof(tmpTaxed));
file.write(reinterpret_cast<char*>(&tmoAuthorized), sizeof(tmoAuthorized));
}
}
}
}
void Records::LoadFromFile()
{
std::ifstream file("datos.txt", std::ios::binary);
assert(file);
if (file)
{
unsigned int nTmpRecords = 0;
nRecords = 0;
file.read(reinterpret_cast<char*>(&nTmpRecords), sizeof(nTmpRecords));
if (nTmpRecords > 0)
{
elements.clear();
for (unsigned int i = 0; i < nTmpRecords && file.good(); ++i)
{
uint32_t tmpId = 0;
uint32_t tmpRequi = 0;
time_t tmpOrigin = 0;
time_t tmpRequest = 0;
time_t tmpAuth = 0;
uint64_t tmpAmount = 0;
bool tmpTaxed;
bool tmpAuthorized;
file.read(reinterpret_cast<char*>(&tmpId), sizeof(tmpId));
file.read(reinterpret_cast<char*>(&tmpRequi), sizeof(tmpRequi));
file.read(reinterpret_cast<char*>(&tmpOrigin), sizeof(tmpOrigin));
file.read(reinterpret_cast<char*>(&tmpRequest), sizeof(tmpRequest));
file.read(reinterpret_cast<char*>(&tmpAuth), sizeof(tmpAuth));
file.read(reinterpret_cast<char*>(&tmpAmount), sizeof(tmpAmount));
file.read(reinterpret_cast<char*>(&tmpTaxed), sizeof(tmpTaxed));
file.read(reinterpret_cast<char*>(&tmpAuthorized), sizeof(tmpAuthorized));
Element tmpElement(tmpRequi, tmpOrigin, tmpRequest, tmpAuth,
tmpAmount, tmpTaxed, tmpAuthorized);
tmpElement.SetId(tmpId);
elements.emplace_back(tmpElement);
}
}
}
}
void Records::ListRequi()
{
if (elements.size() > 0)
{
std::sort(elements.begin(), elements.end());
//test code
//auto result = std::find_if(elements.begin(), elements.end(),
// [](const Element& e)
// {
// return e.GetId() == 2;
// });
//if (result != elements.end())
//{
// std::cout << result->GetRequi() << std::endl;
//}
std::copy(elements.begin(), elements.end(), std::ostream_iterator<Element>(std::cout, "\n") );
}
}
This is just to test the database app, it will save to a file named "datos.txt".
main.cpp
#include <iostream>
#include "Records.h"
#include <algorithm>
#include <ctime>
int main()
{
//test code to insert elements to records and save to file
{
time_t tRequest = 0;
time_t tElaboration = 0;
// get today's date and time
std::time(&tRequest);
std::time(&tElaboration);
Records reg;
reg.Insert(Element(22132, tElaboration, tRequest, 0, 1586450, true));
reg.Insert(Element(22340, tElaboration, tRequest, 0, 123490, false));
reg.Insert(Element(45398, tElaboration, tRequest, 0, 58674, true));
reg.Insert(Element(21518, tElaboration, tRequest, 0, 879965, true));
reg.SaveToFile();
std::cout << "The following records has been saved: \n";
reg.ListRequi();
}
std::cout << "======================================\n";
//test code to load elements to memory from file
{
Records reg;
reg.LoadFromFile();
std::cout << "\nThe following records has been loaded \n";
reg.ListRequi();
}
std::cin.get();
return 0;
}
1 Answer 1
Serialisation
What you have is OK. It works, but your serialisation code is verbose, brittle, manual, and hard to maintain.
This is a very common problem. Plenty of libraries out there, including this very good and popular one from Google: libnop.
I refactored your code using that. The number of lines of code dropped significantly and became more robust and maintainable. See below:
Other notable changes
Insert(Element&)
was broken, because it was not accepting rvalues. Added an overload- The
friend operator<<()
forElement
was in the wrong class. It was inRecords
. - Added a default constuctor for
Element
becauselibnop
needs it. This means you can potentially have an uninitialised object. Also the existing constructor leftid
uninitiliased. Give this some thought. - I know this is subjective, but: code formatting. Yours was very "verbose". Consider the style you are using.
// Element.h
#pragma once
#include <cstdint>
#include <ctime>
#include <nop/serializer.h>
class Element {
public:
Element() = default;
Element(uint32_t requi, time_t originDate, time_t requestDate,
time_t authorizedDate, uint64_t amount, bool hasTax,
bool authorized = false);
void SetId(uint32_t id) { this->id = id; }
[[nodiscard]] uint32_t GetId() const { return id; }
[[nodiscard]] uint32_t GetRequi() const { return requi; }
[[nodiscard]] time_t GetOriginDate() const { return originDate; }
[[nodiscard]] time_t GetRequestDate() const { return requestDate; }
[[nodiscard]] time_t GetAuthorizedDate() const { return authorizedDate; }
[[nodiscard]] uint64_t GetAmount() const { return amount; }
[[nodiscard]] bool IsTaxed() const { return hasTax; }
[[nodiscard]] bool IsAuthorized() const { return authorized; }
bool operator<(const Element& rhs) const { return requi < rhs.requi; }
bool operator>(const Element& rhs) const { return requi > rhs.requi; }
bool operator==(const Element& rhs) const { return requi == rhs.requi; }
bool operator!=(const Element& rhs) const { return requi != rhs.requi; }
void Authorize();
friend std::ostream& operator<<(std::ostream& out, const Element& el) {
out << "Id: " << el.GetId() << ", Requi: " << el.GetRequi() << ", Monto: $ "
<< int(el.GetAmount() / 100) << "." << int(el.GetAmount() % 100)
<< (el.IsTaxed() ? " mas IVA" : " neto (no aplica IVA)");
return out;
}
private:
uint32_t id;
uint32_t requi;
time_t originDate;
time_t requestDate;
time_t authorizedDate;
uint64_t amount;
bool hasTax;
bool reviewed = true;
bool authorized = false;
NOP_STRUCTURE(Element, id, requi, originDate, requestDate, authorizedDate,
amount, hasTax, reviewed, authorized);
};
// Element.cpp
#include "Element.h"
Element::Element(uint32_t requi, time_t originDate, time_t requestDate,
time_t authorizedDate, uint64_t amount, bool hasTax,
bool authorized)
: requi(requi), originDate(originDate), requestDate(requestDate),
authorizedDate(authorizedDate), amount(amount), hasTax(hasTax),
authorized(authorized) {}
void Element::Authorize() {
std::time(&authorizedDate);
authorized = true;
}
// Records.h
#pragma once
#include "Element.h"
#include <nop/serializer.h>
#include <nop/utility/die.h>
#include <nop/utility/stream_reader.h>
#include <nop/utility/stream_writer.h>
#include <string>
#include <vector>
class Records {
public:
void Insert(Element& element);
void Insert(Element&& element) { Insert(element); } // support rvalues
void SaveToFile(const std::string& filename);
void LoadFromFile(const std::string& filename);
void ListRequi();
private:
std::vector<Element> elements;
};
// Records.cpp
#include "Records.h"
#include <algorithm>
#include <fstream>
#include <iostream>
#include <iterator>
void Records::Insert(Element& element) {
element.SetId(elements.size() +
1); // autoincrement record count and use it as indes (id)
elements.emplace_back(element);
}
void Records::SaveToFile(const std::string& filename) {
using Writer = nop::StreamWriter<std::ofstream>;
nop::Serializer<Writer> serializer{filename};
serializer.Write(elements) || nop::Die(std::cerr);
}
void Records::LoadFromFile(const std::string& filename) {
using Reader = nop::StreamReader<std::ifstream>;
nop::Deserializer<Reader> deserializer{filename};
deserializer.Read(&elements) || nop::Die(std::cerr);
}
void Records::ListRequi() {
if (elements.size() > 0) {
std::sort(elements.begin(), elements.end());
std::copy(elements.begin(), elements.end(),
std::ostream_iterator<Element>(std::cout, "\n"));
}
}
// main.cpp
#include "Records.h"
#include <algorithm>
#include <ctime>
#include <iostream>
int main() {
const auto filename = std::string{"datos.txt"};
{
time_t tRequest = 0;
time_t tElaboration = 0;
// get today's date and time
std::time(&tRequest);
std::time(&tElaboration);
Records reg;
reg.Insert(Element(22132, tElaboration, tRequest, 0, 1586450, true));
reg.Insert(Element(22340, tElaboration, tRequest, 0, 123490, false));
reg.Insert(Element(45398, tElaboration, tRequest, 0, 58674, true));
reg.Insert(Element(21518, tElaboration, tRequest, 0, 879965, true));
reg.SaveToFile(filename);
std::cout << "The following records have been saved:\n";
reg.ListRequi();
}
{
Records reg;
reg.LoadFromFile(filename);
std::cout << "\nThe following records have been loaded:\n";
reg.ListRequi();
}
return 0;
}
-
\$\begingroup\$ This is clever, I never thought of addint and rvalue supporter for Insert method. Never heard of libop but, it looks nice, let me try it. Thank you very much for your comment!, I need to update my code :D Thank you. \$\endgroup\$Vital Zero– Vital Zero2020年01月31日 18:44:02 +00:00Commented Jan 31, 2020 at 18:44
-
\$\begingroup\$ @VitalZero Thanks. It was fun finding out about
libnop
for myself. Since you're new: If you found the answer useful, please upvote and accept it. \$\endgroup\$Oliver Schönrock– Oliver Schönrock2020年01月31日 18:57:04 +00:00Commented Jan 31, 2020 at 18:57 -
\$\begingroup\$ Tried to use libnop, but it gave me tons of errors, everything seems to be fine except for "#if __has_cpp_attribute(clang::fallthrough)" code which seems to be a typo \$\endgroup\$Vital Zero– Vital Zero2020年01月31日 20:55:56 +00:00Commented Jan 31, 2020 at 20:55
-
\$\begingroup\$ @VitalZero Has for me to diagnose. You should search for "error message libnop" and the name and version of the compiler you are using. That attribute is documented here: clang.llvm.org/docs/LanguageExtensions.html Are you compiling with at least C++14 support ? github.com/google/libnop \$\endgroup\$Oliver Schönrock– Oliver Schönrock2020年01月31日 21:15:47 +00:00Commented Jan 31, 2020 at 21:15
-
\$\begingroup\$ Well, most of the problem comes from NOP_STRUCTURE not being recognized as a MACRO but is being recognized as a member function of Element class, so, it's expecting the definition. Actually it gives me 4 errors and 7 warnings. I'm using c++14. E2512 is for the clang::fallthrough and the rest is C4146. I found a post about error (not specific) that says libnop doesn't support MSVC :( github.com/google/libnop/issues/4 \$\endgroup\$Vital Zero– Vital Zero2020年01月31日 21:40:38 +00:00Commented Jan 31, 2020 at 21:40