Skip to content

Lostmap - #2

Open
lostmap wants to merge 4 commits into
masterfrom
lostmap
Open

Lostmap#2
lostmap wants to merge 4 commits into
masterfrom
lostmap

Conversation

@lostmap

Copy link
Copy Markdown
Owner

Клиент для игры CardGame

@lostmaplostmap added the enhancement New feature or request label Jun 20, 2018
@lostmap
lostmap requested a review from leshiy1295June 20, 2018 17:26

@leshiy1295leshiy1295 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

С архитектурой - беда
По библиотекам (STL, pugi, SFML, конфиги) - есть, но не всё
Жаль, что очень поздно прислали первую версию - уже не будет времени поправить, как следует...

Comment threadclient/client.h

class InterLayer;

#include <SFML/Network.hpp>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

привязки на уровне интерфейса к библиотекам реализации быть не должно
нужно делать либо развязку на уровне классов-адаптеров, либо, например, использовать идиому PIMPL


ClientParsed::ClientParsed(std::string xml) {
pugi::xml_parse_result result = doc.load_file(xml.c_str()); //use for filenames
//pugi::xml_parse_result result = doc.load_string(xml.c_str());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

в итоговом коде не должно быть комментариев без объяснения, почему они были специально оставлены

#include <string>
#include <vector>
#include <map>
#include "pugixml.hpp"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

адаптеры/PIMPL

Comment threadclient/config.h
Spy,
RollCall};

enum Entity_type {NO_ENTITY, BEAR,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

если у вас с сервером используется общий код, то его стоит переиспользовать, в том числе на уровне файлов. Можно относить их в папку common, например...

Comment threadclient/deck.cpp

void Deck::cardsMove() {

for (size_t i = 0; i < size(); ++i) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

forEach...

Comment threadclient/droparea.cpp
int DropArea::getStrength() {
if (_card)
return _card->getStrength();
return -1;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

стоит вынести в именованную константу

Comment threadclient/droparea.h
private:
//sf::Image _image;
//sf::Texture _texture;
sf::Sprite _sprite;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

снова привязка к SFML на уровне .h файла

Comment threadclient/mainwindow.cpp

MainWindow::MainWindow() {

if(!_background.loadFromFile("images/field.jpg"))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

это должно передаваться извне/из конфигурационных файлов

Comment threadclient/party.cpp
_partner->setWin(0);
else
if (win == -1)
_partner->setWin(-1);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ещё более непонятные магические константы...
кажется, что setWin должно быть true/false...

Comment threadclient/sfmlcard.cpp


switch (entity_type) {
case(BEAR):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

о нет))))
для каждой карты должен быть свой класс)))
или это должно специальным образом проксироваться по категориям в другие классы, которые будут вызывать фабричный метод создания карты...

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancementNew feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@lostmap@leshiy1295