Pull Request Reviews

Can I understand this?
Does this meet the requirements?
Are there any potential issues?

PDF with presenter notes

Can I understand this?

this is why peer reviews are important
without this, you can't do the other two tasks

meet requirements
dangerous

Setting up Pull Request Reviews for success

small PR's (consider development branches)
reviewer must understand the context
the writer walk-throughs the reader through the code
in-person reviews are more productive than online-based reviews
a gateway drug to pair programming
link to STYLE.md

^ branches have been too big, getting better

Tell a great story

Functional programming is about data transformation
Tell a great story about the transformation of data with a pipeline
You are a software writer
RailsConf 2014 - Keynote: Writing Software by David Heinemeier Hansson

How to tell a great story

Start with optimizing for readability

If I don't understand something, it's an issue with how the code is expresses it's intent to the reader.
comes down to readability

7 ways to optimize for readability

Limit indentation
Ensure symmetrical code
Use intention revealing names
Think data transformation
Write clean pipelines
Make types explicit
Add documentation to explain what and why (not how)

^ Example: apps/hollar_merchandising/lib/hollar_merchandising/catalog/product_feed_shelf.ex

1. Limit indentation

limit to three levels
Sandy Metz - "Squint test"
RailsConf 2014 - All the Little Things by Sandi Metz

1. Limit indentation

Before

defp determine_and_fetch_shelf(current_user, params) do
  case fetch_personalized_sort_params(current_user, params) do
    {:ok, personalized_sort_params} ->
      Logger.log_personalized_sort_params(personalized_sort_params)

      with {:ok, shelf} <- fetch_shelf_for_personalized_homepage() do
        {shelf, Map.merge(params, personalized_sort_params)}
      else
        {:error, _} ->
          {:presentation_shelf_not_found, Map.merge(params, personalized_sort_params)}
      end

    _ ->
      try_fetch_shelf(params)
  end
end

^ aim for code that is smooth, not jagged

1. Limit indentation

After

defp determine_and_fetch_shelf(current_user, params) do
  case fetch_personalized_sort_params(current_user, params) do
    {:ok, personalized_sort_params} ->
      Logger.log_personalized_sort_params(personalized_sort_params)
      try_fetch_shelf_for_personalized_homepage()
    _ ->
      try_fetch_shelf(params)
  end
end

2. Ensure symmetrical code

Before

defp determine_and_fetch_shelf(current_user, params) do
  case fetch_personalized_sort_params(current_user, params) do
    {:ok, personalized_sort_params} ->
      Logger.log_personalized_sort_params(personalized_sort_params)

      with {:ok, shelf} <- fetch_shelf_for_personalized_homepage() do
        {shelf, Map.merge(params, personalized_sort_params)}
      else
        {:error, _} ->
          {:presentation_shelf_not_found, Map.merge(params, personalized_sort_params)}
      end

    _ ->
      try_fetch_shelf(params)
  end
end

^ make similar things look the same

2. Ensure symmetrical code

After

defp determine_and_fetch_shelf(current_user, params) do
  case fetch_personalized_sort_params(current_user, params) do
    {:ok, personalized_sort_params} ->
      try_fetch_shelf_for_personalized_homepage(params, personalized_sort_params)
    _ ->
      try_fetch_shelf(params)
  end
end

^ Not 100% sure about the name

3. Use intention revealing names

functions
descriptive parameters and variables (e.g. customer vs. pending_customer)
no magic primitives (e.g. 3.14159265359 vs. pi)

^ extract to module attributes

3.Use intention revealing names

Before

defp determine_and_fetch_shelf(current_user, params) do
  case fetch_personalized_sort_params(current_user, params) do
    {:ok, personalized_sort_params} ->
      try_fetch_shelf_for_personalized_homepage(params, personalized_sort_params)
    _ ->
      try_fetch_shelf(params)
  end
end

3. Use intention revealing names

After / 1

defp determine_and_fetch_shelf(current_user, params) do
  case fetch_personalized_sort_params(current_user, params) do
    {:ok, personalized_sort_params} ->
      try_fetch_shelf_for_personalized_home_page(params, personalized_sort_params)
    _ ->
      try_fetch_shelf_for_standard_page(params)
  end
end

3. Use intention revealing names

After / 2

defp determine_and_fetch_shelf(current_user, params) do
  if personalized_request?(current_user, params) do
    try_fetch_shelf_for_personalized_home_page(params)
  else
    try_fetch_shelf_for_standard_page(params)
  end
end

3. Use intention revealing names

After / 3

defp fetch_shelf_for_page(current_user, params) do
  if personalized_request?(current_user, params) do
    try_fetch_shelf_for_personalized_home_page(params)
  else
    try_fetch_shelf_for_standard_page(params)
  end
end

4. Think data transformation

Before

defp fetch_shelf_for_page(current_user, params) do
  if personalized_request?(current_user, params) do
    try_fetch_shelf_for_personalized_home_page(params)
  else
    try_fetch_shelf_for_standard_page(params)
  end
end

turn into a pipeline
Leverage pattern matching

Dr. René Föhring

4. Think data transformation

After

defp fetch_shelf_for_page(current_user, params) do
  current_user
  |> new_token(params)
  |> fetch_shelf
end

defp new_token(current_user, params) do
  personalized_params = fetch_personalized_sort_params(params)
  struct(Token,
    user: current_user,
    personalized_request?: personalized_request?(params),
    sort_params: personalized_params,
    params: params,
    search_params: Map.merge(params, personalized_params)
  )
end

def fetch_shelf(%Token{personalized_request?: true} = token) do
  # fetch shelf for personalized home page
end

def fetch_shelf(token) do
  #  fetch shelf for standard page
end

5. Write clean pipelines

declare "what" you want to do in the pipelines
functions implement the how
i.e. write in a declarative style
don't mix declaration with implementation
no indentation

5. Write clean pipelines

Before

defp fetch_shelf_for_page(current_user, collection_of_params) do
  collection_of_params
  |> Enum.map(fn(params) ->
    Enum.take(params, [:product_id, :taxon_id])
    )
  |> fetch_shelf(current_user)
end

5. Write clean pipelines

After / 1

defp fetch_shelf_for_page(current_user, collection_of_params) do
  collection_of_params
  |> Enum.map(&Enum.take(&1, [:product_id, :taxon_id]))
  |> fetch_shelf(current_user)
end

5. Write clean pipelines

After / 2

defp fetch_shelf_for_page(current_user, params) do
  collection_of_params
  |> filter_params
  |> fetch_shelf(current_user)
end

defp filter_params(collection_of_params) do
  Enum.map(collection_of_params, &Enum.take(&1, [:product_id, :taxon_id]))  
end

5. Write clean pipelines

with statements are pipelines too
favor pure functions
pure function has no side-effects
side-effects (hidden inputs)
- database query
- HTTP requests
- DateTime.utc_now
- Application.get_env/3 (TODO: check)
remember the functional sandwich: impure -> pure -> impure

6. Make types explicit

start with avoiding nil
gives names to concepts we can discuss and reason about
gives an explicit structure enforced by compile
encourages writing small units
related functions on that type can be extracted to a module MyType.action(my_type, other_params)
side on extracting types rather than not

6. Make types explicit

"Primitive Obsession"
Domain Modeling Made Functional - Tackle Software Complexity with Domain-Driven Design and F#

6. Make types explicit

nil was avoided with :presentation_shelf_not_found

Before

@spec shelf_and_params_for_presentation(map(), Page.t(), map()) ::
      {%Shelf{}, list(), integer, map()} |
      {%PresentationShelf{}, list(), integer, map()} |
      {:presentation_shelf_not_found, :presentation_shelf_not_found, 0, map()}
def shelf_and_params_for_presentation(current_user, page, params) do
  case determine_and_fetch_shelf(current_user, params) do
    {:presentation_shelf_not_found, query_params} ->
      {:presentation_shelf_not_found, :presentation_shelf_not_found, 0, query_params}
    {shelf, query_params} -> {
      shelf,
      filter_shelf(shelf, page, query_params),
      ShelfPagination.get_shelf_products_count_for_page(shelf, page),
      query_params
    }
  end
end

6. Make types explicit

After / 1

@spec shelf_and_params_for_presentation(map(), Page.t(), map()) :: ProductFeed.t()
def shelf_and_params_for_presentation(current_user, page, params) do
  case determine_and_fetch_shelf(current_user, params) do
    {:presentation_shelf_not_found, query_params} ->
      %ProductFeed{
        shelf: :presentation_shelf_not_found,
        paginated_shelf_products: [],
        previous_page_shelf_offset: 0,
        params: query_params
      }
    {shelf, query_params} -> %ProductFeed{
      shelf: shelf,
      paginated_shelf_products: filter_shelf(shelf, page, query_params),
      previous_page_shelf_offset: ShelfPagination.get_shelf_products_count_for_page(shelf, page),
      params: query_params
    }
  end
end

6. Make types explicit

After / 2

defmodule HollarMerchandising.Catalog.ProductFeed do
  defstruct shelf: nil, paginated_shelf_products: [], previous_page_shelf_offset: 0, params: nil
end

# ProductFeedShelf

@spec shelf_and_params_for_presentation(map(), Page.t(), map()) :: ProductFeed.t()
def shelf_and_params_for_presentation(current_user, page, params) do
  case determine_and_fetch_shelf(current_user, params) do
    {:presentation_shelf_not_found, query_params} ->
      %ProductFeed{
        shelf: :presentation_shelf_not_found,
        params: query_params
      }
    {shelf, query_params} -> %ProductFeed{
      shelf: shelf,
      paginated_shelf_products: filter_shelf(shelf, page, query_params),
      previous_page_shelf_offset: ShelfPagination.get_shelf_products_count_for_page(shelf, page),
      params: query_params
    }
  end
end

6. Make types explicit

consider constructor functions

After / 3

@spec shelf_and_params_for_presentation(map(), Page.t(), map()) :: ProductFeed.t()
def shelf_and_params_for_presentation(current_user, page, params) do
  case determine_and_fetch_shelf(current_user, params) do
    {:presentation_shelf_not_found, query_params} ->
      ProductFeed.new
        :presentation_shelf_not_found,
        query_params
      )
    {shelf, query_params} -> ProductFeed.new
      shelf,
      filter_shelf(shelf, page, query_params),
      ShelfPagination.get_shelf_products_count_for_page(shelf, page),
      query_params
    )
  end
end

7. Add documentation to explain what and why (not how)

does this add value?
how could this be improved?

defmodule HollarMerchandising.Catalog.ProductFeedShelf do
  @moduledoc """
  Groups all shelf and presentation shelf related logic for product feeds
  """
end

7 ways to optimize for readability

Limit indentation
Ensure symmetrical code
Use intention revealing names
Think data transformation
Write clean pipelines
Make types explicit
Add documentation to explain what and why (not how)

^ think of write code as story telling

nicholasjhenry/presentation.md

Pull Request Reviews

Can I understand this?

Setting up Pull Request Reviews for success

Tell a great story

How to tell a great story

Start with optimizing for readability

7 ways to optimize for readability

1. Limit indentation

1. Limit indentation

Before

1. Limit indentation

After

2. Ensure symmetrical code

Before

2. Ensure symmetrical code

After

3. Use intention revealing names

3.Use intention revealing names

Before

3. Use intention revealing names

After / 1

3. Use intention revealing names

After / 2

3. Use intention revealing names

After / 3

4. Think data transformation

Before

Dr. René Föhring

4. Think data transformation

After

5. Write clean pipelines

5. Write clean pipelines

Before

5. Write clean pipelines

After / 1

5. Write clean pipelines

After / 2

5. Write clean pipelines

6. Make types explicit

6. Make types explicit

6. Make types explicit

Before

6. Make types explicit

After / 1

6. Make types explicit

After / 2

6. Make types explicit

After / 3

7. Add documentation to explain what and why (not how)

7 ways to optimize for readability