From 66762a79db6916c44feb4a4a1aaa6c1ac44371cd Mon Sep 17 00:00:00 2001 From: abreujp Date: Sat, 17 May 2025 15:12:29 -0300 Subject: [PATCH 1/6] docs: enhance non-assertive map access anti-pattern documentation - Add comparison table for access notations - Improve explanations with clearer examples - Add example of error propagation - Include guidance for when to use each access method - Format examples for better readability --- .../pages/anti-patterns/code-anti-patterns.md | 62 ++++++++++++++++++- 1 file changed, 60 insertions(+), 2 deletions(-) diff --git a/lib/elixir/pages/anti-patterns/code-anti-patterns.md b/lib/elixir/pages/anti-patterns/code-anti-patterns.md index 4c9552a6cd1..f192cd5236d 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -324,6 +324,13 @@ When a key is optional, the `map[:key]` notation must be used instead. This way, 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 unexpectedly missing, you will have a `nil` value propagate through the system, instead of raising on map access. +*Table: Comparison of Map Access Notations* + +| Access Notation | Key Exists | Key Doesn't Exist | Use Case | +| --------------- | ----------------- | ----------------- | ------------- | +| `map.key` | Returns the value | Raises `KeyError` | Required keys | +| `map[:key]` | Returns the value | Returns `nil` | Optional keys | + #### Example The function `plot/1` tries to draw a graphic to represent the position of a point in a Cartesian plane. This function receives a parameter of `Map` type with the point attributes, which can be a point of a 2D or 3D Cartesian coordinate system. This function uses dynamic access to retrieve values for the map keys: @@ -359,6 +366,20 @@ iex> Graphics.plot(bad_point) The behavior 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. +Here's another example showing how the non-assertive map access can propagate through the system: + +```elixir +iex> point_without_x = %{y: 10} +%{y: 10} +iex> {x, y, _} = Graphics.plot(point_without_x) +{nil, 10, nil} +iex> distance_from_origin = :math.sqrt(x * x + y * y) +** (ArithmeticError) bad argument in arithmetic expression + :erlang.*(nil, nil) +``` + +The error occurs much later in the code because `nil` (from missing `:x`) is invalid for arithmetic operations. This makes it harder to identify the original issue, which was using a point without the expected `:x` coordinate. + #### Refactoring 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: @@ -380,9 +401,34 @@ iex> Graphics.plot(bad_point) graphic.ex:4: Graphics.plot/1 ``` +This is beneficial because: + +1. It makes your expectations clear to others reading the code +2. It fails fast when required data is missing +3. It allows the compiler to provide warnings when accessing non-existent fields, particularly in compile-time structures like structs + 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 alternative to refactor this anti-pattern is to use pattern matching, defining explicit clauses for 2d vs 3d points: +Here's a quick reference for when to use each access method: + +* Use `map.key` when: + * The key is required for your function to work correctly + * You want to fail fast if the key is missing + * You're working with structs + +* Use `map[:key]` when: + * The key is truly optional + * You have code that handles the `nil` case + * You need to access keys dynamically (using variables) + +Remember that using `Map.get/3` is another option when you want to provide a default value other than `nil`: + +```elixir +# Returns "default" if :some_key doesn't exist +Map.get(map, :some_key, "default") +``` + +An alternative to refactor this anti-pattern is to use pattern matching, defining explicit clauses for 2D vs 3D points: ```elixir defmodule Graphics do @@ -400,7 +446,19 @@ defmodule Graphics do end ``` -Pattern-matching is specially useful when matching over multiple keys as well as on the values themselves at once. +Pattern-matching is specially useful when matching over multiple keys as well as on the values themselves at once. In the example above, the code will not only extract the values but also verify that the required keys exist. If we try to call `plot/1` with a map that doesn't have the required keys, we'll get a `FunctionClauseError`: + +```elixir +iex> incomplete_point = %{x: 5} +%{x: 5} +iex> Graphics.plot(incomplete_point) +** (FunctionClauseError) no function clause matching in Graphics.plot/1 + + The following arguments were given to Graphics.plot/1: + + # 1 + %{x: 5} +``` 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: From e4ef581ea62ca24d594961c3d86c15fe92f956bb Mon Sep 17 00:00:00 2001 From: abreujp Date: Mon, 19 May 2025 06:56:48 -0300 Subject: [PATCH 2/6] docs: enhance map access table and examples - Add "Map is `nil`" column to map access table - Include Map.get and Map.fetch! access methods - Combine error propagation examples - Add summary section on pattern-matching --- .../pages/anti-patterns/code-anti-patterns.md | 27 ++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/lib/elixir/pages/anti-patterns/code-anti-patterns.md b/lib/elixir/pages/anti-patterns/code-anti-patterns.md index f192cd5236d..c622404f5c9 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -326,10 +326,12 @@ When you use `map[:key]` to access a key that always exists in the map, you are *Table: Comparison of Map Access Notations* -| Access Notation | Key Exists | Key Doesn't Exist | Use Case | -| --------------- | ----------------- | ----------------- | ------------- | -| `map.key` | Returns the value | Raises `KeyError` | Required keys | -| `map[:key]` | Returns the value | Returns `nil` | Optional keys | +| Access Notation | Map is `nil` | Key Exists | Key Doesn't Exist | Use Case | +| --------------- | ------------ | ---------- | ----------------- | -------- | +| `map.key` | Raises `KeyError` | Returns the value | Raises `KeyError` | Required map, required atom keys | +| `map[:key]` | Returns `nil` | Returns the value | Returns `nil` | Optional map, optional keys | +| `Map.get(map, :key)` | Raises `ArgumentError` | Returns the value | Returns `nil` | Required map, optional keys | +| `Map.fetch!(map, "key")` | Raises `ArgumentError` | Returns the value | Raises `KeyError` | Required map, required keys (works with any key type) | #### Example @@ -362,13 +364,6 @@ iex> bad_point = %{y: 3, z: 4} %{y: 3, z: 4} iex> Graphics.plot(bad_point) {nil, 3, 4} -``` - -The behavior 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. - -Here's another example showing how the non-assertive map access can propagate through the system: - -```elixir iex> point_without_x = %{y: 10} %{y: 10} iex> {x, y, _} = Graphics.plot(point_without_x) @@ -378,7 +373,7 @@ iex> distance_from_origin = :math.sqrt(x * x + y * y) :erlang.*(nil, nil) ``` -The error occurs much later in the code because `nil` (from missing `:x`) is invalid for arithmetic operations. This makes it harder to identify the original issue, which was using a point without the expected `:x` coordinate. +The behavior 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. As shown in the last example, the error occurs much later in the code because `nil` (from missing `:x`) is invalid for arithmetic operations. This makes it harder to identify the original issue, which was using a point without the expected `:x` coordinate. #### Refactoring @@ -471,6 +466,14 @@ end 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. +In summary, Elixir provides several ways to access map values, each with different behaviors: + +1. **Static access** (`map.key`): Fails fast when keys are missing, ideal for required fields +2. **Dynamic access** (`map[:key]`): Handles missing keys and nil maps by returning `nil`, suitable for optional fields +3. **Pattern matching**: Provides a powerful way to both extract values and ensure required keys exist in one operation + +Choosing the right approach depends on your requirements and can significantly impact the reliability and maintainability of your code. + #### Additional remarks This anti-pattern was formerly known as [Accessing non-existent map/struct fields](https://github.com/lucasvegi/Elixir-Code-Smells#accessing-non-existent-mapstruct-fields). From d4084b57a2e6b8138e6d089e1a784232277058c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 20 May 2025 09:19:12 +0200 Subject: [PATCH 3/6] Update code-anti-patterns.md --- lib/elixir/pages/anti-patterns/code-anti-patterns.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/elixir/pages/anti-patterns/code-anti-patterns.md b/lib/elixir/pages/anti-patterns/code-anti-patterns.md index c622404f5c9..dc29c23763b 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -364,6 +364,11 @@ iex> bad_point = %{y: 3, z: 4} %{y: 3, z: 4} iex> Graphics.plot(bad_point) {nil, 3, 4} +``` + +The behavior 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, as shown next: + +```iex iex> point_without_x = %{y: 10} %{y: 10} iex> {x, y, _} = Graphics.plot(point_without_x) @@ -373,7 +378,7 @@ iex> distance_from_origin = :math.sqrt(x * x + y * y) :erlang.*(nil, nil) ``` -The behavior 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. As shown in the last example, the error occurs much later in the code because `nil` (from missing `:x`) is invalid for arithmetic operations. This makes it harder to identify the original issue, which was using a point without the expected `:x` coordinate. +The error above occurs later in the code because `nil` (from missing `:x`) is invalid for arithmetic operations, making it harder to identify the original issue. #### Refactoring From a97338ac2e96958375035c9008384d72faa7cf9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 20 May 2025 09:32:52 +0200 Subject: [PATCH 4/6] Apply suggestions from code review --- .../pages/anti-patterns/code-anti-patterns.md | 39 +++++-------------- 1 file changed, 9 insertions(+), 30 deletions(-) diff --git a/lib/elixir/pages/anti-patterns/code-anti-patterns.md b/lib/elixir/pages/anti-patterns/code-anti-patterns.md index dc29c23763b..2f934ad54de 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -326,12 +326,10 @@ When you use `map[:key]` to access a key that always exists in the map, you are *Table: Comparison of Map Access Notations* -| Access Notation | Map is `nil` | Key Exists | Key Doesn't Exist | Use Case | -| --------------- | ------------ | ---------- | ----------------- | -------- | -| `map.key` | Raises `KeyError` | Returns the value | Raises `KeyError` | Required map, required atom keys | -| `map[:key]` | Returns `nil` | Returns the value | Returns `nil` | Optional map, optional keys | -| `Map.get(map, :key)` | Raises `ArgumentError` | Returns the value | Returns `nil` | Required map, optional keys | -| `Map.fetch!(map, "key")` | Raises `ArgumentError` | Returns the value | Raises `KeyError` | Required map, required keys (works with any key type) | +| Access notation | Key exists | Key doesn't exist | Use case | +| --------------- | ---------- | ----------------- | -------- | +| `map.key` | Returns the value | Raises `KeyError` | Structs and maps with known atom keys | +| `map[:key]` | Returns the value | Returns `nil` | Any `Access`-based data structure, optional keys | #### Example @@ -407,26 +405,7 @@ This is beneficial because: 2. It fails fast when required data is missing 3. It allows the compiler to provide warnings when accessing non-existent fields, particularly in compile-time structures like structs -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. - -Here's a quick reference for when to use each access method: - -* Use `map.key` when: - * The key is required for your function to work correctly - * You want to fail fast if the key is missing - * You're working with structs - -* Use `map[:key]` when: - * The key is truly optional - * You have code that handles the `nil` case - * You need to access keys dynamically (using variables) - -Remember that using `Map.get/3` is another option when you want to provide a default value other than `nil`: - -```elixir -# Returns "default" if :some_key doesn't exist -Map.get(map, :some_key, "default") -``` +Overall, the usage of `map.key` and `map[:key]` encode important information about your data structure, allowing developers to be clear about their intent. The `Access` module documentation also provides useful reference on this topic. You can also consider the `Map` module when working with maps of any keys, which contains functions for fetching keys (with or without default values), updating and removing keys, traversals, and more. An alternative to refactor this anti-pattern is to use pattern matching, defining explicit clauses for 2D vs 3D points: @@ -473,11 +452,11 @@ Generally speaking, structs are useful when sharing data structures across modul In summary, Elixir provides several ways to access map values, each with different behaviors: -1. **Static access** (`map.key`): Fails fast when keys are missing, ideal for required fields -2. **Dynamic access** (`map[:key]`): Handles missing keys and nil maps by returning `nil`, suitable for optional fields -3. **Pattern matching**: Provides a powerful way to both extract values and ensure required keys exist in one operation +1. **Static access** (`map.key`): Fails fast when keys are missing, ideal for structs and maps with known atom keys +2. **Dynamic access** (`map[:key]`): Works with any `Access` data structure, suitable for optional fields, returns nil for missing keys +3. **Pattern matching**: Provides a powerful way to both extract values and ensure required map/struct keys exist in one operation -Choosing the right approach depends on your requirements and can significantly impact the reliability and maintainability of your code. +Choosing the right approach depends if the keys are known upfront or not. Static access and pattern matching are mostly equivalent (although pattern matching allows you to match on multiple keys at once, including matching on the struct name). #### Additional remarks From fd56a8c806962e3ed5b2b983607935c751aa8c96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 20 May 2025 14:30:31 +0200 Subject: [PATCH 5/6] Update lib/elixir/pages/anti-patterns/code-anti-patterns.md --- lib/elixir/pages/anti-patterns/code-anti-patterns.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/elixir/pages/anti-patterns/code-anti-patterns.md b/lib/elixir/pages/anti-patterns/code-anti-patterns.md index 2f934ad54de..721684133af 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -324,7 +324,7 @@ When a key is optional, the `map[:key]` notation must be used instead. This way, 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 unexpectedly missing, you will have a `nil` value propagate through the system, instead of raising on map access. -*Table: Comparison of Map Access Notations* +##### Table: Comparison of Map Access Notations | Access notation | Key exists | Key doesn't exist | Use case | | --------------- | ---------- | ----------------- | -------- | From ed44b247b797551f9e8c9f323abca2736b90c025 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 20 May 2025 14:30:50 +0200 Subject: [PATCH 6/6] Update lib/elixir/pages/anti-patterns/code-anti-patterns.md --- lib/elixir/pages/anti-patterns/code-anti-patterns.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/elixir/pages/anti-patterns/code-anti-patterns.md b/lib/elixir/pages/anti-patterns/code-anti-patterns.md index 721684133af..921717d946c 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -324,7 +324,7 @@ When a key is optional, the `map[:key]` notation must be used instead. This way, 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 unexpectedly missing, you will have a `nil` value propagate through the system, instead of raising on map access. -##### Table: Comparison of Map Access Notations +##### Table: Comparison of map access notations | Access notation | Key exists | Key doesn't exist | Use case | | --------------- | ---------- | ----------------- | -------- |