Last pass over anti-patterns

Author: josevalim

Diff for: lib/elixir/pages/anti-patterns/code-anti-patterns.md

@@ -309,9 +309,13 @@ There are few known exceptions to this anti-pattern:
 
 #### Problem
 
-In Elixir, it is possible to access values from `Map`s, which are key-value data structures, either statically or dynamically. When a key is expected to exist in the map, it must be accessed using the `map.key` notation, which asserts the key exists. If the key does not exist, an exception is raised (and in some situations also compiler warnings), allowing developers to catch bugs early on.
+In Elixir, it is possible to access values from `Map`s, which are key-value data structures, either statically or dynamically.
 
-`map[:key]` must be used with optional keys. This way, if the informed key does not exist, `nil` is returned. When used with required keys, this return can be confusing and allow `nil` values to pass through the system, while `map.key` would raise upfront. In this way, this anti-pattern may cause bugs in the code.
+When a key is expected to exist in a map, it must be accessed using the `map.key` notation, making it clear to developers (and the compiler) that the key must exist. If the key does not exist, an exception is raised (and in some cases also compiler warnings). This is also known as the static notation, as the key is known at the time of writing the code.
+
+When a key is optional, the `map[:key]` notation must be used instead. This way, if the informed key does not exist, `nil` is returned. This is the dynamic notation, as it also supports dynamic key access, such as `map[some_var]`.
+
+When you use `map[:key]` to access a key that always exists in the map, you are making the code less clear for developers and for the compiler, as they now need to work with the assumption the key may not be there. This mismatch may also make it harder to track certain bugs. If the key is unexpected missing, you will have a `nil` value propagate through the system, instead of raising on map access.
 
 #### Example
 
@@ -321,8 +325,6 @@ The function `plot/1` tries to draw a graphic to represent the position of a poi
 defmodule Graphics do
   def plot(point) do
     # Some other code...
-
-    # Dynamic access to use point values
     {point[:x], point[:y], point[:z]}
   end
 end
@@ -331,92 +333,77 @@ end
 ```elixir
 iex> point_2d = %{x: 2, y: 3}
 %{x: 2, y: 3}
-iex> point_3d = %{x: 5, y: 6, z: nil}
-%{x: 5, y: 6, z: nil}
+iex> point_3d = %{x: 5, y: 6, z: 7}
+%{x: 5, y: 6, z: 7}
 iex> Graphics.plot(point_2d)
-{2, 3, nil}   # <= ambiguous return
+{2, 3, nil}
 iex> Graphics.plot(point_3d)
-{5, 6, nil}
+{5, 6, 7}
+```
+
+Given we want to plot both 2D and 3D points, the behaviour above is expected. But what happens if we forget to pass a point with either `:x` or `:y`?
+
+```elixir
+iex> bad_point = %{y: 3, z: 4}
+%{y: 3, z: 4}
+iex> Graphics.plot(bad_point)
+{nil, 3, 4}
 ```
 
-As can be seen in the example above, even when the key `:z` does not exist in the map (`point_2d`), dynamic access returns the value `nil`. This return can be dangerous because of its ambiguity. It is not possible to conclude from it whether the map has the key `:z` or not. If the function relies on the return value to make decisions about how to plot a point, this can be problematic and even cause errors when testing the code.
+The behaviour above is unexpected because our function should not work with points without a `:x` key. This leads to subtle bugs, as we may now pass `nil` to another function, instead of raising early on.
 
 #### Refactoring
 
-To remove this anti-pattern, whenever accessing an existing key of `Atom` type in the map, replace the dynamic `map[:key]` syntax by the static `map.key` notation. This way, when a non-existent key is accessed, Elixir raises an error immediately, allowing developers to find bugs faster. The next code illustrates the refactoring of `plot/1`, removing this anti-pattern:
+To remove this anti-pattern, we must use the dynamic `map[:key]` syntax and the static `map.key` notation according to our requirements. We expect `:x` and `:y` to always exist, but not `:z`. The next code illustrates the refactoring of `plot/1`, removing this anti-pattern:
 
 ```elixir
 defmodule Graphics do
   def plot(point) do
     # Some other code...
-
-    # Strict access to use point values
-    {point.x, point.y, point.z}
+    {point.x, point.y, point[:z]}
   end
 end
 ```
 
 ```elixir
-iex> point_2d = %{x: 2, y: 3}
-%{x: 2, y: 3}
-iex> point_3d = %{x: 5, y: 6, z: nil}
-%{x: 5, y: 6, z: nil}
 iex> Graphics.plot(point_2d)
-** (KeyError) key :z not found in: %{x: 2, y: 3} # <= explicitly warns that
-  graphic.ex:6: Graphics.plot/1                  # <= the :z key does not exist!
-iex> Graphics.plot(point_3d)
-{5, 6, nil}
+{2, 3, nil}
+iex> Graphics.plot(bad_point)
+** (KeyError) key :x not found in: %{y: 3, z: 4} # <= explicitly warns that
+  graphic.ex:4: Graphics.plot/1                  # <= the :z key does not exist!
 ```
 
 Overall, the usage of `map.key` and `map[:key]` encode important information about your data structure, allowing developers to be clear about their intent. See both `Map` and `Access` module documentation for more information and examples.
 
-An even simpler alternative to refactor this anti-pattern is to use pattern matching:
+An alternative to refactor this anti-pattern is to use pattern matching, defining explicit clauses for 2d vs 3d points:
 
 ```elixir
 defmodule Graphics do
-  def plot(%{x: x, y: y, z: z}) do
+  # 2d
+  def plot(%{x: x, y: y}) do
     # Some other code...
+    {x, y}
+  end
 
-    # Strict access to use point values
+  # 3d
+  def plot(%{x: x, y: y, z: z}) do
+    # Some other code...
     {x, y, z}
   end
 end
 ```
 
-```elixir
-iex> point_2d = %{x: 2, y: 3}
-%{x: 2, y: 3}
-iex> point_3d = %{x: 5, y: 6, z: nil}
-%{x: 5, y: 6, z: nil}
-iex> Graphics.plot(point_2d)
-** (FunctionClauseError)  no function clause matching in Graphics.plot/1
-  graphic.ex:2: Graphics.plot/1                  # <= the :z key does not exist!
-iex> Graphics.plot(point_3d)
-{5, 6, nil}
-```
-
-Pattern-matching is specially useful when matching over multiple keys at once and also when you want to match and assert on the values of a map.
+Pattern-matching is specially useful when matching over multiple keys as well as on the values themselves at once.
 
-Another alternative is to use structs. By default, structs only support static access to its fields:
+Another option is to use structs. By default, structs only support static access to its fields. In such scenarios, you may consider defining structs for both 2D and 3D points:
 
 ```elixir
-defmodule Point.2D do
+defmodule Point2D do
   @enforce_keys [:x, :y]
   defstruct [x: nil, y: nil]
 end
 ```
 
-```elixir
-iex> point = %Point.2D{x: 2, y: 3}
-%Point.2D{x: 2, y: 3}
-iex> point.x   # <= strict access to use point values
-2
-iex> point.z   # <= trying to access a non-existent key
-** (KeyError) key :z not found in: %Point{x: 2, y: 3}
-iex> point[:x] # <= by default, struct does not support dynamic access
-** (UndefinedFunctionError) ... (Point does not implement the Access behaviour)
-```
-
 Generally speaking, structs are useful when sharing data structures across modules, at the cost of adding a compile time dependency between these modules. If module `A` uses a struct defined in module `B`, `A` must be recompiled if the fields in the struct `B` change.
 
 #### Additional remarks
@@ -498,7 +485,7 @@ case some_function(arg) do
 end
 ```
 
-In particular, avoid matching solely on `_`, as shown below, as it is less clear in intent and it may hide bugs if `some_function/1` adds new return values in the future:
+In particular, avoid matching solely on `_`, as shown below:
 
 ```elixir
 case some_function(arg) do
@@ -507,6 +494,8 @@ case some_function(arg) do
 end
 ```
 
+ Matching on `_` is less clear in intent and it may hide bugs if `some_function/1` adds new return values in the future.
+
 #### Additional remarks
 
 This anti-pattern was formerly known as [Speculative assumptions](https://github.com/lucasvegi/Elixir-Code-Smells#speculative-assumptions).

Diff for: lib/elixir/pages/anti-patterns/design-anti-patterns.md

@@ -1,7 +1,6 @@
 # Design-related anti-patterns
 
-This document outlines potential anti-patterns related to your modules, functions, and the role they
-play within a codebase.
+This document outlines potential anti-patterns related to your modules, functions, and the role they play within a codebase.
 
 ## Alternative return types
 
@@ -242,66 +241,6 @@ defmodule MyApp do
 end
 ```
 
-## Propagating invalid data
-
-#### Problem
-
-This anti-pattern refers to a function that does not validate its parameters and propagates them to other functions, which can produce internal unexpected behavior. When an error is raised inside a function due to an invalid parameter value, it can be confusing for developers and make it harder to locate and fix the error.
-
-#### Example
-
-An example of this anti-pattern is when a function receives an invalid parameter and then passes it to other functions, either in the same library or in a third-party library. This can cause an error to be raised deep inside the call stack, which may be confusing for the developer who is working with invalid data. As shown next, the function `foo/1` is a user-facing API which doesn't validate its parameters at the boundary. In this way, it is possible that invalid data will be passed through, causing an error that is obscure and hard to debug.
-
-```elixir
-defmodule MyLibrary do
-  def foo(invalid_data) do
-    # Some other code...
-
-    MyLibrary.Internal.sum(1, invalid_data)
-  end
-end
-```
-
-```elixir
-iex> MyLibrary.foo(2)
-3
-iex> MyLibrary.foo("José") # With invalid data
-** (ArithmeticError) bad argument in arithmetic expression: 1 + "José"
-  :erlang.+(1, "José")
-  my_library.ex:4: MyLibrary.Internal.sum/2
-```
-
-#### Refactoring
-
-To remove this anti-pattern, the client code must validate input parameters at the boundary with the user, via guard clauses, pattern matching, or conditionals. This prevents errors from occurring elsewhere in the call stack, making them easier to understand and debug. This refactoring also allows libraries to be implemented without worrying about creating internal protection mechanisms. The next code snippet illustrates the refactoring of `foo/1`, removing this anti-pattern:
-
-```elixir
-defmodule MyLibrary do
-  def foo(data) when is_integer(data) do
-    # Some other code
-
-    MyLibrary.Internal.sum(1, data)
-  end
-end
-```
-
-```elixir
-iex> MyLibrary.foo(2) # With valid data
-3
-iex> MyLibrary.foo("José") # With invalid data
-** (FunctionClauseError) no function clause matching in MyLibrary.foo/1.
-The following arguments were given to MyLibrary.foo/1:
-
-      # 1
-      "José"
-
-  my_library.ex:2: MyLibrary.foo/1
-```
-
-#### Additional remarks
-
-This anti-pattern was formerly known as [Working with invalid data](https://github.com/lucasvegi/Elixir-Code-Smells#working-with-invalid-data).
-
 ## Unrelated multi-clause function
 
 #### Problem
@@ -377,7 +316,7 @@ end
 
 You can see this pattern in practice within Elixir itself. The `+/2` operator can add `Integer`s and `Float`s together, but not `String`s, which instead use the `<>/2` operator. In this sense, it is reasonable to handle integers and floats in the same operation, but strings are unrelated enough to deserve their own function.
 
-You will also find examples in Elixir of functions that work with any struct, such as `struct/2`:
+You will also find examples in Elixir of functions that work with any struct, which would seemingly be an occurrence of this anti-pattern, such as `struct/2`:
 
 ```elixir
 iex> struct(URI.parse("/foo/bar"), path: "/bar/baz")

Diff for: lib/elixir/pages/anti-patterns/macro-anti-patterns.md

@@ -63,7 +63,7 @@ end
 
 #### Problem
 
-**Macros** are powerful meta-programming mechanisms that can be used in Elixir to extend the language. While using macros is not an anti-pattern in itself, this meta-programming mechanism should only be used when absolutely necessary. Whenever a macro is used, but it would have been possible to solve the same problem using functions or other existing Elixir structures, the code becomes unnecessarily more complex and less readable. Because macros are more difficult to implement and reason about, their indiscriminate use can compromise the evolution of a system, reducing its maintainability.
+*Macros* are powerful meta-programming mechanisms that can be used in Elixir to extend the language. While using macros is not an anti-pattern in itself, this meta-programming mechanism should only be used when absolutely necessary. Whenever a macro is used, but it would have been possible to solve the same problem using functions or other existing Elixir structures, the code becomes unnecessarily more complex and less readable. Because macros are more difficult to implement and reason about, their indiscriminate use can compromise the evolution of a system, reducing its maintainability.
 
 #### Example
 

Diff for: lib/elixir/pages/anti-patterns/process-anti-patterns.md

@@ -6,7 +6,7 @@ This document outlines potential anti-patterns related to processes and process-
 
 #### Problem
 
-This anti-pattern refers to code that is unnecessarily organized by processes. A process itself does not represent an anti-pattern, but it should only be used to model runtime properties (such as concurrency, access to shared resources, and event scheduling). When you use a process for code organization, it can create bottlenecks in the system.
+This anti-pattern refers to code that is unnecessarily organized by processes. A process itself does not represent an anti-pattern, but it should only be used to model runtime properties (such as concurrency, access to shared resources, error isolation, etc). When you use a process for code organization, it can create bottlenecks in the system.
 
 #### Example
 
@@ -261,7 +261,9 @@ defmodule Counter do
   use GenServer
 
   @doc "Starts a counter process."
-  def start(initial_value, name \\ __MODULE__) when is_integer(initial_value) do
+  def start_link(opts \\ []) do
+    initial_valye = Keyword.get(opts, :initial_value, 0)
+    name = Keywoird.get(opts, :name, __MODULE__)
     GenServer.start(__MODULE__, initial_value, name: name)
   end
 
@@ -271,7 +273,7 @@ defmodule Counter do
   end
 
   @doc "Bumps the value of the given counter."
-  def bump(value, pid_name \\ __MODULE__) do
+  def bump(pid_name \\ __MODULE__, value) do
     GenServer.call(pid_name, {:bump, value})
   end
 
@@ -292,17 +294,17 @@ end
 ```
 
 ```elixir
-iex> Counter.start(0)
+iex> Counter.start_link()
 {:ok, #PID<0.115.0>}
 iex> Counter.get()
 0
-iex> Counter.start(15, :other_counter)
+iex> Counter.start_link(initial_value: 15, name: :other_counter)
 {:ok, #PID<0.120.0>}
 iex> Counter.get(:other_counter)
 15
-iex> Counter.bump(-3, :other_counter)
+iex> Counter.bump(:other_counter, -3)
 12
-iex> Counter.bump(7)
+iex> Counter.bump(Counter, 7)
 7
 ```
 
@@ -317,8 +319,13 @@ defmodule SupervisedProcess.Application do
   @impl true
   def start(_type, _args) do
     children = [
-      %{id: Counter, start: {Counter, :start, [0]}},
-      %{id: :other_counter, start: {Counter, :start, [0, :other_counter]}}
+      # With the default values for counter and name
+      Counter,
+      # With custom values for counter, name, and a custom ID
+      Supervisor.child_spec(
+        {Counter, name: :other_counter, initial_value: 15},
+        id: :other_counter
+      )
     ]
 
     Supervisor.start_link(children, strategy: :one_for_one, name: App.Supervisor)
@@ -332,8 +339,8 @@ iex> Supervisor.count_children(App.Supervisor)
 iex> Counter.get(Counter)
 0
 iex> Counter.get(:other_counter)
-0
-iex> Counter.bump(7, Counter)
+15
+iex> Counter.bump(Counter, 7)
 7
 iex> Supervisor.terminate_child(App.Supervisor, Counter)
 iex> Supervisor.count_children(App.Supervisor) # Only one active child