[US.1][4] Object representation of cards #12 #13
Conversation
…ded default value stateRailway - OneRailway
…tructors to meet rules of five
…ove-assignment from class property. Allocated object property in stack during tests.
src/Card.hpp
Outdated
| { | ||
| std::array<std::string, 16> jobs_; | ||
| public: | ||
| void setJob(int number, const std::string & job); |
There was a problem hiding this comment.
@Angan7a I think the number can be const
test/CardTests.cpp
Outdated
|
|
||
| TEST_F(CardTests, check_if_call_setJob_out_of_range_will_throw_exception) | ||
| { | ||
| ASSERT_THROW(redCard.setJob(-9, "Cofasz się o trzy pola." ), std::exception); |
There was a problem hiding this comment.
If we don't care about the exception type we can use ASSERT_ANY_THROW. If we care then probably std::out_of_range is a proper exception type :)
test/CardTests.cpp
Outdated
|
|
||
| TEST_F(CardTests, set_job_3_Wracasz_do_Madrytu_check_if_value_getJob_3_return_the_same_message) | ||
| { | ||
| redCard.setJob(3, "Wracasz do Madrytu."); |
There was a problem hiding this comment.
Please extract a string to a variable and use it here and in ASSERT_EQ
src/Card.hpp
Outdated
| #include <string> | ||
| #include <array> | ||
|
|
||
| class Card |
src/Card.hpp
Outdated
|
|
||
| class Card | ||
| { | ||
| std::array<std::string, 16> jobs_; |
There was a problem hiding this comment.
I'm not sure if I get a concept of this representation. We will have an array with cards descriptions, which are set by a setter. Why not in a constructor? Who (which class) will set those values?
Codecov Report
@@ Coverage Diff @@
## master #13 +/- ##
==========================================
+ Coverage 93.1% 94.73% +1.63%
==========================================
Files 6 8 +2
Lines 29 38 +9
==========================================
+ Hits 27 36 +9
Misses 2 2
Continue to review full report at Codecov.
|
test/CardsTests.cpp
Outdated
|
|
||
| struct CardsTests : public ::testing::Test | ||
| { | ||
| json dataJson = R"({ |
There was a problem hiding this comment.
It would be better to keep this info in a separate json file and read it in this test file.
There was a problem hiding this comment.
I need only 1 card for test this class, so I make it in class CardsTests - than test will be independent of the size of input json file ;-).
src/Cards.cpp
Outdated
| @@ -0,0 +1,18 @@ | |||
| #include "Cards.hpp" | |||
|
|
|||
| Cards::Cards(const std::string & color, json dataPacked) : | |||
There was a problem hiding this comment.
Why not an enum? Color can have only 2 values.
Good idea with taking json as a constructor parameter :)
test/CardsTests.cpp
Outdated
|
|
||
| TEST_F(CardsTests, check_if_call_getOneCard_out_of_range_will_throw_exception) | ||
| { | ||
| ASSERT_ANY_THROW(redCards.getOneCard(83)); |
There was a problem hiding this comment.
16 is the first number which should throw an exception so it's good to use it in a test.
| "15": "Wychodzisz wolny z więzienia. Kartę należy zachować do wykorzystania lub sprzedania." | ||
| })"_json; | ||
| Cards redCards{"red", dataJson}; | ||
| json dataJson = R"({"2" : "Wracasz do Madrytu."})"_json; |
There was a problem hiding this comment.
You can extract a string "Wracasz do Madrytu" to a const and use it here and in line 23 to avoid duplications.
src/Cards.hpp
Outdated
| @@ -5,9 +5,15 @@ | |||
| using json = nlohmann::json; | |||
| using oneCard = std::string; | |||
|
|
|||
| enum class CardsColor | |||
| { | |||
| red, | |||
There was a problem hiding this comment.
Usually values for enum are in UPPERCASE
Codecov Report
@@ Coverage Diff @@
## master #13 +/- ##
==========================================
- Coverage 93.1% 86.27% -6.83%
==========================================
Files 6 28 +22
Lines 29 204 +175
==========================================
+ Hits 27 176 +149
- Misses 2 28 +26
Continue to review full report at Codecov.
|
| return collectionCards_.at(number); | ||
| } | ||
|
|
||
| void Cards::doOn(std::shared_ptr<Player> player) |
| BLUE | ||
| }; | ||
|
|
||
| class Cards : public Field |
There was a problem hiding this comment.
Why the Cards class inherit from the Field class?
| src/Field.cpp | ||
| src/OrdinaryCard.hpp | ||
| src/OrdinaryCard.cpp | ||
| src/Railway.hpp |
There was a problem hiding this comment.
Why so many types of cards? They are not needed now. They should be implemented later on requirements like:
Każdorazowe przejście przez pole "Start" lub zatrzymanie się na nim, nagradzane jest wypłacaną przez bank sumą w wysokości 400$, o ile grający nie "udaje się do wiezienia".
Pole "Parking strzeżony" traktowane jest jako pole dodatkowe nie podlegające wykupowi. Osoba znajdująca się swoim pionkiem na tym polu płaci do banku 400$. Pole "Podatek od wzbogacenia" traktowane jest podobnie, przy czym grający wpłaca do banku 200$.
Pole "Darmowy parking" funkcjonuje w grze jako pole nie przynoszące ani zysku ani strat.
A lot of code can be deleted now :)
fix #12