|
| 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 ;) |
0 commit comments