sent-hil · June 15, 2018 18:15
diff --git a/gistfile1.txt b/gistfile1.txt
 The following below are what *I* think are best principles (after going through the last dozen mostly commented PRs):

 1. Principle of least astonishment: does every single line follow principle of least astonishment?
    This means if you, as a "reasonable" Rails programmer would be confused about this change, then this code either needs to be refactored or documented properly (there are situations that require us to break best practices, but these situations are very, very rare).

 2. Readability: can I understand everything in PR (title, description, comments, code and tests) without having to ask the PR author to explain to me what it means?
    And if there are edge case, are they documented properly?

 3. Are edge cases, user input errors accounted for?
    If the code relies on user input, what happens if the user is trying to sabotage? ie https://xkcd.com/327/
    If the user enters something unexpected do we return a reasonable error (ie error with enough information to correct the error) back?
    Can this code be run async, ie in a sidekiq job?
        If it’s run as a sidekiq job, does it have to be synchronized with what needs to be shown to the user or something else? If so, how do you prevent race conditions?

 4. Performance: will the code cause a noticeable degradation in terms of ux for user? 
    Are you using find_in_batches when getting every record from table? (why are you getting every record from database?)
    Are you returning a response to user in a "reasonable" time?
    Are you using in memory filtering or using database filtering?

 5. Is there enough information in this PR that anyone from the team can be merge it and deploy it without having to ask the author of PR how to do something before deploy to production?
    If your PR requires something like run rake task or coordinate with other teams, are they documented in the PR and labeled appropriately?

 6. Are simple things taken care off?
    * Removed all empty new lines?
    * Remove unused methods/variables?
    * Fixed rubocop complaints (for the most part)?
    * Method/variable names that overly descriptive?
    * Remove unnecessary puts, binding.pry, commented lines
       * If there’s a puts/log in there, what’s the follow up for it? Are there papertrail alerts when it happens?
    * Are user facing strings in localeapp?
    * Are there no accidentally committed changes?

 7. If it's a new feature, is it using rollouts?
    Users not in rollout should not feel the effects of user who are in rollout.

 8. Tested: Is the PR adequately tested (every code path must be touched in test at least once)?
    If lib/model is touched are there unit tests?
    If views are touched, are there integration tests?
    If routes are touched, are there routing tests?

 9. External api checks
    If the code hits an external api, what happens if the api goes down or takes too long to respond or responds with empty body?
      If request fails, are there proper logging and retry logic?
    If you’re accessing response from external api, are there defensive checks for data not being in format you would expect it to be in?
	The following below are what I think are best principles (after going through the last dozen mostly commented PRs):

	1. Principle of least astonishment: does every single line follow principle of least astonishment?
	This means if you, as a "reasonable" Rails programmer would be confused about this change, then this code either needs to be refactored or documented properly (there are situations that require us to break best practices, but these situations are very, very rare).

	2. Readability: can I understand everything in PR (title, description, comments, code and tests) without having to ask the PR author to explain to me what it means?
	And if there are edge case, are they documented properly?

	3. Are edge cases, user input errors accounted for?
	If the code relies on user input, what happens if the user is trying to sabotage? ie https://xkcd.com/327/
	If the user enters something unexpected do we return a reasonable error (ie error with enough information to correct the error) back?
	Can this code be run async, ie in a sidekiq job?
	If it’s run as a sidekiq job, does it have to be synchronized with what needs to be shown to the user or something else? If so, how do you prevent race conditions?

	4. Performance: will the code cause a noticeable degradation in terms of ux for user?
	Are you using find_in_batches when getting every record from table? (why are you getting every record from database?)
	Are you returning a response to user in a "reasonable" time?
	Are you using in memory filtering or using database filtering?

	5. Is there enough information in this PR that anyone from the team can be merge it and deploy it without having to ask the author of PR how to do something before deploy to production?
	If your PR requires something like run rake task or coordinate with other teams, are they documented in the PR and labeled appropriately?

	6. Are simple things taken care off?
	* Removed all empty new lines?
	* Remove unused methods/variables?
	* Fixed rubocop complaints (for the most part)?
	* Method/variable names that overly descriptive?
	* Remove unnecessary puts, binding.pry, commented lines
	* If there’s a puts/log in there, what’s the follow up for it? Are there papertrail alerts when it happens?
	* Are user facing strings in localeapp?
	* Are there no accidentally committed changes?

	7. If it's a new feature, is it using rollouts?
	Users not in rollout should not feel the effects of user who are in rollout.

	8. Tested: Is the PR adequately tested (every code path must be touched in test at least once)?
	If lib/model is touched are there unit tests?
	If views are touched, are there integration tests?
	If routes are touched, are there routing tests?

	9. External api checks
	If the code hits an external api, what happens if the api goes down or takes too long to respond or responds with empty body?
	If request fails, are there proper logging and retry logic?
	If you’re accessing response from external api, are there defensive checks for data not being in format you would expect it to be in?