* Code Review Draft *.md

Status	🚧
Tags	code-review, principles

General Code Review Principles

• Always try to get familiar with the problem domain at least for some adequate time period before asking questions (make an initial investigation, check existing codebase, find confluence pages, read the docs, prepare a list of concrete questions, etc).

• Try to predict possible edge cases by yourself (you don't need to implement all of them, but you should be aware of the probability).

• Do not commit if it does NOT work.

• Do not commit if it works, but you don't know how.

• Do not commit if you don't know how to debug.

• Do not commit if you don't know how to test.

• Do not commit any redundant code.

• Do not commit if you are unsure is it really necessary.

• Do not commit if your feature is not complete (except cases when it is under disabled feature toggle).

• Do not commit if you haven't executed your code at least a couple of times locally.

• Do not trust your IDE.

• Do not trust copied/pasted code. Always analyze copied/pasted code, if it existed before, it does not automatically mean that it is perfect, it does not even mean that it works.

• Do not commit a complex solution if you have no plans to support it.

• Do not commit a complex solution if you are not going to provide any documentation for it.

• Do not commit if you don't match the technical requirements.

• Do not commit if you don't match the business requirements.

• Do not commit if you are tired.

• Avoid error shadowing.

• Always log entry results.

• Write small, simple, safe, straightforward, predictable, isolated, self-descriptive services as much as it is possible.

• Write reliable specs that fail when the source is changed. Check different input combinations, e.g: zero, one, many, none, some, all.

• Do not commit without specs.

• Do not use git revert!!! Always prefer the creation of a new branch and cherry-picking into it.
  TODO(marian13): Minimal example why not.

• If you are working with an old class, that was not covered by specs before, that does NOT mean, that you don't need to write new specs for it! Cover by specs at least your logic.

• Express your intent inside code via naming, comments, links, docs, etc as much as it is possible.

• Avoid spelling and grammar mistakes (Grammarly is your friend).

• Always read the ticket description before code review.

• Do not take code review comments too literally, they are not silver bullets. They can not be used as all-in-one solutions. For example, if someone asks you to remove the hardcode, it does not mean you have to do it all the time. Context matters! Think about it! Always!

• When you can NOT follow any of the principles above for whatever reason - always provide an explicit explanation why in a written, visual form.

comparison.md

Status	🚧
Tags	comparison

Prefer ==

consistent_return.md

Status	🚧
Tags	return, nullity-checks

Consistent return values to decrease nullity checks

Arrays

# bad - `nil` or array.
def fresh_fruits 
  return if fruits.none?

  fruits.map(&:fresh?)
end

# `fresh_fruits` author forces `fresh_fruits` user to check for nullity.
fresh_fruits.sum(&:weight) if fresh_fruits

# good - always array.
def fresh_fruits 
  return [] if fruits.none?

  fruits.map(&:fresh?)
end

# `fresh_fruits` user is safe to rely that `#sum` is available.
fresh_fruits.sum(&:weight)

# better - no `if` at all.
def fresh_fruits 
  fruits.map(&:fresh?)
end

# `fresh_fruits` user is safe to rely that `#sum` is available.
fresh_fruits.sum(&:weight)

Strings

# bad - `nil` or string.
def phone_number
  return if country_code.blank?
  return if number.blank?

  "#{country_code}#{number}"
end

# `phone_number` author forces `phone_number` user to check for nullity.
phone_number.end_with?("9") if phone_number

# good - always string
def phone_number
  return "" if country_code.blank?
  return "" if number.blank?

  "#{country_code}#{number}"
end

# `phone_number` user is safe to rely that `#end_with?` is available.
phone_number.end_with?("9")

Hashes

# bad - `nil` or hash.
def hidden_attributes
  return if user.admin?

  {passport: passport, credit_card: credit_card}
end

# `hidden_attributes` author forces `hidden_attributes` user to check for nullity.
hidden_attributes[:passport] if hidden_attributes

# good - always hash.
def hidden_attributes
  return {} if user.admin?

  {passport: passport, credit_card: credit_card}
end

# `hidden_attributes` user is safe to rely that `#[]` is available.
hidden_attributes[:passport]

data-test-id.md

Status	🚧
Tags	data-test-id

data-test-id

TODO:

datetime-data-attribute.md

Status	🚧
Tags	comment

DateTime data attribute with iso8601

TODO:

gistfile1.txt

| | |
| - | - |
| Status | 🚧 |
| Tags | `comment` |

## Order-independent code

gistfile2.txt

# Clean Code


- Accumulation effect

primitive_obssession.md

Status	🚧
Tags	primitive-obsession

Avoid Primitive Obsession

Primitive Obsession.

reason_to_create_method.md

Status	🚧
Tags	method

Reason to create a method

• Caching

  # before
  
  jazz = {
    checker: Services::IsJazz.result(params: params),
    factory: Entities::Factories::Jazz
  }

  # after
  
  def jazz
    @jazz ||= {
      checker: Services::IsJazz.result(params: params),
      factory: Entities::Factories::Jazz
    }
  end

reasons_to_create_methods.md

Status	🚧
Tags	method

• Additional information.

  # before
  
  params[:warehouses].any?

  # after
  
  def possible_delivery? 
    params[:warehouses].any?
  end

• Additional behaviour.

  For example, caching:

  # before
  
  jazz = {
    checker: Services::IsJazz.result(params: params),
    factory: Entities::Factories::Jazz
  }

  # after
  
  def jazz
    @jazz ||= {
      checker: Services::IsJazz.result(params: params),
      factory: Entities::Factories::Jazz
    }
  end

standard_ruby.md

Status	🚧
Tags	linter, config

Standard Ruby as Rubocop configuration

Use Standard Ruby for lintiing Ruby code.

unless.md

Status	🚧
Tags	unless

No complex unless

# bad - multiple conditions.
def sign
  return unless document.present? && document.signed? 
  
  # ...
end

# bad - multiple lines.
def sign
  unless document.present?
    # ...
    
    return
  end

  # ...
end

# bad - alternative without negation is available: `return if document.blank?`.
def sign
  return unless document.present?

  # ...
end

# good - single line, single condition, no alternative without negation.
def sign
  return unless document.pdf?

  # ...
end