Skip to content

Latest commit

 

History

History
237 lines (191 loc) · 8.97 KB

code-review.md

File metadata and controls

237 lines (191 loc) · 8.97 KB
Raw

Przykłady z Code Review

Example:

bool doesPasswordsMatch(const std::string& password, const std::string& repeatedPassword) {
    if (password == repeatedPassword) {
        return true;
    }
    return false;
}

Better:

bool doesPasswordsMatch(const std::string& password, const std::string& repeatedPassword) {
    return password == repeatedPassword;
}

Example:

std::string getErrorMessage(ErrorCode Error) {
    switch (Error) {
        case ErrorCode::Ok:
            return "OK";
            break;
        case ErrorCode::PasswordNeedsAtLeastNineCharacters:
            return "Password Needs At Least Nine Characters";
            break;
        case ErrorCode::PasswordNeedsAtLeastOneNumber:
            return "Password Needs At Least One Number";
            break;
        case ErrorCode::PasswordNeedsAtLeastOneSpecialCharacter:
            return "Password Needs At Least One Special Character";
            break;
        case ErrorCode::PasswordNeedsAtLeastOneUppercaseLetter:
            return "Password Needs A Least One Uppercase Letter";
            break;
        case ErrorCode::PasswordsDoesNotMatch:
            return "Passwords Does Not Match";
            break;
        default:
            return "UNDEFINED ERROR";
    }
 }
 // What do you think about removing case ErrorCode::Ok: and putting it in default?
 // 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.
 // We usually don't indent case and code inside namespace

Better?:

std::string getErrorMessage(ErrorCode error) {
    switch (error) {
    case ErrorCode::PasswordNeedsAtLeastNineCharacters:
        return "Password Needs At Least Nine Characters";
    case ErrorCode::PasswordNeedsAtLeastOneNumber:
        return "Password Needs At Least One Number";
    case ErrorCode::PasswordNeedsAtLeastOneSpecialCharacter:
        return "Password Needs At Least One Special Character";
    case ErrorCode::PasswordNeedsAtLeastOneUppercaseLetter:
        return "Password Needs A Least One Uppercase Letter";
    case ErrorCode::PasswordsDoesNotMatch:
        return "Passwords Does Not Match";
    default:
        return "OK";
    }
 }

Example:

if(doesPasswordsMatch(first_pass, second_pass)) {
    return checkPasswordRules(first_pass);
} else {
    return ErrorCode::PasswordsDoesNotMatch;
}

Better:

if(doesPasswordsMatch(first_pass, second_pass)) {
    return checkPasswordRules(first_pass);
}
return ErrorCode::PasswordsDoesNotMatch;
enum class ErrorCode {
    PasswordNeedsAtLeastNineCharacters,
    PasswordNeedsAtLeastOneUppercaseLetter,
    PasswordNeedsAtLeastOneSpecialCharacters,  // trailing comma
}

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 :)

A: You can specify a size of vector in constructor :)

std::vector<std::shared_ptr<int>> resultVector(count);

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.

std::vector<std::shared_ptr<int>> resultVector{};
if (count < 0) {
    return resultVector;
}
resultVector.reserve(count);

A: you are right :) , my fault :)

B: No problem, thanks for review :)

https://github.com/coders-school/kurs_cpp_podstawowy/pull/254/files

Max długość linii - 120. Jak formatować?

Zazwyczaj linie są długie gdy funkcja przyjmuje wiele parametrów:

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) {
    // do sth
}

// usage
auto result = superFunction(mySuperVector, myMiniMap, myGreatHashTable, [](const auto & lhs, const auto & rhs) { return lhs >= rhs;})

Better formatting:

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) {
    // do sth
}

// usage
auto result = superFunction(mySuperVector,
                            myMiniMap,
                            myGreatHashTable,
                            [](const auto & lhs, const auto & rhs) { return lhs >= rhs;});

Or for longer lambdas / too long first parameter which exceeds line limit:

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) {
        // two levels of indentation above to avoid confusion.
        // The function body below will be indented with one level
    // do sth
}

// usage
auto result = superFunction(mySuperVector,
                            myMiniMap,
                            myGreatHashTable,
                            [](const auto & lhs, const auto & rhs) {
                                return lhs >= rhs;
                            });
  • Nice usage of std::map instead of ifs' ladder
  • Zwracajcie uwagę na znaki końca linii na końcu dokumentu.
  • enum lub enum class są pod spodem wartościami całkowitymi (jakiś rodzaj inta). Nie warto przekazywać ich przez const& w celu optymalizacji.
  • 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.
  • Dobra praktyka - alokowanie pojemności wektora, gdy jest z góry znana.
  • Kiedy stosujemy konwencje?
    • 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)
    • Praca indywidualna - można stosować własne upodobania, ważne żeby było jednolicie.
    • 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 {}.
  • #include - ja (Łukasz) nie jestem zbyt nazistowski z komentowaniem wam że:
    • jest nieposortowane
    • nie ma nowej linii
    • żeby to przenosić do cpp zamiast trzymać w hpp
    • usunąć, bo nieużywane.

    Tylko jak mi się rzuci w oczy to daję taki komentarz. Ale wiedzcie, że to są dobre praktyki i warto je stosować.

  • 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 ;)