3
\$\begingroup\$

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;
}
\$\endgroup\$
2
\$\begingroup\$

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<<() for Element was in the wrong class. It was in Records.
  • Added a default constuctor for Element because libnop needs it. This means you can potentially have an uninitialised object. Also the existing constructor left id 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;
}
\$\endgroup\$
14
  • \$\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 Jan 31 '20 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 Jan 31 '20 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 Jan 31 '20 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 Jan 31 '20 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 Jan 31 '20 at 21:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.