Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ricardo Pena - PR - adding my solution #8

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

TheRick88
Copy link

@TheRick88 TheRick88 commented Jul 10, 2022


Ricardo Pena - solution code review
about: A template PR for code review, complete with checklist
title: 'Code Review: challenge name, strategy name'

Before the reviewer merges this PR to main/master, all of these boxes must be
checked. Only the reviewer can check boxes.

Behavior

What the function does - inputs and outputs

  • The Function
    • The function's name is clear and helpful
    • The parameter names are clear and helpful
    • Default parameters are used if possible
  • JSDoc Comment
    • The docstring is well-formatted
    • There are no spelling mistakes
    • The description of the function's behavior is clear
    • All parameters are documented with types
    • Documented parameter names match the function's parameter names
    • Parameters have a good description
    • If default parameters are used, they are the correct type
    • The return value is documented with a type
    • The return value has a clear description
  • README
    • All the sections in the solution README are complete
    • Descriptions in the README are thorough and clear
    • There is at least 2 use-cases
  • Unit Tests
    • Unit tests are present
    • There are at least 20 test cases
    • All describes and its have clear and helpful names
    • All test names actually describe what is being tested
    • Tests are grouped logically using describe
    • There is only one expect inside every it
    • There is no logic (loops, conditionals, ...) in the test cases
    • Side-effects are tested, if necessary
    • All tests pass in the CI check
    • The tests have 100% coverage of the solution
    • Tricky edge cases are tested (extra credit)

Strategy

The logic behind the solution

  • The Function
    • Variables have names that help understand the strategy
    • Spacing and formatting help to understand the strategy
    • The code is written as simply and clearly as possible
    • Use short, clear comments to describe the strategy
  • README
    • There is an explanation of the solution's strategy in the README
    • The explanation is clear and understandable
    • The explanation actually matches the function that was written

Implementation

The code used to write the solution

  • Formatting
    • No formatting errors appear in the linting check
  • Linting
    • There should be no linting errors or warnings in the CI check
    • Spelling should be correct in all code, comments and docstrings
  • The solution file
    • Has a JSDoc comment
    • Has an exported function
    • Does no other code or comments outside of the function body (you can use
      sandbox.js for saving extra ideas)
  • The function body Should ...
    • Be well and consistently formatted
    • Have clear and helpful variable names
    • Declare variables in the smallest scope possible
    • Be a simple and readable implementation
  • The function body should Not ...
    • Have extra comments that are not about the solution's strategy
    • Have any user interactions (prompt, alert, confirm)
    • Have any console calls (.log, .assert, ...)
    • Have any debugger statements

@TheRick88 TheRick88 requested a review from Dnyandeo33 July 10, 2022 11:15
@TheRick88 TheRick88 self-assigned this Jul 10, 2022
@TheRick88 TheRick88 requested a review from Haneefa-Shaik July 10, 2022 13:20
Copy link
Contributor

@Dnyandeo33 Dnyandeo33 left a comment

Choose a reason for hiding this comment

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

Good job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants