Prośba o sprawdzenie jakości kodu

0

Witam.

Bardzo bym prosił o sprawdzenie poniższego kodu. Kod działa tak jak chcę, ale ponieważ uczę się sam, to chciałym, żeby ktoś zweryfikował jakość mojego kodu.

Założenie są takie. Mam dwie klasy. Klasa Object ma dwa pola(string i int), a klasa ClassWithVector ma pole typu string i vector wskaźników do obiektów klasy Object. W tym momencie te klasy oczywiście nie mają większego sensu, ale chcę ten kod wykorzystać przy robieniu projektu, a to co wstawiam poniżej służy tylko do ćwiczeń. Chciałbym z poziomu programu dodawać do vectora nowe elementy. Zawartość vectora ma być wczytana z pliku podczas tworzenia obiektu, a następnie, tak zmodyfikowany vector ma być zapisany do pliku.

Z góry bardzo dziękuję za wszelkie krytyczne uwagi.

#ifndef OBJECT_H_INCLUDED
#define OBJECT_H_INCLUDED

class Object
{
    private:
    std::string name;
    int age;

    public:
    Object ( std::string name, int age );
    std::string getName();
    int getAge();
};

#endif // OBJECT_H_INCLUDED

#include <vector>
#include <string>
#include "Object.h"
using namespace std;


Object::Object( string name, int age )
{
    this->name = name;
    this->age = age;
};


string Object::getName()
{
    return this->name;
}

int Object::getAge()
{

    return this->age;
}

#ifndef CLASSWITHVECTOR_H_INCLUDED
#define CLASSWITHVECTOR_H_INCLUDED

class Object;

class ClassWithVector
{
    private:
    std::string name;
    std::vector<Object*> objects;
    public:
    ClassWithVector ( std::string name );
    void readDataFromFile();
    void saveDataToFile();
    void vecToString();
    std::vector<Object*>* getVector();
    void addObjectsToVector( Object* o, std::vector<Object*>* vec );

};

#endif // CLASSWITHVECTOR_H_INCLUDED

#include <boost/algorithm/string.hpp>
#include <fstream>
#include <iostream>
#include <vector>
#include <string>
#include "ClassWithVector.h"
#include "Object.h"
using namespace std;

ClassWithVector::ClassWithVector( string name )
{
    this->name = name;
    vector <Object*> objects;
    readDataFromFile();
};

void ClassWithVector::readDataFromFile()
{
    string oneLine;
    int intLine;
    fstream myFile;
    string fileName = "file";


    myFile.open( fileName, ios::in );
    if( myFile.good() == true )
    {
        cout << "The file is being read...\n";
        while( getline( myFile, oneLine ) )
        {
            vector<string> strs;
            boost::split(strs, oneLine, boost::is_any_of(" "));
            intLine = stoi(strs[1]);
            //cout<<strs[0]<<" "<<strs[1]<<"\n";
            Object *obj = new Object(strs[0], intLine);
           // cout<<obj->getName()<<"  "<<obj->getAge()<<"\n";
            this->objects.push_back(obj);
        }
        cout<<"End of the file.\n\n\n";
        myFile.close();

    }
    else
        cout << "There's a problem with reading the file :(\n\n\n";
}


void ClassWithVector::saveDataToFile()
{

    string oneLine;
    fstream myFile;
    string fileName = "file";


    myFile.open( fileName, ios::out  );
    if( myFile.good() == true )
    {
        cout << "The file is being opened...\n";
        for ( Object *o : objects )
        {
            myFile<<o->getName()<<" "<<o->getAge()<<"\n";

        }
        cout<<"End of writinig the data to the file.\n\n\n";
        myFile.close();

    }
    else
        cout << "There's a problem with opening the file :(\n\n\n";
}

void ClassWithVector::addObjectsToVector( Object* o, vector<Object*> *vec )
{
    vec->push_back(o);
}

void ClassWithVector::vecToString()
{
    for( Object *o : objects )
    {
        cout << o->getName();
        cout<<"  "<<o->getAge()<< "\n";
    }
}

vector<Object*>* ClassWithVector::getVector()
{
    cout<<"size in get: "<<this->objects.size()<<"\n";
    return &(this->objects);
}

int main()
{

    ClassWithVector c( "a1" );
    Object *o1 = new Object("n1", 1);
    Object *o2 = new Object("n2", 2);
    Object *o3 = new Object("n3", 3);
    c.addObjectsToVector(o1, c.getVector());
    c.addObjectsToVector(o2, c.getVector());
    c.addObjectsToVector(o3, c.getVector());


    c.saveDataToFile();
    c.vecToString();

return 0;
}

0

wrzucić na https://review.gerrithub.io/
a później można o jakimś review gadać.

2
  1. W konstruktorze Object użyj listy inicjalizacyjnej. Identycznie w przypadku ClassWithVector.
  2. Funkcje Object::getName i Object::getAge nie modifykują obiektu, więc oznacz je jako const
  3. W ciele konstruktora ClassWithVector linijka vector<Object*> objects jest zupełnie nie potrzebna.
  4. Boost jest zbędny. Wczytywanie można znacznie uprościć
  5. Serializację klasy Object przeniósłbym do klasy Object.
  6. Funkcja ClassWithVector::addObjectsToVector jest bezsensu.
  7. Funkcja ClassWithVector::getVector niech zwraca stałą referencję do vector<Object*> (jeśli w ogóle upublicznianie prywatnej składowej jest konieczne)
  8. Ciężko powiedzieć do czego służy klasa Object. Dlaczego nie użyjesz wektora obiektów zamiast wektora wskaźników? A jeśli już muszą być wskaźniki to pomyśl nad std::shared_ptr/std::unique_ptr.
2
  1. nazwa Object jest zbyt generyczna, nic nie mówi o funkcjonalności klasy. Może lepsze by było: Person, VentageItem
  2. standardy kodowania C++, mówią że pola powinny być na końcu.
  3. nazwa ClassWithVector jest zła, bo mówi o szczegółach implementacji, a nie o funkcjonalności
  4. surowe wskaźniki to obecnie anachronizm (>=C++11) w tym kontekście
  • brak konstruktora kopiującego lub jego blokady (brak surowych wskaźników spowodowałby, że sprawa rozwiązuje się sama)
  • w zasadzie dla tego kodu wskaźniki to overkill
  1. porównanie wartości bool do true dla większości jest dziwne.
  2. każdy blok w if else lepiej otoczyć klamrami, nawet jeśli jest to jedna linijka (dobra praktyka).
  3. trzeba było użyć std::istringstream a nie boost::split.
  4. po kiego grzyba dodatkowy argument w void addObjectsToVector( Object* o, std::vector<Object*>* vec );?

1 użytkowników online, w tym zalogowanych: 0, gości: 1