Skip to content
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

Frame and tests for the database #2

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Frame and tests for the database #2

wants to merge 16 commits into from

Conversation

RozeQz
Copy link
Collaborator

@RozeQz RozeQz commented May 3, 2023

No description provided.

@RozeQz RozeQz requested a review from DPeshkoff May 3, 2023 18:28
Copy link

@a-badin a-badin left a comment

Choose a reason for hiding this comment

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

Оцениваю на 10, единственное кажется, что на выходе из ваших классов уже должны быть какие-то классы из доменной модели бизнес логики вашего приложения, чтобы не парсить сначала вывод от базы данных в json, а потом json в классы

BidsTable(std::shared_ptr<IDatabase> client);

json addBid(const json &info) const;
json deleteBid(const size_t id) const;
Copy link

Choose a reason for hiding this comment

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

в объявлении обычно const у value не пишут

class IDatabase
{
public:
virtual json createTable(json) = 0;
Copy link

Choose a reason for hiding this comment

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

кажется немного странно конвернтировать из представления базы данных в json, чтобы потом json cконвертировать уже в классы вашей бизнес логики. возможно было бы лучше возвращать тут std::string

return data;
}

static inline std::shared_ptr<PostgreSQL> client;
Copy link

Choose a reason for hiding this comment

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

Не нужно делать static объекты - глобальные состояния это всегда плохо (если они не const). Gtest специально для каждого теста создает новый экземпляр BaseTestConnection, чтобы он не мог быть в каком-то промежуточном состоянии от предыдущих тестов. Через static вы эту логику меняете, так, что теперь любой тест, который сломает client или connection может сломать все тесты сразу

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.

2 participants