|
| 1 | +# Powtórka #2 |
| 2 | + |
| 3 | +## Przykłady z Code Review |
| 4 | + |
| 5 | +--- |
| 6 | + |
| 7 | +Example: |
| 8 | + |
| 9 | +```cpp |
| 10 | +bool doesPasswordsMatch(const std::string& password, const std::string& repeatedPassword) { |
| 11 | + if (password == repeatedPassword) { |
| 12 | + return true; |
| 13 | + } |
| 14 | + return false; |
| 15 | +} |
| 16 | +``` |
| 17 | +
|
| 18 | +Better: |
| 19 | +
|
| 20 | +```cpp |
| 21 | +bool doesPasswordsMatch(const std::string& password, const std::string& repeatedPassword) { |
| 22 | + return password == repeatedPassword; |
| 23 | +} |
| 24 | +``` |
| 25 | + |
| 26 | +--- |
| 27 | + |
| 28 | +Example: |
| 29 | + |
| 30 | +```cpp |
| 31 | +std::string getErrorMessage(ErrorCode Error) { |
| 32 | + switch (Error) { |
| 33 | + case ErrorCode::Ok: |
| 34 | + return "OK"; |
| 35 | + break; |
| 36 | + case ErrorCode::PasswordNeedsAtLeastNineCharacters: |
| 37 | + return "Password Needs At Least Nine Characters"; |
| 38 | + break; |
| 39 | + case ErrorCode::PasswordNeedsAtLeastOneNumber: |
| 40 | + return "Password Needs At Least One Number"; |
| 41 | + break; |
| 42 | + case ErrorCode::PasswordNeedsAtLeastOneSpecialCharacter: |
| 43 | + return "Password Needs At Least One Special Character"; |
| 44 | + break; |
| 45 | + case ErrorCode::PasswordNeedsAtLeastOneUppercaseLetter: |
| 46 | + return "Password Needs A Least One Uppercase Letter"; |
| 47 | + break; |
| 48 | + case ErrorCode::PasswordsDoesNotMatch: |
| 49 | + return "Passwords Does Not Match"; |
| 50 | + break; |
| 51 | + default: |
| 52 | + return "UNDEFINED ERROR"; |
| 53 | + } |
| 54 | + } |
| 55 | + // What do you think about removing case ErrorCode::Ok: and putting it in default? |
| 56 | + // Braces are not needed even for multiline cases. It's only matter of convention if you should apply them or not. They don't provide additional safety. |
| 57 | + // We usually don't indent case and code inside namespace |
| 58 | +``` |
| 59 | +
|
| 60 | +Better?: |
| 61 | +
|
| 62 | +```cpp |
| 63 | +std::string getErrorMessage(ErrorCode error) { |
| 64 | + switch (error) { |
| 65 | + case ErrorCode::PasswordNeedsAtLeastNineCharacters: |
| 66 | + return "Password Needs At Least Nine Characters"; |
| 67 | + case ErrorCode::PasswordNeedsAtLeastOneNumber: |
| 68 | + return "Password Needs At Least One Number"; |
| 69 | + case ErrorCode::PasswordNeedsAtLeastOneSpecialCharacter: |
| 70 | + return "Password Needs At Least One Special Character"; |
| 71 | + case ErrorCode::PasswordNeedsAtLeastOneUppercaseLetter: |
| 72 | + return "Password Needs A Least One Uppercase Letter"; |
| 73 | + case ErrorCode::PasswordsDoesNotMatch: |
| 74 | + return "Passwords Does Not Match"; |
| 75 | + default: |
| 76 | + return "OK"; |
| 77 | + } |
| 78 | + } |
| 79 | +``` |
| 80 | + |
| 81 | +--- |
| 82 | + |
| 83 | +Example: |
| 84 | + |
| 85 | +```cpp |
| 86 | +if(doesPasswordsMatch(first_pass, second_pass)) { |
| 87 | + return checkPasswordRules(first_pass); |
| 88 | +} else { |
| 89 | + return ErrorCode::PasswordsDoesNotMatch; |
| 90 | +} |
| 91 | +``` |
| 92 | + |
| 93 | +Better: |
| 94 | + |
| 95 | +```cpp |
| 96 | +if(doesPasswordsMatch(first_pass, second_pass)) { |
| 97 | + return checkPasswordRules(first_pass); |
| 98 | +} |
| 99 | +return ErrorCode::PasswordsDoesNotMatch; |
| 100 | +``` |
| 101 | + |
| 102 | + |
| 103 | +--- |
| 104 | + |
| 105 | +```cpp |
| 106 | +enum class ErrorCode { |
| 107 | + PasswordNeedsAtLeastNineCharacters, |
| 108 | + PasswordNeedsAtLeastOneUppercaseLetter, |
| 109 | + PasswordNeedsAtLeastOneSpecialCharacters, // trailing comma |
| 110 | +} |
| 111 | +``` |
| 112 | +
|
| 113 | +> I do not know really, it's maybe my habit taken from python, to have commas also in the last element of enum/collection, because if I add new element in the future, i didn't have to worry about some errors in regards to missing comma :) |
| 114 | +
|
| 115 | +--- |
| 116 | +
|
| 117 | +> A: You can specify a size of vector in constructor :) |
| 118 | +
|
| 119 | +```cpp |
| 120 | +std::vector<std::shared_ptr<int>> resultVector(count); |
| 121 | +``` |
| 122 | + |
| 123 | +> B: Yes, but what about situation, when count is negative value? I do not think such value should be used in the vector constructor, that's why I do such check first. |
| 124 | +
|
| 125 | +```cpp |
| 126 | +std::vector<std::shared_ptr<int>> resultVector{}; |
| 127 | +if (count < 0) { |
| 128 | + return resultVector; |
| 129 | +} |
| 130 | +resultVector.reserve(count); |
| 131 | +``` |
| 132 | +
|
| 133 | +> A: you are right :) , my fault :) |
| 134 | +
|
| 135 | +> B: No problem, thanks for review :) |
| 136 | +
|
| 137 | +https://github.com/coders-school/kurs_cpp_podstawowy/pull/254/files |
| 138 | +
|
| 139 | +--- |
| 140 | +
|
| 141 | +Max długość linii - 120. Jak formatować? |
| 142 | +
|
| 143 | +Zazwyczaj linie są długie gdy funkcja przyjmuje wiele parametrów: |
| 144 | +
|
| 145 | +```cpp |
| 146 | +int superFunction(std::vector<std::shared_ptr<int>> vectorOfSharedPointers, std::map<std::string, int> miniMap, std::unordered_set<int> hashes, std::function<void(int, int)> compareFunction) { |
| 147 | + // do sth |
| 148 | +} |
| 149 | +
|
| 150 | +// usage |
| 151 | +auto result = superFunction(mySuperVector, myMiniMap, myGreatHashTable, [](const auto & lhs, const auto & rhs) { return lhs >= rhs;}) |
| 152 | +``` |
| 153 | +
|
| 154 | +Better formatting: |
| 155 | +
|
| 156 | +```cpp |
| 157 | +int superFunction(std::vector<std::shared_ptr<int>> vectorOfSharedPointers, |
| 158 | + std::map<std::string, int> miniMap, |
| 159 | + std::unordered_set<int> hashes, |
| 160 | + std::function<void(int, int)> compareFunction) { |
| 161 | + // do sth |
| 162 | +} |
| 163 | +
|
| 164 | +// usage |
| 165 | +auto result = superFunction(mySuperVector, |
| 166 | + myMiniMap, |
| 167 | + myGreatHashTable, |
| 168 | + [](const auto & lhs, const auto & rhs) { return lhs >= rhs;}); |
| 169 | +``` |
| 170 | +
|
| 171 | +Or for longer lambdas / too long first parameter which exceeds line limit: |
| 172 | +
|
| 173 | +```cpp |
| 174 | +int superFunction( |
| 175 | + std::vector<std::shared_ptr<int>> vectorOfSharedPointers, |
| 176 | + std::map<std::string, int> miniMap, |
| 177 | + std::unordered_set<int> hashes, |
| 178 | + std::function<void(int, int)> compareFunction) { |
| 179 | + // two levels of indentation above to avoid confusion. The function body below will be indented with one level |
| 180 | + // do sth |
| 181 | +} |
| 182 | +
|
| 183 | +// usage |
| 184 | +auto result = superFunction(mySuperVector, |
| 185 | + myMiniMap, |
| 186 | + myGreatHashTable, |
| 187 | + [](const auto & lhs, const auto & rhs) { |
| 188 | + return lhs >= rhs; |
| 189 | + }); |
| 190 | +``` |
| 191 | +
|
| 192 | +--- |
| 193 | +
|
| 194 | +* [Nice usage of std::map instead of ifs' ladder](https://github.com/coders-school/kurs_cpp_podstawowy/pull/252/files) |
| 195 | +* Zwracajcie uwagę na znaki końca linii na końcu dokumentu. |
| 196 | +* `enum` lub `enum class` są pod spodem wartościami całkowitymi (jakiś rodzaj inta). Nie warto przekazywać ich przez const& w celu optymalizacji. |
| 197 | +* Max 2 puste linie. Zazwyczaj wystarcza jedna. 2 puste linie nie mogą wystąpić wewnątrz żadnych bloków (zakresów), jak funkcje, klasy, enumy. |
| 198 | +* Dobra praktyka - alokowanie pojemności wektora, gdy jest z góry znana. |
| 199 | +* Kiedy stosujemy konwencje? |
| 200 | + * Współpraca grupowa - obowiązkowo. Nie ma nic gorszego niż niejednolite formatowanie w projekcie. Narzucamy wam styl zmodyfikowany styl Chromium, aby nie było przepychanek. Możecie go egzekwować do woli, w szczególności w Internal Code Review (czyli wewnątrz grupy, zanim zgłosicie nam Pull Request do sprawdzenia) |
| 201 | + * Praca indywidualna - można stosować własne upodobania, ważne żeby było jednolicie. |
| 202 | + * Nie bądźcie "code style nazi" i nie wymuszajcie na innych osobach waszego stylu formatowania, jeśli komentujecie indywidualne PR. Oczywiście fajnie gdyby wszystko było tak samo wszędzie, ale tak naprawdę co firma/projekt to spotkacie się z innym formatowaniem. Warto przywyknąć, że nie zawsze wam pasuje to co sprawdzacie. Głównie odnosi się to do nowych linii i nawiasów {}. |
| 203 | +* `#include` - ja (Łukasz) nie jestem zbyt nazistowski z komentowaniem wam że: |
| 204 | + * jest nieposortowane |
| 205 | + * nie ma nowej linii |
| 206 | + * żeby to przenosić do cpp zamiast trzymać w hpp |
| 207 | + * usunąć, bo nieużywane. |
| 208 | + |
| 209 | + Tylko jak mi się rzuci w oczy to daję taki komentarz. Ale wiedzcie, że to są dobre praktyki i warto je stosować. |
| 210 | + |
| 211 | +* Też nie bądźcie nazistami i nie róbcie całego code review tylko po to, żeby komuś wytknąć nieposortowane headery lub brakujące {} w jednym miejscu. Podczas projektów grupowych takie rzeczy sobie nawytykacie wewnątrz grup i tak się najszybciej nauczycie :) Na zewnątrz do sprawdzenia ma wychodzić kod przejrzany przez pozostałe osoby z grupy. Nie ma zwalania winy, że to pisał X i on nie dopilnował. Cała grupa obrywa równo ;) |
| 212 | + |
| 213 | +--- |
| 214 | + |
| 215 | +## Współpraca grupowa - Scrum |
| 216 | + |
| 217 | +[Spotkania Scrumowe](ScrumFramework.pdf) |
| 218 | + |
| 219 | +[Ściąga ze Scruma](sciaga_scrum.pdf) |
| 220 | + |
| 221 | +--- |
| 222 | + |
| 223 | +## Scrum - dobre praktyki |
| 224 | + |
| 225 | +* **Daily meetings** - codzienne spotkania w celu zsynchronizowania się, zobaczenia jak idzie robota i w szczególności, komu trzeba pomóc, bo utknął. |
| 226 | +* **Podejście iteracyjne** - dostarczamy małe kawałki funkcjonalności. Ważne żeby jak najszybciej dostarczyć cokolwiek, co powoduje, że choćby tylko jeden test przechodzi. Potem każdy może nad tym nadbudowywać kolejne funkcjonalności. |
| 227 | +* **Retrospektywy** - wg mnie najważniejsze spotkanie. Odbywa się po zakończonej iteracji (sprincie) i zespół klepie się po pleckach jak to było wspaniale, jak dużo się udało zrobić i jak to można jeszcze polepszyć. Poza tym zespół wrzuca sobie najgorsze epitety odnośnie nieumiejętności współpracy, wyrobienia się na czas i poprawnego zaplanowania swojej pracy. Po tym spotkaniu muszą być sformułowane wnioski co można poprawić, aby lepiej się współpracowało oraz akcje do zrobienia dla każdej osoby. |
| 228 | + |
| 229 | +--- |
| 230 | + |
| 231 | +## Pytania z kanału #powtórka |
| 232 | + |
| 233 | +@edmundoPL |
| 234 | +> Pytanie ode mnie na powtórkę. O co chodzi z tym zapewnianiem odpowiednio długiego życia zmiennej, na która wskazuje pointer lub referencja? |
| 235 | +> Pojawiło się to już kilka razy, a ja nadal jakoś tego nie czuje. Niby trochę rozumiem ale nie do końca. Może jakiś ciekawy przykład się znajdzie do czwartku? |
| 236 | +
|
| 237 | +Prześledźmy stos w tym kawałku kodu |
| 238 | + |
| 239 | +```cpp |
| 240 | +#include <iostream> |
| 241 | + |
| 242 | +int doNothing() { |
| 243 | +/* G */ int b = 50; |
| 244 | + int c = 100; |
| 245 | +/* H */ return b; |
| 246 | +} |
| 247 | + |
| 248 | +int* getObject() { |
| 249 | +/* C */ int a = 10; |
| 250 | + int* ptr = &a; |
| 251 | +/* D */ return ptr; |
| 252 | +} |
| 253 | + |
| 254 | +int main() { |
| 255 | +/* A */ int number = 5; |
| 256 | +/* B */ int* pointer = getObject(); |
| 257 | +/* E */ *pointer = 20; |
| 258 | +/* F */ number = doNothing(); |
| 259 | +/* I */ std::cout << *pointer << '\n'; |
| 260 | +} |
| 261 | +``` |
| 262 | + |
| 263 | +Stos graficznie. Prawo -> kolejne linie kodu |
| 264 | + |
| 265 | +| FRAME | A | B | C | D | E | F | G | H | I | |
| 266 | +| :---: | :-------------- | :------------- | :---------- | :----------- | :------------------- | :----------- | :-------------- | :----------- | :-------------- | |
| 267 | +| 1 | | | **ptr = ?** | **ptr = &a** | | | **c = ?** | **c = 100** | | |
| 268 | +| 1 | | | **a = ?** | **a = 10** | | **??? = 20** | **b = ? (20?)** | **b = 50** | | |
| 269 | +| 0 | **pointer = ?** | pointer = ? | pointer = ? | pointer = ? | **pointer = &a** !!! | pointer = &a | pointer = &a | pointer = &a | pointer = &a | |
| 270 | +| 0 | **number = ?** | **number = 5** | number = 5 | number = 5 | number = 5 | number = 5 | number = 5 | number = 5 | **number = 50** | |
| 271 | + |
| 272 | +--- |
| 273 | + |
| 274 | +@Jakub J |
| 275 | +> Generyczne lambdy, jaki jest w takim razie sens stosowania zwykłych lambd(w których przekazujemy konkretny typ)? |
| 276 | +
|
| 277 | +Ograniczony. Tylko gdy chcemy aby lambda zadziałała dla określonego typu i np nie skompilowała się dla innego. |
| 278 | +Normalnie zalecam stosowanie generycznych. W kodzie można się natknąć na takie i takie. Generyczne weszły w C++14 jeszcze się dobrze nie upowszechniły. Ale mocno do nich zachęcam. |
| 279 | + |
| 280 | +--- |
| 281 | + |
| 282 | +@lisie_sprawy |
| 283 | +> Przeklejam z innego kanału, żeby nie uciekło :) czemu w niektórych zadaniach związanych z wywaleniem samogłosek pojawia się np. "constexpr int", zamiast "const int"? |
| 284 | +
|
| 285 | +`constexpr` w kontekście zmiennej to taki silniejszy const. Oprócz tego, że jest to stała i nie zmienimy jej wartości to kompilator może dodatkowo optymalizować wszelkie obliczenia wykonywane z jej użyciem i po prostu już podczas kompilacji obliczyć wynik i wstawić go w odpowiednie miejsca jako stałe programu. Jak to robić to się nauczymy na nowoczesnym C++, gdzie bardziej fascynujący będzie temat funkcji `constexpr`, a nie zmiennych (stałych). |
| 286 | + |
| 287 | +W każdym bądź razie jest to optymalizacja i warto mieć odruch pisania constexpr od razu. Jeśli gdzieś kompilator zaprotestuje, to znaczy że się nie da i zostawiamy sam const. Od C++20 `constexpr` wejdzie w użycie jeszcze powszechniej, bo dużo ograniczeń funkcji `constexpr` zostanie zniesionych. |
| 288 | + |
| 289 | +Do zapamiętania na teraz: staraj się domyślnie używać `constexpr` przy definiowaniu stałych zamiast const. |
| 290 | + |
| 291 | +--- |
| 292 | + |
| 293 | +## Wnioski po konkursie |
| 294 | + |
| 295 | +* Formułowanie zadań / wymagań to sztuka. Jakby dokładnie tego nie zrobić to zawsze będzie wiele osób, które będzie potrafiło inaczej zinterpretować wymagania niż zakładał autor. [Polecam motzno to wideo](https://www.youtube.com/watch?v=Ct-lOOUqmyY) |
| 296 | +* Ilu trenerów tyle opinii na temat wymagań |
| 297 | +* Ilu trenerów tyle opinii na temat styli kodowania |
0 commit comments