From aa7b40cf37e4594fc5ac71f5e65e30a962e5734b Mon Sep 17 00:00:00 2001 From: Lucas Francisco da Matta Vegi Date: Fri, 14 Jul 2023 09:44:37 -0300 Subject: [PATCH 01/39] "Complex extractions in clauses" (Anti-pattern documentation) --- .../pages/anti-patterns/code-anti-patterns.md | 36 ++++++++++++++++++- 1 file changed, 35 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 7e3a96277f3..68c3a9b005b 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -171,7 +171,41 @@ end ## Complex extractions in clauses -TODO. +#### Problem + +When we use multi-clause functions, it is possible to extract values in the clauses for further usage and for pattern matching/guard checking. This extraction itself does not represent an anti-pattern, but when you have too many clauses or too many arguments, it becomes hard to know which extracted parts are used for pattern/guards and what is used only inside the function body. This anti-pattern is related to [Unrelated multi-clause function](https://github.com/elixir-lang/elixir/blob/main/lib/elixir/pages/anti-patterns/design-anti-patterns.md#unrelated-multi-clause-function), but with implications of its own. It impairs the code readability in a different way. + +#### Example + +The multi-clause function `drive/1` is extracting fields of an `%User{}` struct for usage in the clause expression (`age`) and for usage in the function body (`name`). Ideally, a function should not mix pattern matching extractions for usage in its clauses expressions and also in the function body. + +```elixir +def drive(%User{name: name, age: age}) when age >= 18 do + "#{name} can drive" +end + +def drive(%User{name: name, age: age}) when age < 18 do + "#{name} cannot drive" +end +``` + +While the example is small and looks like a clear code, try to imagine a situation where `drive/1` was more complex, having many more clauses, arguments, and extractions. This is the really smelly code! + +#### Refactoring + +As shown below, a possible solution to this anti-pattern is to extract only pattern/guard related variables in the signature once you have many arguments or multiple clauses: + +```elixir +def drive(%User{age: age} = user) when age >= 18 do + %User{name: name} = user + "#{name} can drive" +end + +def drive(%User{age: age} = user) when age < 18 do + %User{name: name} = user + "#{name} cannot drive" +end +``` ## Dynamic map fields access From efa35af635ef04baf51490c53dcff332997addd6 Mon Sep 17 00:00:00 2001 From: Lucas Francisco da Matta Vegi Date: Fri, 14 Jul 2023 10:01:30 -0300 Subject: [PATCH 02/39] "Dynamic map fields access" (Anti-pattern documentation) --- .../pages/anti-patterns/code-anti-patterns.md | 80 ++++++++++++++++++- 1 file changed, 79 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 68c3a9b005b..230f70d3617 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -209,7 +209,85 @@ end ## Dynamic map fields access -TODO. +#### Problem + +In Elixir, it is possible to access values from `Maps`, which are key-value data structures, either strictly or dynamically. When trying to dynamically access the value of a key from a `Map`, if the informed key does not exist, a null value (`nil`) will be returned. This return can be confusing and does not allow developers to conclude whether the key is non-existent in the `Map` or just has no bound value. In this way, this anti-pattern may cause bugs in the code. + +#### 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. To decide if a point is 2D or 3D, this function uses dynamic access to retrieve values of the `Map` keys: + +```elixir +defmodule Graphics do + def plot(point) do + #Some other code... + + # Dynamic access to use point values + {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) +{2, 3, nil} # <= ambiguous return +iex> Graphics.plot(point_3d) +{5, 6, nil} +``` + +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. + +#### Refactoring + +To remove this anti-pattern, whenever a `Map` has keys of `Atom` type, replace the dynamic access to its values per strict access. When a non-existent key is strictly 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: + +```elixir +defmodule Graphics do + def plot(point) do + #Some other code... + + # Strict access to use point values + {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} +``` + +As shown below, another alternative to refactor this anti-pattern is to replace a `Map` with a `struct` (named map). By default, structs only support strict access to values. In this way, accesses will always return clear and objective results: + +```elixir +defmodule Point do + @enforce_keys [:x, :y] + defstruct [x: nil, y: nil] +end +``` +```elixir +iex> point = %Point{x: 2, y: 3} +%Point{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) +``` + +#### 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). ## Dynamic atom creation From e8f5a76923eb90731b33d7ba8d92965bc1a15810 Mon Sep 17 00:00:00 2001 From: Lucas Francisco da Matta Vegi Date: Fri, 14 Jul 2023 12:33:10 -0300 Subject: [PATCH 03/39] Fixing blanks-around-fences problem (Anti-patterns documentation) --- lib/elixir/pages/anti-patterns/code-anti-patterns.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/elixir/pages/anti-patterns/code-anti-patterns.md b/lib/elixir/pages/anti-patterns/code-anti-patterns.md index 230f70d3617..fa486b8791c 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -227,6 +227,7 @@ defmodule Graphics do end end ``` + ```elixir iex> point_2d = %{x: 2, y: 3} %{x: 2, y: 3} @@ -254,6 +255,7 @@ defmodule Graphics do end end ``` + ```elixir iex> point_2d = %{x: 2, y: 3} %{x: 2, y: 3} @@ -274,6 +276,7 @@ defmodule Point do defstruct [x: nil, y: nil] end ``` + ```elixir iex> point = %Point{x: 2, y: 3} %Point{x: 2, y: 3} From 38f3036fe82d0d4093ee9894ceabc956bf3b6aa9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Sun, 16 Jul 2023 20:08:51 +0200 Subject: [PATCH 04/39] Apply suggestions from code review --- lib/elixir/pages/anti-patterns/code-anti-patterns.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/elixir/pages/anti-patterns/code-anti-patterns.md b/lib/elixir/pages/anti-patterns/code-anti-patterns.md index fa486b8791c..376b385cb33 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -173,11 +173,11 @@ end #### Problem -When we use multi-clause functions, it is possible to extract values in the clauses for further usage and for pattern matching/guard checking. This extraction itself does not represent an anti-pattern, but when you have too many clauses or too many arguments, it becomes hard to know which extracted parts are used for pattern/guards and what is used only inside the function body. This anti-pattern is related to [Unrelated multi-clause function](https://github.com/elixir-lang/elixir/blob/main/lib/elixir/pages/anti-patterns/design-anti-patterns.md#unrelated-multi-clause-function), but with implications of its own. It impairs the code readability in a different way. +When we use multi-clause functions, it is possible to extract values in the clauses for further usage and for pattern matching/guard checking. This extraction itself does not represent an anti-pattern, but when you have too many clauses or too many arguments, it becomes hard to know which extracted parts are used for pattern/guards and what is used only inside the function body. This anti-pattern is related to [Unrelated multi-clause function](design-anti-patterns.md#unrelated-multi-clause-function), but with implications of its own. It impairs the code readability in a different way. #### Example -The multi-clause function `drive/1` is extracting fields of an `%User{}` struct for usage in the clause expression (`age`) and for usage in the function body (`name`). Ideally, a function should not mix pattern matching extractions for usage in its clauses expressions and also in the function body. +The multi-clause function `drive/1` is extracting fields of an `%User{}` struct for usage in the clause expression (`age`) and for usage in the function body (`name`). Ideally, a function should not mix pattern matching extractions for usage in its guard expressions and also in its body. ```elixir def drive(%User{name: name, age: age}) when age >= 18 do @@ -189,7 +189,7 @@ def drive(%User{name: name, age: age}) when age < 18 do end ``` -While the example is small and looks like a clear code, try to imagine a situation where `drive/1` was more complex, having many more clauses, arguments, and extractions. This is the really smelly code! +While the example is small and looks like a clear code, try to imagine a situation where `drive/1` was more complex, having many more clauses, arguments, and extractions. #### Refactoring From a81d7ad56ae2e57f36f14d0d4402b9419b01dfc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Sun, 16 Jul 2023 20:17:28 +0200 Subject: [PATCH 05/39] Apply suggestions from code review --- .../pages/anti-patterns/code-anti-patterns.md | 38 ++++++++++++++++--- 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/lib/elixir/pages/anti-patterns/code-anti-patterns.md b/lib/elixir/pages/anti-patterns/code-anti-patterns.md index 376b385cb33..12863f00545 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -211,7 +211,7 @@ end #### Problem -In Elixir, it is possible to access values from `Maps`, which are key-value data structures, either strictly or dynamically. When trying to dynamically access the value of a key from a `Map`, if the informed key does not exist, a null value (`nil`) will be returned. This return can be confusing and does not allow developers to conclude whether the key is non-existent in the `Map` or just has no bound value. In this way, this anti-pattern may cause bugs in the code. +In Elixir, it is possible to access values from `Maps`, which are key-value data structures, either statically or dynamically. When trying to dynamically access the value of a key from a `Map`, if the informed key does not exist, `nil` is returned. This return can be confusing and does not allow developers to conclude whether the key is non-existent in the `Map` or just has no bound value. In this way, this anti-pattern may cause bugs in the code. #### Example @@ -243,7 +243,7 @@ As can be seen in the example above, even when the key `:z` does not exist in th #### Refactoring -To remove this anti-pattern, whenever a `Map` has keys of `Atom` type, replace the dynamic access to its values per strict access. When a non-existent key is strictly 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, whenever a `Map` has keys of `Atom` type, replace the dynamic access to its values by the `map.field`syntax. When a non-existent key is statically 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: ```elixir defmodule Graphics do @@ -268,18 +268,43 @@ iex> Graphics.plot(point_3d) {5, 6, nil} ``` -As shown below, another alternative to refactor this anti-pattern is to replace a `Map` with a `struct` (named map). By default, structs only support strict access to values. In this way, accesses will always return clear and objective results: +As shown below, another alternative to refactor this anti-pattern is to use pattern matching: ```elixir -defmodule Point do +defmodule Graphics do + def plot(%{x: x, y: y, z: z}) do + #Some other code... + + # Strict access to use point values + {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} +``` + +Another alternative is to use structs. By default, structs only support static access to its fields, promoting cleaner patterns: + +```elixir +defmodule Point.2D do @enforce_keys [:x, :y] defstruct [x: nil, y: nil] end ``` ```elixir -iex> point = %Point{x: 2, y: 3} -%Point{x: 2, y: 3} +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 @@ -287,6 +312,7 @@ iex> point.z # <= trying to access a non-existent key iex> point[:x] # <= by default, struct does not support dynamic access ** (UndefinedFunctionError) ... (Point does not implement the Access behaviour) ``` +``` #### Additional remarks From b7ce804c359b23d632b95f1d02b41bccecb54fb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Sun, 16 Jul 2023 20:18:53 +0200 Subject: [PATCH 06/39] Update lib/elixir/pages/anti-patterns/code-anti-patterns.md --- lib/elixir/pages/anti-patterns/code-anti-patterns.md | 1 - 1 file changed, 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 12863f00545..8165d5f18cf 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -312,7 +312,6 @@ iex> point.z # <= trying to access a non-existent key iex> point[:x] # <= by default, struct does not support dynamic access ** (UndefinedFunctionError) ... (Point does not implement the Access behaviour) ``` -``` #### Additional remarks From edf23e1c196994753b559a915141ecd190fdefc3 Mon Sep 17 00:00:00 2001 From: Lucas Francisco da Matta Vegi Date: Mon, 17 Jul 2023 10:10:14 -0300 Subject: [PATCH 07/39] "Dynamic atom creation" added (Documentation) --- .../pages/anti-patterns/code-anti-patterns.md | 63 ++++++++++++++++++- 1 file changed, 62 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 8165d5f18cf..ba20da6f8ae 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -319,7 +319,68 @@ This anti-pattern was formerly known as [Accessing non-existent Map/Struct field ## Dynamic atom creation -TODO. +#### Problem + +An `atom` is a primary data type of Elixir whose value is its own name. They are often useful to identify resources or express an operation's state. The creation of an `atom` do not characterize an anti-pattern by itself; however, `atoms` are not collected by Elixir's Garbage Collector, so values of this type live in memory while software is executing, during its entire lifetime. Also, BEAM limits the number of `atoms` that can exist in an application (`1_048_576`) and each `atom` has a maximum size limited to 255 Unicode code points. For these reasons, the dynamic atom creation is considered an anti-pattern, since in this way the developer has no control over how many `atoms` will be created during the software execution. This unpredictable scenario can expose the software to unexpected behavior caused by excessive memory usage, or even by reaching the maximum number of `atoms` possible. + +#### Example + +Imagine that you are implementing a code that performs the conversion of `string` values into `atoms` to identify resources. These `strings` can come from user input or even have been received as response from requests to an API. As this is a dynamic and unpredictable scenario, it is possible for identical `strings` to be converted into new `atoms` that are repeated unnecessarily. This kind of conversion, in addition to wasting memory, can be problematic for the software if it happens too often. + +```elixir +defmodule Identifier do + def generate(id) when is_bitstring(id) do + String.to_atom(id) #<= dynamic atom creation!! + end +end +``` + +```elixir +iex> string_from_user_input = "my_id" +"my_id" +iex> string_from_API_response = "my_id" +"my_id" +iex> Identifier.generate(string_from_user_input) +:my_id +iex> Identifier.generate(string_from_API_response) +:my_id #<= atom repeated was created! +``` + +When we use the `String.to_atom/1` function to dynamically create an `atom`, it is created regardless of whether there is already another one with the same value in memory, so when this happens automatically, we will not have control over meeting the limits established by BEAM. + +#### Refactoring + +To remove this anti-pattern, we must ensure that all the identifier `atoms` are created statically, only once, at the beginning of an application's execution: + +```elixir +# statically created atoms... +_ = :my_id +_ = :my_id2 +_ = :my_id3 +_ = :my_id4 +``` + +Next, you should replace the use of the `String.to_atom/1` function with the `String.to_existing_atom/1` function. This will allow string-to-atom conversions to just map the strings to atoms already in memory (statically created at the beginning of the execution), thus preventing repeated `atoms` from being created dynamically. This second part of the refactoring is presented below. + +```elixir +defmodule Identifier do + def generate(id) when is_bitstring(id) do + String.to_existing_atom(id) #<= just maps a string to an existing atom! + end +end +``` + +```elixir +iex> Identifier.generate("my_id") +:my_id +iex> Identifier.generate("my_id2") +:my_id2 +iex> Identifier.generate("non_existent_id") +** (ArgumentError) errors were found at the given arguments: +* 1st argument: not an already existing atom +``` + +Note that in the third use example, when a `string` different from an already existing `atom` is given, Elixir shows an error instead of performing the conversion. This demonstrates that this refactoring creates a more controlled and predictable scenario for the application in terms of memory usage. ## Namespace trespassing From 9ef5eb48a293a4d23a5318b6064ca0469ad4b059 Mon Sep 17 00:00:00 2001 From: Lucas Francisco da Matta Vegi Date: Mon, 17 Jul 2023 13:45:09 -0300 Subject: [PATCH 08/39] Fixing backquotes usage (Dynamic atom creation - anti-pattern) --- .../pages/anti-patterns/code-anti-patterns.md | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/elixir/pages/anti-patterns/code-anti-patterns.md b/lib/elixir/pages/anti-patterns/code-anti-patterns.md index ba20da6f8ae..96bf279c850 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -321,15 +321,15 @@ This anti-pattern was formerly known as [Accessing non-existent Map/Struct field #### Problem -An `atom` is a primary data type of Elixir whose value is its own name. They are often useful to identify resources or express an operation's state. The creation of an `atom` do not characterize an anti-pattern by itself; however, `atoms` are not collected by Elixir's Garbage Collector, so values of this type live in memory while software is executing, during its entire lifetime. Also, BEAM limits the number of `atoms` that can exist in an application (`1_048_576`) and each `atom` has a maximum size limited to 255 Unicode code points. For these reasons, the dynamic atom creation is considered an anti-pattern, since in this way the developer has no control over how many `atoms` will be created during the software execution. This unpredictable scenario can expose the software to unexpected behavior caused by excessive memory usage, or even by reaching the maximum number of `atoms` possible. +An `Atom` is a basic data type of Elixir whose value is its own name. *Atoms* are often useful to identify resources or express the state or result of an operation. Creatings *atoms* dynamically is not an anti-pattern by itself; however, *atoms* are not collected by the BEAM's garbage collector, so values of this type live in memory while software is executing, during its entire lifetime. Also, BEAM limits the number of *atoms* that can exist in an application (*1_048_576* by default) and each *atom* has a maximum size limited to 255 Unicode code points. For these reasons, creating *atoms* dynamically is considered an anti-pattern, since in this way the developer has no control over how many *atoms* will be created during the software execution. This unpredictable scenario can expose the software to unexpected behavior caused by excessive memory usage, or even by reaching the maximum number of *atoms* possible. #### Example -Imagine that you are implementing a code that performs the conversion of `string` values into `atoms` to identify resources. These `strings` can come from user input or even have been received as response from requests to an API. As this is a dynamic and unpredictable scenario, it is possible for identical `strings` to be converted into new `atoms` that are repeated unnecessarily. This kind of conversion, in addition to wasting memory, can be problematic for the software if it happens too often. +Imagine that you are implementing a code that performs the conversion of *string* values into *atoms* to identify resources. These *strings* can come from user input or even have been received as response from requests to an API. As this is a dynamic and unpredictable scenario, it is possible for identical *strings* to be converted into new *atoms* that are repeated unnecessarily. This kind of conversion, in addition to wasting memory, can be problematic for the software if it happens too often. ```elixir defmodule Identifier do - def generate(id) when is_bitstring(id) do + def generate(id) when is_binary(id) do String.to_atom(id) #<= dynamic atom creation!! end end @@ -338,7 +338,7 @@ end ```elixir iex> string_from_user_input = "my_id" "my_id" -iex> string_from_API_response = "my_id" +iex> string_from_api_response = "my_id" "my_id" iex> Identifier.generate(string_from_user_input) :my_id @@ -346,11 +346,11 @@ iex> Identifier.generate(string_from_API_response) :my_id #<= atom repeated was created! ``` -When we use the `String.to_atom/1` function to dynamically create an `atom`, it is created regardless of whether there is already another one with the same value in memory, so when this happens automatically, we will not have control over meeting the limits established by BEAM. +When we use the `String.to_atom/1` function to dynamically create an *atom*, it is created regardless of whether there is already another one with the same value in memory, so when this happens automatically, we will not have control over meeting the limits established by BEAM. #### Refactoring -To remove this anti-pattern, we must ensure that all the identifier `atoms` are created statically, only once, at the beginning of an application's execution: +To remove this anti-pattern, we must ensure that all the identifier *atoms* are created statically, only once, at the beginning of an application's execution: ```elixir # statically created atoms... @@ -360,11 +360,11 @@ _ = :my_id3 _ = :my_id4 ``` -Next, you should replace the use of the `String.to_atom/1` function with the `String.to_existing_atom/1` function. This will allow string-to-atom conversions to just map the strings to atoms already in memory (statically created at the beginning of the execution), thus preventing repeated `atoms` from being created dynamically. This second part of the refactoring is presented below. +Next, you should replace the use of the `String.to_atom/1` function with the `String.to_existing_atom/1` function. This will allow string-to-atom conversions to just map the strings to atoms already in memory (statically created at the beginning of the execution), thus preventing repeated *atoms* from being created dynamically. This second part of the refactoring is presented below. ```elixir defmodule Identifier do - def generate(id) when is_bitstring(id) do + def generate(id) when is_binary(id) do String.to_existing_atom(id) #<= just maps a string to an existing atom! end end @@ -380,7 +380,7 @@ iex> Identifier.generate("non_existent_id") * 1st argument: not an already existing atom ``` -Note that in the third use example, when a `string` different from an already existing `atom` is given, Elixir shows an error instead of performing the conversion. This demonstrates that this refactoring creates a more controlled and predictable scenario for the application in terms of memory usage. +Note that in the third use example, when a *string* different from an already existing *atom* is given, Elixir shows an error instead of performing the conversion. This demonstrates that this refactoring creates a more controlled and predictable scenario for the application in terms of memory usage. ## Namespace trespassing From add1f7e076636c079573dabad61a313a457f0b00 Mon Sep 17 00:00:00 2001 From: Lucas Francisco da Matta Vegi Date: Thu, 10 Aug 2023 11:26:22 -0300 Subject: [PATCH 09/39] "Dynamic atom creation" documentation adjustments --- .../pages/anti-patterns/code-anti-patterns.md | 74 ++++++++++--------- 1 file changed, 41 insertions(+), 33 deletions(-) diff --git a/lib/elixir/pages/anti-patterns/code-anti-patterns.md b/lib/elixir/pages/anti-patterns/code-anti-patterns.md index 96bf279c850..ee0ce808bc8 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -321,66 +321,74 @@ This anti-pattern was formerly known as [Accessing non-existent Map/Struct field #### Problem -An `Atom` is a basic data type of Elixir whose value is its own name. *Atoms* are often useful to identify resources or express the state or result of an operation. Creatings *atoms* dynamically is not an anti-pattern by itself; however, *atoms* are not collected by the BEAM's garbage collector, so values of this type live in memory while software is executing, during its entire lifetime. Also, BEAM limits the number of *atoms* that can exist in an application (*1_048_576* by default) and each *atom* has a maximum size limited to 255 Unicode code points. For these reasons, creating *atoms* dynamically is considered an anti-pattern, since in this way the developer has no control over how many *atoms* will be created during the software execution. This unpredictable scenario can expose the software to unexpected behavior caused by excessive memory usage, or even by reaching the maximum number of *atoms* possible. +An `Atom` is a basic data type of Elixir whose value is its own name. *Atoms* are often useful to identify resources or express the state or result of an operation. Creatings *atoms* dynamically is not an anti-pattern by itself; however, *atoms* are not collected by the BEAM's garbage collector, so values of this type live in memory while software is executing, during its entire lifetime. Also, BEAM limits the number of *atoms* that can exist in an application (*1_048_576* by default) and each *atom* has a maximum size limited to 255 Unicode code points. For these reasons, creating *atoms* dynamically can be considered an anti-pattern in some situations, since in this way the developer has no control over how many *atoms* will be created during the software execution. This unpredictable scenario can expose the software to unexpected behavior caused by excessive memory usage, or even by reaching the maximum number of *atoms* possible. #### Example -Imagine that you are implementing a code that performs the conversion of *string* values into *atoms* to identify resources. These *strings* can come from user input or even have been received as response from requests to an API. As this is a dynamic and unpredictable scenario, it is possible for identical *strings* to be converted into new *atoms* that are repeated unnecessarily. This kind of conversion, in addition to wasting memory, can be problematic for the software if it happens too often. +Picture yourself implementing code that converts *string* values into *atoms*. These *strings* could have been received as responses to API requests, for instance. This dynamic and unpredictable scenario poses a security risk, as these uncontrolled conversions can potentially trigger out-of-memory errors. ```elixir -defmodule Identifier do - def generate(id) when is_binary(id) do - String.to_atom(id) #<= dynamic atom creation!! +defmodule API do + def request(foo) do + #Some other code... + + case foo do + "bar" -> %{code: 110, type: "error"} + "qux" -> %{code: 112, type: "ok"} + end + end + + def generate_atom(response) when is_map(response) + String.to_atom(response.type) #<= dynamic atom creation!! end end ``` ```elixir -iex> string_from_user_input = "my_id" -"my_id" -iex> string_from_api_response = "my_id" -"my_id" -iex> Identifier.generate(string_from_user_input) -:my_id -iex> Identifier.generate(string_from_API_response) -:my_id #<= atom repeated was created! +iex> API.request("bar") |> API.generate_atom() +:error +iex> API.request("qux") |> API.generate_atom() +:ok ``` -When we use the `String.to_atom/1` function to dynamically create an *atom*, it is created regardless of whether there is already another one with the same value in memory, so when this happens automatically, we will not have control over meeting the limits established by BEAM. +When we use the `String.to_atom/1` function to dynamically create an *atom* in an API, it essentially gains potential access to create arbitrary *atoms* in our system, causing us to lose control over adhering to the limits established by the BEAM. This issue could be exploited by someone to create enough *atoms* to shut down a system. #### Refactoring -To remove this anti-pattern, we must ensure that all the identifier *atoms* are created statically, only once, at the beginning of an application's execution: +To eliminate this anti-pattern, before any string-to-atom conversion takes place, we must ensure that all potential *atoms* to be created from *string* conversions are statically defined in the module where these conversions will occur. Next, you should replace the use of the `String.to_atom/1` function with the `String.to_existing_atom/1` function. This will allow us to have greater control over which *atoms* can be created by the API, as only those *atoms* loaded at the module level will be convertible from *strings*. ```elixir -# statically created atoms... -_ = :my_id -_ = :my_id2 -_ = :my_id3 -_ = :my_id4 -``` - -Next, you should replace the use of the `String.to_atom/1` function with the `String.to_existing_atom/1` function. This will allow string-to-atom conversions to just map the strings to atoms already in memory (statically created at the beginning of the execution), thus preventing repeated *atoms* from being created dynamically. This second part of the refactoring is presented below. +defmodule API do + # statically created atoms + _ = :error + _ = :ok + + def request(foo) do + #Some other code... -```elixir -defmodule Identifier do - def generate(id) when is_binary(id) do - String.to_existing_atom(id) #<= just maps a string to an existing atom! + case foo do + "bar" -> %{code: 110, type: "error"} + "qux" -> %{code: 112, type: "ok"} + end + end + + def generate_atom(response) when is_map(response) do + String.to_existing_atom(response.type) #<= just maps a string to an existing atom! end end ``` ```elixir -iex> Identifier.generate("my_id") -:my_id -iex> Identifier.generate("my_id2") -:my_id2 -iex> Identifier.generate("non_existent_id") +iex> API.request("bar") |> API.generate_atom() +:error +iex> API.request("qux") |> API.generate_atom() +:ok +iex> API.generate_atom(%{code: 113, type: "exploit"}) ** (ArgumentError) errors were found at the given arguments: * 1st argument: not an already existing atom ``` -Note that in the third use example, when a *string* different from an already existing *atom* is given, Elixir shows an error instead of performing the conversion. This demonstrates that this refactoring creates a more controlled and predictable scenario for the application in terms of memory usage. +Note that in the third use example, when a *string* different from an already existing *atom* is given, Elixir shows an error instead of performing the conversion. This demonstrates that this refactoring creates a more controlled and predictable scenario for the application in terms of memory usage and security. ## Namespace trespassing From f3bf95ac2424b847f0206da5290b12ba1b5841ae Mon Sep 17 00:00:00 2001 From: Lucas Francisco da Matta Vegi Date: Thu, 10 Aug 2023 14:33:24 -0300 Subject: [PATCH 10/39] "Dynamic atom creation" - static_atom_creation() added --- lib/elixir/pages/anti-patterns/code-anti-patterns.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/elixir/pages/anti-patterns/code-anti-patterns.md b/lib/elixir/pages/anti-patterns/code-anti-patterns.md index ee0ce808bc8..aa78d2f7440 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -355,14 +355,15 @@ When we use the `String.to_atom/1` function to dynamically create an *atom* in a #### Refactoring -To eliminate this anti-pattern, before any string-to-atom conversion takes place, we must ensure that all potential *atoms* to be created from *string* conversions are statically defined in the module where these conversions will occur. Next, you should replace the use of the `String.to_atom/1` function with the `String.to_existing_atom/1` function. This will allow us to have greater control over which *atoms* can be created by the API, as only those *atoms* loaded at the module level will be convertible from *strings*. +To eliminate this anti-pattern, before any string-to-atom conversion takes place, we must ensure that all potential *atoms* to be created from *string* conversions are statically defined in the module where these conversions will occur. We do this via the function `static_atom_creation/0`. Next, you should replace the use of the `String.to_atom/1` function with the `String.to_existing_atom/1` function. This will allow us to have greater control over which *atoms* can be created by the API, as only those *atoms* loaded at the module level will be convertible from *strings*. ```elixir defmodule API do - # statically created atoms - _ = :error - _ = :ok - + defp static_atom_creation() do + _ = :error + _ = :ok + end + def request(foo) do #Some other code... @@ -373,6 +374,7 @@ defmodule API do end def generate_atom(response) when is_map(response) do + static_atom_creation() String.to_existing_atom(response.type) #<= just maps a string to an existing atom! end end From d0ecb3cfdf5996e353cd0cf9c444e4f7a6ddc5b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 16 Aug 2023 08:44:38 +0200 Subject: [PATCH 11/39] Update lib/elixir/pages/anti-patterns/code-anti-patterns.md Co-authored-by: Paulo F. Oliveira --- 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 aa78d2f7440..1d5a63e70e4 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -321,7 +321,7 @@ This anti-pattern was formerly known as [Accessing non-existent Map/Struct field #### Problem -An `Atom` is a basic data type of Elixir whose value is its own name. *Atoms* are often useful to identify resources or express the state or result of an operation. Creatings *atoms* dynamically is not an anti-pattern by itself; however, *atoms* are not collected by the BEAM's garbage collector, so values of this type live in memory while software is executing, during its entire lifetime. Also, BEAM limits the number of *atoms* that can exist in an application (*1_048_576* by default) and each *atom* has a maximum size limited to 255 Unicode code points. For these reasons, creating *atoms* dynamically can be considered an anti-pattern in some situations, since in this way the developer has no control over how many *atoms* will be created during the software execution. This unpredictable scenario can expose the software to unexpected behavior caused by excessive memory usage, or even by reaching the maximum number of *atoms* possible. +An `Atom` is an Elixir basic type whose value is its own name. *Atoms* are often useful to identify resources or express the state, or result, of an operation. Creating *atoms* dynamically is not an anti-pattern by itself; however, *atoms* are not collected by the BEAM's garbage collector, so values of this type live in memory during a software's entire execution lifetime. Also, BEAM limits the number of *atoms* that can exist in an application (*1_048_576*, by default) and each *atom* has a maximum size limited to 255 Unicode code points. For these reasons, creating *atoms* dynamically can be considered an anti-pattern in some situations, since in this way the developer has no control over how many *atoms* will be created during the software execution. This unpredictable scenario can expose the software to unexpected behaviour caused by excessive memory usage, or even by reaching the maximum number of *atoms* possible. #### Example From 1004a49796035418074d42bb8ceb1b76b419c46f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 16 Aug 2023 09:11:30 +0200 Subject: [PATCH 12/39] Update code-anti-patterns.md --- .../pages/anti-patterns/code-anti-patterns.md | 105 +++++++++++------- 1 file changed, 63 insertions(+), 42 deletions(-) diff --git a/lib/elixir/pages/anti-patterns/code-anti-patterns.md b/lib/elixir/pages/anti-patterns/code-anti-patterns.md index 1d5a63e70e4..e031155a985 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -321,76 +321,97 @@ This anti-pattern was formerly known as [Accessing non-existent Map/Struct field #### Problem -An `Atom` is an Elixir basic type whose value is its own name. *Atoms* are often useful to identify resources or express the state, or result, of an operation. Creating *atoms* dynamically is not an anti-pattern by itself; however, *atoms* are not collected by the BEAM's garbage collector, so values of this type live in memory during a software's entire execution lifetime. Also, BEAM limits the number of *atoms* that can exist in an application (*1_048_576*, by default) and each *atom* has a maximum size limited to 255 Unicode code points. For these reasons, creating *atoms* dynamically can be considered an anti-pattern in some situations, since in this way the developer has no control over how many *atoms* will be created during the software execution. This unpredictable scenario can expose the software to unexpected behaviour caused by excessive memory usage, or even by reaching the maximum number of *atoms* possible. +An `Atom` is an Elixir basic type whose value is its own name. Atoms are often useful to identify resources or express the state, or result, of an operation. Creating atoms dynamically is not an anti-pattern by itself; however, atoms are not garbage collected by the Erlang Virtual Machine, so values of this type live in memory during a software's entire execution lifetime. The Erlang VM limits the number of atoms that can exist in an application by default to *1_048_576*, which is more than enough to cover all atoms defined in a program, but attempts to serve as an early limit for applications which are "leaking atoms" through dynamic creation. + +For these reason, creating atoms dynamically can be considered an anti-pattern when the developer has no control over how many atoms will be created during the software execution. This unpredictable scenario can expose the software to unexpected behaviour caused by excessive memory usage, or even by reaching the maximum number of *atoms* possible. #### Example -Picture yourself implementing code that converts *string* values into *atoms*. These *strings* could have been received as responses to API requests, for instance. This dynamic and unpredictable scenario poses a security risk, as these uncontrolled conversions can potentially trigger out-of-memory errors. +Picture yourself implementing code that converts string values into atoms. These strings could have been received from an external system, either as part of a request into our application, or as part of a response to your application. This dynamic and unpredictable scenario poses a security risk, as these uncontrolled conversions can potentially trigger out-of-memory errors. ```elixir -defmodule API do - def request(foo) do - #Some other code... - - case foo do - "bar" -> %{code: 110, type: "error"} - "qux" -> %{code: 112, type: "ok"} - end - end - - def generate_atom(response) when is_map(response) - String.to_atom(response.type) #<= dynamic atom creation!! +defmodule MyRequestHandler do + def parse(%{"status" => status, "message" => message} = _payload) do + %{status: String.to_atom(status), message: message} end end ``` ```elixir -iex> API.request("bar") |> API.generate_atom() -:error -iex> API.request("qux") |> API.generate_atom() -:ok +iex> MyRequestHandler.parse(%{"status" => "ok", "message" => "all good"}) +%{status: :ok, message: "all good"} ``` -When we use the `String.to_atom/1` function to dynamically create an *atom* in an API, it essentially gains potential access to create arbitrary *atoms* in our system, causing us to lose control over adhering to the limits established by the BEAM. This issue could be exploited by someone to create enough *atoms* to shut down a system. +When we use the `String.to_atom/1` function to dynamically create an atom, it essentially gains potential access to create arbitrary atoms in our system, causing us to lose control over adhering to the limits established by the BEAM. This issue could be exploited by someone to create enough atoms to shut down a system. #### Refactoring -To eliminate this anti-pattern, before any string-to-atom conversion takes place, we must ensure that all potential *atoms* to be created from *string* conversions are statically defined in the module where these conversions will occur. We do this via the function `static_atom_creation/0`. Next, you should replace the use of the `String.to_atom/1` function with the `String.to_existing_atom/1` function. This will allow us to have greater control over which *atoms* can be created by the API, as only those *atoms* loaded at the module level will be convertible from *strings*. +To eliminate this anti-pattern, developers must either perform explicit conversions by mapping strings to atoms or replace the use of `String.to_atom/1` with `String.to_existing_atom/1`. An explicit conversion could be done as follows: ```elixir -defmodule API do - defp static_atom_creation() do - _ = :error - _ = :ok +defmodule MyRequestHandler do + def parse(%{"status" => status, "message" => message} = _payload) do + %{status: convert_status(status), message: message} end - def request(foo) do - #Some other code... + defp convert_status("ok"), do: :ok + defp convert_status("error"), do: :error + defp convert_status("redirect"), do: :redirect +end +``` - case foo do - "bar" -> %{code: 110, type: "error"} - "qux" -> %{code: 112, type: "ok"} - end - end - - def generate_atom(response) when is_map(response) do - static_atom_creation() - String.to_existing_atom(response.type) #<= just maps a string to an existing atom! +```elixir +iex> MyRequestHandler.parse(%{"status" => "status_not_seen_anywhere", "message" => "all good"}) +** (FunctionClauseError) no function clause matching in MyRequestHandler.convert_status/1 +``` + +By explicitly listing all supported statuses, you guarantee only a limited number of conversions may happen. Passing an invalid status will lead to a function clause error. + +An alternative is to use `String.to_existing_atom/1`, which will only convert a string to atom if the atom already exists in the system: + +```elixir +defmodule MyRequestHandler do + def parse(%{"status" => status, "message" => message} = _payload) do + %{status: String.to_existing_atom(status), message: message} end end ``` ```elixir -iex> API.request("bar") |> API.generate_atom() -:error -iex> API.request("qux") |> API.generate_atom() -:ok -iex> API.generate_atom(%{code: 113, type: "exploit"}) +iex> MyRequestHandler.parse(%{"status" => "status_not_seen_anywhere", "message" => "all good"}) ** (ArgumentError) errors were found at the given arguments: -* 1st argument: not an already existing atom + + * 1st argument: not an already existing atom +``` + +In such cases, passing an unknown status will raise as long as the status was not defined anywhere as an atom in the system. However, assuming `status` can be either `:ok`, `:error`, or `:redirect`, how can you guarantee those atoms exist? You must ensure those atoms exist somewhere **in the same module** where `String.to_existing_atom/1` is called. For example, if you had this code: + + +```elixir +defmodule MyRequestHandler do + def parse(%{"status" => status, "message" => message} = _payload) do + %{status: String.to_existing_atom(status), message: message} + end + + def handle(%{status: status}) do + case status do + :ok -> ... + :error -> ... + :redirect -> ... + end + end +end +``` + +All valid statuses all defined as atoms within the same module, and that's enough. If you want to be explicit, you could also have a function that lists them: + +```elixir +def valid_statuses do + [:ok, :error, :redirect] +end ``` -Note that in the third use example, when a *string* different from an already existing *atom* is given, Elixir shows an error instead of performing the conversion. This demonstrates that this refactoring creates a more controlled and predictable scenario for the application in terms of memory usage and security. +However, keep in mind using a module attribute or defining the atoms in the module body, outside of a function, are not sufficient, as the module body is only executed during compilation and it is not necessarily part of the compiled module loaded at runtime. ## Namespace trespassing From eaba20b34bd78873b571c2a7357623fb8ba1e7b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 16 Aug 2023 09:12:48 +0200 Subject: [PATCH 13/39] Update lib/elixir/pages/anti-patterns/code-anti-patterns.md --- lib/elixir/pages/anti-patterns/code-anti-patterns.md | 1 - 1 file changed, 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 e031155a985..ddd64e30701 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -386,7 +386,6 @@ iex> MyRequestHandler.parse(%{"status" => "status_not_seen_anywhere", "message" In such cases, passing an unknown status will raise as long as the status was not defined anywhere as an atom in the system. However, assuming `status` can be either `:ok`, `:error`, or `:redirect`, how can you guarantee those atoms exist? You must ensure those atoms exist somewhere **in the same module** where `String.to_existing_atom/1` is called. For example, if you had this code: - ```elixir defmodule MyRequestHandler do def parse(%{"status" => status, "message" => message} = _payload) do From fad00d7763f13cf4e8d16ddbcaf979f488c134a1 Mon Sep 17 00:00:00 2001 From: Lucas Francisco da Matta Vegi Date: Thu, 17 Aug 2023 11:57:26 -0300 Subject: [PATCH 14/39] "Speculative assumptions" added (Anti-patterns documentation) --- .../pages/anti-patterns/code-anti-patterns.md | 60 ++++++++++++++++++- 1 file changed, 59 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 b4d98f658ef..74dc4e5e1f4 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -418,4 +418,62 @@ TODO. ## Speculative assumptions -TODO. +#### Problem + +Overall, Elixir systems are composed of many supervised processes, so the effects of an error will be localized in a single process, not propagating to the entire application. A supervisor will detect the failing process, and restart it at that level. For this type of design to behave well, it's important that problematic code crashes when it fails to fulfill its purpose. However, some code may have undesired behavior making many assumptions we have not really planned for, such as being able to return incorrect values instead of forcing a crash. These speculative assumptions can give a false impression that the code is working correctly. + +#### Example + +The function `get_value/2` tries to extract a value from a specific key of a URL query string. As it is not implemented using pattern matching, `get_value/2` always returns a value, regardless of the format of the URL query string passed as a parameter in the call. Sometimes the returned value will be valid; however, if a URL query string with an unexpected format is used in the call, `get_value/2` will extract incorrect values from it: + +```elixir +defmodule Extract do + def get_value(string, desired_key) do + parts = String.split(string, "&") + Enum.find_value(parts, fn pair -> + key_value = String.split(pair, "=") + Enum.at(key_value, 0) == desired_key && Enum.at(key_value, 1) + end) + end +end +``` + +```elixir +# URL query string according to with the planned format - OK! +iex> Extract.get_value("name=Lucas&university=UFMG&lab=ASERG", "lab") +"ASERG" +iex> Extract.get_value("name=Lucas&university=UFMG&lab=ASERG", "university") +"UFMG" +# Unplanned URL query string format - Unplanned value extraction! +iex> Extract.get_value("name=Lucas&university=institution=UFMG&lab=ASERG", "university") +"institution" # <= why not "institution=UFMG"? or only "UFMG"? +``` + +#### Refactoring + +To remove this anti-pattern, `get_value/2` can be refactored through the use of pattern matching. So, if an unexpected URL query string format is used, the function will crash instead of returning an invalid value. This behaviour, shown below, will allow clients to decide how to handle these errors and will not give a false impression that the code is working correctly when unexpected values are extracted: + +```elixir +defmodule Extract do + def get_value(string, desired_key) do + parts = String.split(string, "&") + Enum.find_value(parts, fn pair -> + [key, value] = String.split(pair, "=") # <= pattern matching + key == desired_key && value + end) + end +end +```` + +```elixir +# URL query string according to with the planned format - OK! +iex> Extract.get_value("name=Lucas&university=UFMG&lab=ASERG", "name") +"Lucas" +# Unplanned URL query string format - Crash explaining the problem to the client! +iex> Extract.get_value("name=Lucas&university=institution=UFMG&lab=ASERG", "university") +** (MatchError) no match of right hand side value: ["university", "institution", "UFMG"] + extract.ex:7: anonymous fn/2 in Extract.get_value/2 # <= left hand: [key, value] pair +iex> Extract.get_value("name=Lucas&university&lab=ASERG", "university") +** (MatchError) no match of right hand side value: ["university"] + extract.ex:7: anonymous fn/2 in Extract.get_value/2 # <= left hand: [key, value] pair +``` From 8888097c8b8b256eb4b163a07835f7bf6c6cc90d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Fri, 18 Aug 2023 11:49:57 +0200 Subject: [PATCH 15/39] Update lib/elixir/pages/anti-patterns/code-anti-patterns.md Co-authored-by: Andrea Leopardi --- 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 74dc4e5e1f4..e4333713b76 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -420,7 +420,7 @@ TODO. #### Problem -Overall, Elixir systems are composed of many supervised processes, so the effects of an error will be localized in a single process, not propagating to the entire application. A supervisor will detect the failing process, and restart it at that level. For this type of design to behave well, it's important that problematic code crashes when it fails to fulfill its purpose. However, some code may have undesired behavior making many assumptions we have not really planned for, such as being able to return incorrect values instead of forcing a crash. These speculative assumptions can give a false impression that the code is working correctly. +Overall, Elixir systems are composed of many supervised processes, so the effects of an error will be localized in a single process, not propagating to the entire application. A supervisor will detect the failing process, and possibly restart it. For this type of design to work well, it's important that problematic code crashes when it fails to fulfill its purpose. However, some code may have undesired behavior making many assumptions we have not really planned for, such as being able to return incorrect values instead of forcing a crash. These speculative assumptions can give a false impression that the code is working correctly. #### Example From e4b24f9adf20c89274caeac125a0e748fbcaa674 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Fri, 18 Aug 2023 11:59:17 +0200 Subject: [PATCH 16/39] Apply suggestions from code review --- lib/elixir/pages/anti-patterns/code-anti-patterns.md | 8 ++++++-- 1 file changed, 6 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 e4333713b76..f04b761865c 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -420,16 +420,17 @@ TODO. #### Problem -Overall, Elixir systems are composed of many supervised processes, so the effects of an error will be localized in a single process, not propagating to the entire application. A supervisor will detect the failing process, and possibly restart it. For this type of design to work well, it's important that problematic code crashes when it fails to fulfill its purpose. However, some code may have undesired behavior making many assumptions we have not really planned for, such as being able to return incorrect values instead of forcing a crash. These speculative assumptions can give a false impression that the code is working correctly. +Overall, Elixir systems are composed of many supervised processes, so the effects of an error are localized to a single process, not propagating to the entire application. A supervisor will detect the failing process, report it, and possibly restart it. This means Elixir developers do not need to program defensively, making assumptions we have not really planned for, such as being able to return incorrect values instead of forcing a crash. These speculative assumptions can give a false impression that the code is working correctly. #### Example -The function `get_value/2` tries to extract a value from a specific key of a URL query string. As it is not implemented using pattern matching, `get_value/2` always returns a value, regardless of the format of the URL query string passed as a parameter in the call. Sometimes the returned value will be valid; however, if a URL query string with an unexpected format is used in the call, `get_value/2` will extract incorrect values from it: +The function `get_value/2` tries to extract a value from a specific key of a URL query string. As it is not implemented using pattern matching, `get_value/2` always returns a value, regardless of the format of the URL query string passed as a parameter in the call. Sometimes the returned value will be valid. However, if a URL query string with an unexpected format is used in the call, `get_value/2` will extract incorrect values from it: ```elixir defmodule Extract do def get_value(string, desired_key) do parts = String.split(string, "&") + Enum.find_value(parts, fn pair -> key_value = String.split(pair, "=") Enum.at(key_value, 0) == desired_key && Enum.at(key_value, 1) @@ -457,6 +458,7 @@ To remove this anti-pattern, `get_value/2` can be refactored through the use of defmodule Extract do def get_value(string, desired_key) do parts = String.split(string, "&") + Enum.find_value(parts, fn pair -> [key, value] = String.split(pair, "=") # <= pattern matching key == desired_key && value @@ -477,3 +479,5 @@ iex> Extract.get_value("name=Lucas&university&lab=ASERG", "university") ** (MatchError) no match of right hand side value: ["university"] extract.ex:7: anonymous fn/2 in Extract.get_value/2 # <= left hand: [key, value] pair ``` + +The goal is to promote an assertive style of programming where you handle the known cases. Once an unexpected scenario arises in production, you can decide to address it accordingly, based on the needs of the code using actual examples, or conclude the scenario is indeed expected and the exception is the desired choice. From 3f165fa2f005240db3ac48f853ed032287cd2cd5 Mon Sep 17 00:00:00 2001 From: Lucas Francisco da Matta Vegi Date: Fri, 18 Aug 2023 13:43:09 -0300 Subject: [PATCH 17/39] "Long parameter list" added (Anti-patterns documentation) --- .../pages/anti-patterns/code-anti-patterns.md | 39 +++++++++++++++++-- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/lib/elixir/pages/anti-patterns/code-anti-patterns.md b/lib/elixir/pages/anti-patterns/code-anti-patterns.md index f04b761865c..8e11161004e 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -47,7 +47,38 @@ Elixir makes a clear distinction between **documentation** and code comments. Th ## Long parameter list -TODO. +#### Problem + +In a functional language like Elixir, functions tend to be pure and therefore do not access or manipulate values outside their scopes. Since the input values of these functions are just their parameters, it is natural to expect that functions with a long list of parameters be created to keep them pure. However, when this list has **more than three or four parameters**, the function's interface becomes confusing and prone to errors during use. + +#### Example + +In the following example, the `loan/6` function has an unnecessarily long list of parameters, causing its interface to be confusing and potentially leading developers to introduce errors during calls to this function. + +```elixir +defmodule Library do + # Too many parameters that can be grouped! + def loan(user_name, email, password, user_alias, book_title, book_ed) do + ... + end +end +``` + +#### Refactoring + +To eliminate this anti-pattern, related parameters can be grouped using `Maps`, `Structs`, or even `Tuples`. This effectively reduces the number of function parameters, simplifying its interface. In the case of the `loan/6`, its parameters were grouped into two different `Maps`, thereby reducing its arity to `loan/2`: + +```elixir +defmodule Library do + def loan(%{name: name, email: email, password: password, alias: alias} = user, %{title: title, ed: ed} = book) do + ... + end +end +``` + +#### Additional remarks + +Although this refactoring has grouped parameters using `Maps`, we can find, in different functions, identical sets of parameters that could be grouped. These duplicated sets are known in the literature as *Data Clumps*. In that case, is better to create a `Struct` to group these parameters and reuse this to refactor all functions where these sets of parameters occur. ## Complex branching @@ -220,7 +251,7 @@ The function `plot/1` tries to draw a graphic to represent the position of a poi ```elixir defmodule Graphics do def plot(point) do - #Some other code... + # Some other code... # Dynamic access to use point values {point[:x], point[:y], point[:z]} @@ -248,7 +279,7 @@ To remove this anti-pattern, whenever a map has keys of `Atom` type, replace the ```elixir defmodule Graphics do def plot(point) do - #Some other code... + # Some other code... # Strict access to use point values {point.x, point.y, point.z} @@ -273,7 +304,7 @@ As shown below, another alternative to refactor this anti-pattern is to use patt ```elixir defmodule Graphics do def plot(%{x: x, y: y, z: z}) do - #Some other code... + # Some other code... # Strict access to use point values {x, y, z} From b889bc3b48012668f11dca1e5857f9c1450654ae Mon Sep 17 00:00:00 2001 From: Lucas Francisco da Matta Vegi Date: Fri, 18 Aug 2023 13:46:57 -0300 Subject: [PATCH 18/39] Fixing typo ("Long Parameter List" documentation) --- 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 8e11161004e..5c4bc5be99d 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -71,7 +71,7 @@ To eliminate this anti-pattern, related parameters can be grouped using `Maps`, ```elixir defmodule Library do def loan(%{name: name, email: email, password: password, alias: alias} = user, %{title: title, ed: ed} = book) do - ... + ... end end ``` From cfd552ca8d27451d2666984ff824483108ad5515 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Sun, 20 Aug 2023 20:55:44 +0200 Subject: [PATCH 19/39] Update code-anti-patterns.md --- lib/elixir/pages/anti-patterns/code-anti-patterns.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/elixir/pages/anti-patterns/code-anti-patterns.md b/lib/elixir/pages/anti-patterns/code-anti-patterns.md index 5c4bc5be99d..2536102f87f 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -49,11 +49,11 @@ Elixir makes a clear distinction between **documentation** and code comments. Th #### Problem -In a functional language like Elixir, functions tend to be pure and therefore do not access or manipulate values outside their scopes. Since the input values of these functions are just their parameters, it is natural to expect that functions with a long list of parameters be created to keep them pure. However, when this list has **more than three or four parameters**, the function's interface becomes confusing and prone to errors during use. +In a functional language like Elixir, functions tend to explicitly receive all inputs and return all relevant outputs, instead of relying on mutations or side-effects. As functions grow in complexity, the amount of arguments (parameters) they need to work with may grow, to a point the function's interface becomes confusing and prone to errors during use. #### Example -In the following example, the `loan/6` function has an unnecessarily long list of parameters, causing its interface to be confusing and potentially leading developers to introduce errors during calls to this function. +In the following example, the `loan/6` functions takes too many arguments, causing its interface to be confusing and potentially leading developers to introduce errors during calls to this function. ```elixir defmodule Library do @@ -66,7 +66,7 @@ end #### Refactoring -To eliminate this anti-pattern, related parameters can be grouped using `Maps`, `Structs`, or even `Tuples`. This effectively reduces the number of function parameters, simplifying its interface. In the case of the `loan/6`, its parameters were grouped into two different `Maps`, thereby reducing its arity to `loan/2`: +To address this anti-pattern, related arguments can be grouped using maps, structs, or even tuples. This effectively reduces the number of arguments, simplifying the function's interface. In the case of `loan/6`, its arguments were grouped into two different maps, thereby reducing its arity to `loan/2`: ```elixir defmodule Library do @@ -76,9 +76,9 @@ defmodule Library do end ``` -#### Additional remarks +In some cases, the function with too many arguments may be a private function, which gives us more flexibility over how to separate the function arguments. One possible suggestion for such scenarios is to split the arguments in two maps (or tuples): one map keeps the data that may change, and the other keeps the data that won't change (read-only). This gives us a mechanical option to refactor the code. -Although this refactoring has grouped parameters using `Maps`, we can find, in different functions, identical sets of parameters that could be grouped. These duplicated sets are known in the literature as *Data Clumps*. In that case, is better to create a `Struct` to group these parameters and reuse this to refactor all functions where these sets of parameters occur. +Other times, a function may legitimately take half a dozen or more completely unrelated arguments. This may suggest the function is trying to do too much and would be better broken into multiple functions, each responsible for a smaller piece of the overall responsibility. ## Complex branching From 098d665fa356d6a3f6655bf736d1ad41029dc58b Mon Sep 17 00:00:00 2001 From: Lucas Francisco da Matta Vegi Date: Fri, 1 Sep 2023 10:47:49 -0300 Subject: [PATCH 20/39] "Alternative return types" added (Anti-patterns documentation) --- .../anti-patterns/design-anti-patterns.md | 57 ++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/lib/elixir/pages/anti-patterns/design-anti-patterns.md b/lib/elixir/pages/anti-patterns/design-anti-patterns.md index 937d153c2a6..e23ac23482d 100644 --- a/lib/elixir/pages/anti-patterns/design-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/design-anti-patterns.md @@ -69,7 +69,62 @@ The following arguments were given to MyLibrary.foo/1: ## Alternative return types -TODO +#### Problem + +This anti-pattern refers to functions that receive options (for example, *keyword list*) parameters that drastically change their return type. Because options are optional and sometimes set dynamically, if they change the return type it may be hard to understand what the function actually returns. + +#### Example + +An example of this anti-pattern, as shown below, is when a library (for example, `AlternativeInteger`) has a multi-clause function `parse/2` with many alternative return types. Depending on the options received as a parameter, the function will have a different return type. + +```elixir +defmodule AlternativeInteger do + def parse(string, opts) when is_list(opts) do + case opts[:discard_rest] do + true -> String.to_integer(string) + _ -> Integer.parse(string) + end + end + + def parse(string, opts \\ :default) do + case opts do + :default -> Integer.parse(string) + end + end +end +``` + +```elixir +iex> AlternativeInteger.parse("13") +{13, ""} +iex> AlternativeInteger.parse("13", discard_rest: true) +13 +iex> AlternativeInteger.parse("13", discard_rest: false) +{13, ""} +``` + +#### Refactoring + +To refactor this anti-pattern, as shown next, it's better to add in the library a specific function for each return type (for example, `parse_no_rest/1`), no longer delegating this to an options parameter. + +```elixir +defmodule AlternativeInteger do + def parse_no_rest(string) do + String.to_integer(string) + end + + def parse(string) do + Integer.parse(string) + end +end +``` + +```elixir +iex> AlternativeInteger.parse("13") +{13, ""} +iex> AlternativeInteger.parse_no_rest("13") +13 +``` ## Unrelated multi-clause function From 8efb1891ec0e0359f98d311a68af37117d2b7d08 Mon Sep 17 00:00:00 2001 From: Andrea Leopardi Date: Fri, 1 Sep 2023 17:58:40 +0200 Subject: [PATCH 21/39] Apply suggestions from code review --- lib/elixir/pages/anti-patterns/design-anti-patterns.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/elixir/pages/anti-patterns/design-anti-patterns.md b/lib/elixir/pages/anti-patterns/design-anti-patterns.md index e23ac23482d..d3097f7ce4c 100644 --- a/lib/elixir/pages/anti-patterns/design-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/design-anti-patterns.md @@ -105,7 +105,7 @@ iex> AlternativeInteger.parse("13", discard_rest: false) #### Refactoring -To refactor this anti-pattern, as shown next, it's better to add in the library a specific function for each return type (for example, `parse_no_rest/1`), no longer delegating this to an options parameter. +To refactor this anti-pattern, as shown next, add a specific function for each return type (for example, `parse_no_rest/1`), no longer delegating this to options passed as arguments. ```elixir defmodule AlternativeInteger do From d64185ef6aec05033195e43c25b8d37385f5a54a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Mon, 11 Sep 2023 10:25:05 +0200 Subject: [PATCH 22/39] Apply suggestions from code review Co-authored-by: Andrea Leopardi --- .../pages/anti-patterns/design-anti-patterns.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/elixir/pages/anti-patterns/design-anti-patterns.md b/lib/elixir/pages/anti-patterns/design-anti-patterns.md index d3097f7ce4c..cd4b2715149 100644 --- a/lib/elixir/pages/anti-patterns/design-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/design-anti-patterns.md @@ -75,14 +75,15 @@ This anti-pattern refers to functions that receive options (for example, *keywor #### Example -An example of this anti-pattern, as shown below, is when a library (for example, `AlternativeInteger`) has a multi-clause function `parse/2` with many alternative return types. Depending on the options received as a parameter, the function will have a different return type. +An example of this anti-pattern, as shown below, is when a function has many alternative return types, depending on the options received as a parameter. ```elixir defmodule AlternativeInteger do - def parse(string, opts) when is_list(opts) do - case opts[:discard_rest] do - true -> String.to_integer(string) - _ -> Integer.parse(string) + def parse(string, options \\ []) when is_list(options) do + if Keyword.get(options, :discard_rest, false) do + String.to_integer(string) + else + Integer.parse(string) end end From 60a1282476bdd60f5e81a399551b3d63c2ea6a31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Mon, 11 Sep 2023 10:25:34 +0200 Subject: [PATCH 23/39] Apply suggestions from code review Co-authored-by: Andrea Leopardi --- lib/elixir/pages/anti-patterns/design-anti-patterns.md | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/lib/elixir/pages/anti-patterns/design-anti-patterns.md b/lib/elixir/pages/anti-patterns/design-anti-patterns.md index cd4b2715149..78721e82cf9 100644 --- a/lib/elixir/pages/anti-patterns/design-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/design-anti-patterns.md @@ -79,6 +79,7 @@ An example of this anti-pattern, as shown below, is when a function has many alt ```elixir defmodule AlternativeInteger do + @spec parse(String.t(), keyword()) :: integer() | {integer(), String.t()} | :error def parse(string, options \\ []) when is_list(options) do if Keyword.get(options, :discard_rest, false) do String.to_integer(string) @@ -86,12 +87,6 @@ defmodule AlternativeInteger do Integer.parse(string) end end - - def parse(string, opts \\ :default) do - case opts do - :default -> Integer.parse(string) - end - end end ``` From 4e2be8c241c0a94080ff879b53314b3bb99991ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Mon, 11 Sep 2023 10:27:53 +0200 Subject: [PATCH 24/39] Apply suggestions from code review Co-authored-by: Eksperimental --- .../pages/anti-patterns/design-anti-patterns.md | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/lib/elixir/pages/anti-patterns/design-anti-patterns.md b/lib/elixir/pages/anti-patterns/design-anti-patterns.md index 78721e82cf9..feccb70cc71 100644 --- a/lib/elixir/pages/anti-patterns/design-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/design-anti-patterns.md @@ -84,7 +84,10 @@ defmodule AlternativeInteger do if Keyword.get(options, :discard_rest, false) do String.to_integer(string) else - Integer.parse(string) + case String.parse(string) do + {int, _rest} -> int + :error -> :error + end end end end @@ -105,13 +108,15 @@ To refactor this anti-pattern, as shown next, add a specific function for each r ```elixir defmodule AlternativeInteger do - def parse_no_rest(string) do - String.to_integer(string) - end - + @spec parse(String.t()) :: {integer(), rest :: String.t()} | :error def parse(string) do Integer.parse(string) end + + @spec parse_discard_rest(String.t()) :: integer() + def parse_discard_rest(string) do + String.to_integer(string) + end end ``` From 159fe842fe95ae3683eb3f89d168c3ad8ab5fe7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Mon, 11 Sep 2023 10:28:41 +0200 Subject: [PATCH 25/39] Update lib/elixir/pages/anti-patterns/design-anti-patterns.md --- lib/elixir/pages/anti-patterns/design-anti-patterns.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/elixir/pages/anti-patterns/design-anti-patterns.md b/lib/elixir/pages/anti-patterns/design-anti-patterns.md index feccb70cc71..e598c49d8f9 100644 --- a/lib/elixir/pages/anti-patterns/design-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/design-anti-patterns.md @@ -82,9 +82,9 @@ defmodule AlternativeInteger do @spec parse(String.t(), keyword()) :: integer() | {integer(), String.t()} | :error def parse(string, options \\ []) when is_list(options) do if Keyword.get(options, :discard_rest, false) do - String.to_integer(string) + Integer.parse(string) else - case String.parse(string) do + case Integer.parse(string) do {int, _rest} -> int :error -> :error end From 304fde0ed0bf6e671146e993b810d1855b73fcfb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Mon, 11 Sep 2023 10:30:20 +0200 Subject: [PATCH 26/39] Apply suggestions from code review --- .../pages/anti-patterns/design-anti-patterns.md | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/elixir/pages/anti-patterns/design-anti-patterns.md b/lib/elixir/pages/anti-patterns/design-anti-patterns.md index e598c49d8f9..3dea9a9e54c 100644 --- a/lib/elixir/pages/anti-patterns/design-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/design-anti-patterns.md @@ -104,18 +104,21 @@ iex> AlternativeInteger.parse("13", discard_rest: false) #### Refactoring -To refactor this anti-pattern, as shown next, add a specific function for each return type (for example, `parse_no_rest/1`), no longer delegating this to options passed as arguments. +To refactor this anti-pattern, as shown next, add a specific function for each return type (for example, `parse_discard_rest/1`), no longer delegating this to options passed as arguments. ```elixir defmodule AlternativeInteger do - @spec parse(String.t()) :: {integer(), rest :: String.t()} | :error + @spec parse(String.t()) :: {integer(), String.t()} | :error def parse(string) do Integer.parse(string) end - @spec parse_discard_rest(String.t()) :: integer() + @spec parse_discard_rest(String.t()) :: integer() | :error def parse_discard_rest(string) do - String.to_integer(string) + case Integer.parse(string) do + {int, _rest} -> int + :error -> :error + end end end ``` @@ -123,7 +126,7 @@ end ```elixir iex> AlternativeInteger.parse("13") {13, ""} -iex> AlternativeInteger.parse_no_rest("13") +iex> AlternativeInteger.parse_discard_rest("13") 13 ``` From ca5ac56e7c349be2e4b7ec7d7b8c8000590ec456 Mon Sep 17 00:00:00 2001 From: Lucas Francisco da Matta Vegi Date: Mon, 11 Sep 2023 11:30:27 -0300 Subject: [PATCH 27/39] "Using exceptions for control-flow" added (Anti-patterns documentation) --- .../anti-patterns/design-anti-patterns.md | 94 ++++++++++++++++++- 1 file changed, 93 insertions(+), 1 deletion(-) diff --git a/lib/elixir/pages/anti-patterns/design-anti-patterns.md b/lib/elixir/pages/anti-patterns/design-anti-patterns.md index 3dea9a9e54c..d04b4f4141d 100644 --- a/lib/elixir/pages/anti-patterns/design-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/design-anti-patterns.md @@ -144,7 +144,99 @@ TODO ## Using exceptions for control-flow -TODO +#### Problem + +This anti-pattern refers to code that forces developers to handle exceptions for control-flow. Exception handling itself does not represent an anti-pattern, but this should not be the only alternative available to developers to handle an error in client code. When developers have no freedom to decide if an error is exceptional or not, this is considered an anti-pattern. + +#### Example + +An example of this anti-pattern, as shown below, is when a library (for example, `MyModule`) forces its clients to use `try .. rescue` statements to capture and evaluate errors. This library does not allow developers to decide if an error is exceptional or not in their applications. + +```elixir +defmodule MyModule do + def janky_function(value) do + if is_integer(value) do + #... + "Result..." + else + raise RuntimeError, message: "invalid argument. Is not integer!" + end + end +end +``` + +```elixir +defmodule Client do + # Client forced to use exceptions for control-flow. + def foo(arg) do + try do + value = MyModule.janky_function(arg) + "All good! #{value}." + rescue + e in RuntimeError -> + reason = e.message + "Uh oh! #{reason}." + end + end +end +``` + +```elixir +iex> Client.foo(1) +"All good! Result...." +iex> Client.foo("lucas") +"Uh oh! invalid argument. Is not integer!." +``` + +#### Refactoring + +Library authors should guarantee that clients are not required to use exceptions for control-flow in their applications. As shown below, this can be done by refactoring the library `MyModule`, providing two versions of the function that forces clients to use exceptions for control-flow: + +1) A version with the raised exceptions should have the same name as the smelly one, but with a trailing `!` (`janky_function!/1`); + +2) Another version, without raised exceptions, should have a name identical to the original version (`janky_function/1`) and should return the result wrapped in a tuple. + +```elixir +defmodule MyModule do + def janky_function(value) do + if is_integer(value) do + #... + {:ok, "Result..."} + else + {:error, "invalid argument. Is not integer!"} + end + end + + def janky_function!(value) do + case janky_function(value) do + {:ok, result} -> result + {:error, message} -> raise RuntimeError, message: message + end + end +end +``` + +This refactoring gives clients more freedom to decide how to proceed in the event of errors, defining what is exceptional or not in different situations. As shown next, when an error is not exceptional, clients can use specific control-flow structures, such as the `case` statement along with pattern matching. + +```elixir +defmodule Client do + # Clients now can also choose to use control-flow structures + # for control-flow when an error is not exceptional. + def foo(arg) do + case MyModule.janky_function(arg) do + {:ok, value} -> "All good! #{value}." + {:error, reason} -> "Uh oh! #{reason}." + end + end +end +``` + +```elixir +iex> Client.foo(1) +"All good! Result...." +iex> Client.foo("lucas") +"Uh oh! invalid argument. Is not integer!." +``` ## Using application configuration for libraries From 6b32731aa1e60e20f8012f88290a5963a1807389 Mon Sep 17 00:00:00 2001 From: Andrea Leopardi Date: Mon, 11 Sep 2023 17:12:17 +0200 Subject: [PATCH 28/39] Apply suggestions from code review --- .../anti-patterns/design-anti-patterns.md | 59 +++++++++---------- 1 file changed, 28 insertions(+), 31 deletions(-) diff --git a/lib/elixir/pages/anti-patterns/design-anti-patterns.md b/lib/elixir/pages/anti-patterns/design-anti-patterns.md index d04b4f4141d..6e9aeb8a024 100644 --- a/lib/elixir/pages/anti-patterns/design-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/design-anti-patterns.md @@ -146,20 +146,19 @@ TODO #### Problem -This anti-pattern refers to code that forces developers to handle exceptions for control-flow. Exception handling itself does not represent an anti-pattern, but this should not be the only alternative available to developers to handle an error in client code. When developers have no freedom to decide if an error is exceptional or not, this is considered an anti-pattern. +This anti-pattern refers to code that forces developers to handle exceptions for control flow. Exception handling itself does not represent an anti-pattern, but this should not be the only alternative available to developers to handle an error in client code. When developers have no freedom to decide if an error is exceptional or not, this is considered an anti-pattern. #### Example -An example of this anti-pattern, as shown below, is when a library (for example, `MyModule`) forces its clients to use `try .. rescue` statements to capture and evaluate errors. This library does not allow developers to decide if an error is exceptional or not in their applications. +An example of this anti-pattern, as shown below, is when a library forces its users to use `try/1` statements to rescue raised exceptions and handle different cases. Such a library library does not allow developers to decide if an error is exceptional or not in their applications. ```elixir defmodule MyModule do def janky_function(value) do if is_integer(value) do - #... - "Result..." + "It's an integer" else - raise RuntimeError, message: "invalid argument. Is not integer!" + raise "expected integer, got: #{inspect(value)}" end end end @@ -167,62 +166,60 @@ end ```elixir defmodule Client do - # Client forced to use exceptions for control-flow. - def foo(arg) do + # Client forced to use exceptions for control flow. + def print_janky_function(arg) do try do - value = MyModule.janky_function(arg) - "All good! #{value}." + result = MyModule.janky_function(arg) + "All good! #{result}." rescue - e in RuntimeError -> - reason = e.message - "Uh oh! #{reason}." + exception in RuntimeError -> + "Uh oh! #{exception.message}." end end end ``` ```elixir -iex> Client.foo(1) -"All good! Result...." -iex> Client.foo("lucas") -"Uh oh! invalid argument. Is not integer!." +iex> Client.print_janky_function(1) +"All good! It's an integer." +iex> Client.foo("Lucas") +"Uh oh! expected integer, got: \"Lucas\"" ``` #### Refactoring -Library authors should guarantee that clients are not required to use exceptions for control-flow in their applications. As shown below, this can be done by refactoring the library `MyModule`, providing two versions of the function that forces clients to use exceptions for control-flow: +Library authors should guarantee that users are not required to use exceptions for control flow in their applications. As shown below, this can be done by refactoring `MyModule`, providing two versions of the function that forces clients to use exceptions for control flow: -1) A version with the raised exceptions should have the same name as the smelly one, but with a trailing `!` (`janky_function!/1`); + 1. A version that raises exceptions should have the same name as the "janky" one, but with a trailing `!` (`janky_function!/1`); -2) Another version, without raised exceptions, should have a name identical to the original version (`janky_function/1`) and should return the result wrapped in a tuple. + 2. Another version, without raised exceptions, should have a name identical to the original version (`janky_function/1`) and should return the result wrapped in a tuple. ```elixir defmodule MyModule do def janky_function(value) do if is_integer(value) do - #... - {:ok, "Result..."} + {:ok, "It's an integer"} else - {:error, "invalid argument. Is not integer!"} + {:error, "expected an integer, got: #{inspect(value)}"} end end def janky_function!(value) do case janky_function(value) do {:ok, result} -> result - {:error, message} -> raise RuntimeError, message: message + {:error, message} -> raise(message) end end end ``` -This refactoring gives clients more freedom to decide how to proceed in the event of errors, defining what is exceptional or not in different situations. As shown next, when an error is not exceptional, clients can use specific control-flow structures, such as the `case` statement along with pattern matching. +This refactoring gives users more freedom to decide how to proceed in the event of errors, defining what is exceptional or not in different situations. As shown next, when an error is not exceptional, clients can use specific control-flow structures, such as the `case/2` statement. ```elixir defmodule Client do - # Clients now can also choose to use control-flow structures - # for control-flow when an error is not exceptional. - def foo(arg) do + # Users now can also choose to use control-flow structures + # for control flow when an error is not exceptional. + def print_janky_function(arg) do case MyModule.janky_function(arg) do {:ok, value} -> "All good! #{value}." {:error, reason} -> "Uh oh! #{reason}." @@ -232,10 +229,10 @@ end ``` ```elixir -iex> Client.foo(1) -"All good! Result...." -iex> Client.foo("lucas") -"Uh oh! invalid argument. Is not integer!." +iex> Client.print_janky_function(1) +"All good! It's an integer." +iex> Client.print_janky_function("Lucas") +"Uh oh! expected an integer, got: \"Lucas\"" ``` ## Using application configuration for libraries From f6836182b6265709cdd3935d361614fde5314fae Mon Sep 17 00:00:00 2001 From: Andrea Leopardi Date: Mon, 11 Sep 2023 22:41:33 +0200 Subject: [PATCH 29/39] Update lib/elixir/pages/anti-patterns/design-anti-patterns.md --- lib/elixir/pages/anti-patterns/design-anti-patterns.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/elixir/pages/anti-patterns/design-anti-patterns.md b/lib/elixir/pages/anti-patterns/design-anti-patterns.md index 6e9aeb8a024..1642e34d1c4 100644 --- a/lib/elixir/pages/anti-patterns/design-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/design-anti-patterns.md @@ -188,7 +188,7 @@ iex> Client.foo("Lucas") #### Refactoring -Library authors should guarantee that users are not required to use exceptions for control flow in their applications. As shown below, this can be done by refactoring `MyModule`, providing two versions of the function that forces clients to use exceptions for control flow: +Library authors should guarantee that users are not required to use exceptions for control flow in their applications. As shown below, this can be done by refactoring `MyModule`, providing two versions of the function that forces clients to use exceptions for control flow: 1. A version that raises exceptions should have the same name as the "janky" one, but with a trailing `!` (`janky_function!/1`); From 6bf7f086cc70248054529d142799dc9f0379e43e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 13 Sep 2023 10:37:47 +0200 Subject: [PATCH 30/39] Update design-anti-patterns.md --- .../anti-patterns/design-anti-patterns.md | 83 ++++++------------- 1 file changed, 27 insertions(+), 56 deletions(-) diff --git a/lib/elixir/pages/anti-patterns/design-anti-patterns.md b/lib/elixir/pages/anti-patterns/design-anti-patterns.md index 1642e34d1c4..fe29f9c8f8e 100644 --- a/lib/elixir/pages/anti-patterns/design-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/design-anti-patterns.md @@ -146,94 +146,65 @@ TODO #### Problem -This anti-pattern refers to code that forces developers to handle exceptions for control flow. Exception handling itself does not represent an anti-pattern, but this should not be the only alternative available to developers to handle an error in client code. When developers have no freedom to decide if an error is exceptional or not, this is considered an anti-pattern. +This anti-pattern refers to code that uses exceptions for control flow. Exception handling itself does not represent an anti-pattern, but developers must prefer to use `case` and pattern matching to change the flow of their code, instead of `try/rescue`. In turn, library authors should provide developers with APIs to handle errors without relying on exception handling. When developers have no freedom to decide if an error is exceptional or not, this is considered an anti-pattern. #### Example -An example of this anti-pattern, as shown below, is when a library forces its users to use `try/1` statements to rescue raised exceptions and handle different cases. Such a library library does not allow developers to decide if an error is exceptional or not in their applications. +An example of this anti-pattern, as shown below, is using `try/rescue` to deal with file operations: ```elixir defmodule MyModule do - def janky_function(value) do - if is_integer(value) do - "It's an integer" - else - raise "expected integer, got: #{inspect(value)}" - end - end -end -``` - -```elixir -defmodule Client do - # Client forced to use exceptions for control flow. - def print_janky_function(arg) do + def print_file(file) do try do - result = MyModule.janky_function(arg) - "All good! #{result}." + IO.puts(File.read!(file)) rescue - exception in RuntimeError -> - "Uh oh! #{exception.message}." + e -> IO.puts(:stderr, Exception.message(e)) end end end ``` ```elixir -iex> Client.print_janky_function(1) -"All good! It's an integer." -iex> Client.foo("Lucas") -"Uh oh! expected integer, got: \"Lucas\"" +iex> MyModule.print_file("valid_file") +This is a valid file! +:ok +iex> MyModule.print_file("invalid_file") +could not read file "invalid_file": no such file or directory +:ok ``` #### Refactoring -Library authors should guarantee that users are not required to use exceptions for control flow in their applications. As shown below, this can be done by refactoring `MyModule`, providing two versions of the function that forces clients to use exceptions for control flow: - - 1. A version that raises exceptions should have the same name as the "janky" one, but with a trailing `!` (`janky_function!/1`); - - 2. Another version, without raised exceptions, should have a name identical to the original version (`janky_function/1`) and should return the result wrapped in a tuple. +To refactor this anti-pattern, as shown next, use `File.read/1`, which returns tuples instead of raising when a file cannot be read: ```elixir defmodule MyModule do - def janky_function(value) do - if is_integer(value) do - {:ok, "It's an integer"} - else - {:error, "expected an integer, got: #{inspect(value)}"} - end - end - - def janky_function!(value) do - case janky_function(value) do - {:ok, result} -> result - {:error, message} -> raise(message) + def print_file(file) do + case File.read(file) do + {:ok, binary} -> IO.puts(binary) + {:error, reason} -> IO.puts(:stderr, "could not read file #{file}: #{reason}") end end end ``` -This refactoring gives users more freedom to decide how to proceed in the event of errors, defining what is exceptional or not in different situations. As shown next, when an error is not exceptional, clients can use specific control-flow structures, such as the `case/2` statement. +This is only possible because the `File` module provides APIs for reading files with tuples as results (`File.read/1`), as well as a version that raises an exception (`File.read!/1`). The bang (exclamation point) is effectively part of [Elixir's naming conventions](naming-conventions.html#trailing-bang-foo). + +Library authors are encouraged to follow the same practices. In practice, the bang variant is implemented on top of the non-raising version of the code. For example, `File.read/1` is implemented as: ```elixir -defmodule Client do - # Users now can also choose to use control-flow structures - # for control flow when an error is not exceptional. - def print_janky_function(arg) do - case MyModule.janky_function(arg) do - {:ok, value} -> "All good! #{value}." - {:error, reason} -> "Uh oh! #{reason}." - end +def read!(path) do + case read(path) do + {:ok, binary} -> + binary + + {:error, reason} -> + raise File.Error, reason: reason, action: "read file", path: IO.chardata_to_string(path) end end ``` -```elixir -iex> Client.print_janky_function(1) -"All good! It's an integer." -iex> Client.print_janky_function("Lucas") -"Uh oh! expected an integer, got: \"Lucas\"" -``` +A common practice followed by the community is to make the non-raising version to return `{:ok, result}` or `{:error, Exception.t}`. For example, an HTTP client may return `{:ok, %HTTP.Response{}}` on success cases and a `{:error, %HTTP.Error{}}` for failures, where `HTTP.Error` is [implemented as an exception](`Kernel.defexception/1`). This makes it convenient for anyone to raise an exception by simply calling `Kernel.raise/1`. ## Using application configuration for libraries From 580b345ef9a0897efff4ed128b988198adb6b97d Mon Sep 17 00:00:00 2001 From: Lucas Francisco da Matta Vegi Date: Thu, 14 Sep 2023 08:51:25 -0300 Subject: [PATCH 31/39] "Unrelated multi-clause function" added (Anti-patterns documentation) --- .../anti-patterns/design-anti-patterns.md | 79 ++++++++++++++++++- 1 file changed, 78 insertions(+), 1 deletion(-) diff --git a/lib/elixir/pages/anti-patterns/design-anti-patterns.md b/lib/elixir/pages/anti-patterns/design-anti-patterns.md index fe29f9c8f8e..d771b596434 100644 --- a/lib/elixir/pages/anti-patterns/design-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/design-anti-patterns.md @@ -132,7 +132,84 @@ iex> AlternativeInteger.parse_discard_rest("13") ## Unrelated multi-clause function -TODO +#### Problem + +Using multi-clause functions in Elixir, to group functions of the same name, is not an anti-pattern in itself. However, due to the great flexibility provided by this programming feature, some developers may abuse the number of guard clauses and pattern matches to group *unrelated* functionality. + +#### Example + +A recurrent example of abusive use of multi-clause functions is when we’re trying to mix too much-unrelated business logic into the function definitions. This makes it difficult to read and understand the logic involved in the functions, which may impair code maintainability. Some developers use documentation mechanisms such as `@doc` annotations to compensate for poor code readability, but unfortunately, with a multi-clause function, we can only use these annotations once per function name, particularly on the first or header function. As shown next, all other variations of the function need to be documented only with comments, a mechanism that cannot automate tests, leaving the code prone to bugs. + +```elixir +@doc """ +Update sharp product with 0 or empty count + +## Examples +iex> Namespace.Module.update(...) +expected result... +""" +def update(%Product{count: nil, material: material}) when material in ["metal", "glass"] do + # ... +end + +# update blunt product +def update(%Product{count: count, material: material}) when count > 0 and material in ["metal", "glass"] do + # ... +end + +# update animal... +def update(%Animal{count: 1, skin: skin}) when skin in ["fur", "hairy"] do + # ... +end +``` + +#### Refactoring + +As shown below, a possible solution to this anti-pattern is to break the business rules that are mixed up in a single unrelated multi-clause function in several different simple functions. Each function can have a specific `@doc`, describing its behavior and parameters received. While this refactoring sounds simple, it can have a lot of impact on the function's current users, so be careful! + +```elixir +@doc """ +Update sharp product + +## Parameter +struct: %Product{...} + +## Examples +iex> Namespace.Module.update_sharp_product(%Product{...}) +expected result... +""" +def update_sharp_product(struct) do + # ... +end + +@doc """ +Update blunt product + +## Parameter +struct: %Product{...} + +## Examples +iex> Namespace.Module.update_blunt_product(%Product{...}) +expected result... +""" +def update_blunt_product(struct) do + # ... +end + +@doc """ +Update animal + +## Parameter +struct: %Animal{...} + +## Examples +iex> Namespace.Module.update_animal(%Animal{...}) +expected result... +""" +def update_animal(struct) do + # ... +end +``` ## Feature envy From 336a5536866d0c9248ab8d3b09af26133a8247fa Mon Sep 17 00:00:00 2001 From: Andrea Leopardi Date: Thu, 14 Sep 2023 19:24:00 +0200 Subject: [PATCH 32/39] Apply suggestions from code review --- .../anti-patterns/design-anti-patterns.md | 50 ++++++++++--------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/lib/elixir/pages/anti-patterns/design-anti-patterns.md b/lib/elixir/pages/anti-patterns/design-anti-patterns.md index d771b596434..bdce2976bc6 100644 --- a/lib/elixir/pages/anti-patterns/design-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/design-anti-patterns.md @@ -138,26 +138,28 @@ Using multi-clause functions in Elixir, to group functions of the same name, is #### Example -A recurrent example of abusive use of multi-clause functions is when we’re trying to mix too much-unrelated business logic into the function definitions. This makes it difficult to read and understand the logic involved in the functions, which may impair code maintainability. Some developers use documentation mechanisms such as `@doc` annotations to compensate for poor code readability, but unfortunately, with a multi-clause function, we can only use these annotations once per function name, particularly on the first or header function. As shown next, all other variations of the function need to be documented only with comments, a mechanism that cannot automate tests, leaving the code prone to bugs. +A frequent example of this usage of multi-clause functions is when developers mix unrelated business logic into the function definitions. This makes it difficult to read and understand the logic involved in the functions, which may impair code maintainability. Some developers use documentation mechanisms such as `@doc` annotations to compensate for poor code readability, but unfortunately, with a multi-clause function, we can only use these annotations once per function name, particularly on the first or header function. As shown next, all other variations of the function need to be documented only with comments, a mechanism that cannot automate tests, leaving the code prone to bugs. ```elixir @doc """ -Update sharp product with 0 or empty count +Updates "sharp" product with `0` or empty count. ## Examples -iex> Namespace.Module.update(...) -expected result... + + iex> Namespace.Module.update(...) + ... + """ def update(%Product{count: nil, material: material}) when material in ["metal", "glass"] do # ... end -# update blunt product +# Updates "blunt" product def update(%Product{count: count, material: material}) when count > 0 and material in ["metal", "glass"] do # ... end -# update animal... +# Updates animal def update(%Animal{count: 1, skin: skin}) when skin in ["fur", "hairy"] do # ... end @@ -169,43 +171,43 @@ As shown below, a possible solution to this anti-pattern is to break the busines ```elixir @doc """ -Update sharp product - -## Parameter -struct: %Product{...} +Updates a "sharp" product. ## Examples -iex> Namespace.Module.update_sharp_product(%Product{...}) -expected result... + + iex> Namespace.Module.update_sharp_product(%Product{...}) + ... + """ +@spec update_sharp_product(Product.t()) :: term() def update_sharp_product(struct) do # ... end @doc """ -Update blunt product - -## Parameter -struct: %Product{...} +Updates a "blunt" product. ## Examples -iex> Namespace.Module.update_blunt_product(%Product{...}) -expected result... + + iex> Namespace.Module.update_blunt_product(%Product{...}) + ... + """ +@spec update_blunt_product(Product.t()) :: term() def update_blunt_product(struct) do # ... end @doc """ -Update animal - -## Parameter -struct: %Animal{...} +Updates an animal. ## Examples -iex> Namespace.Module.update_animal(%Animal{...}) -expected result... + + iex> Namespace.Module.update_animal(%Animal{...}) + ... + """ +@spec update_animal(Animal.t()) :: term() def update_animal(struct) do # ... end From 8e4f6e7140c1691b0e50b9f0d42550a8de226074 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Sat, 16 Sep 2023 11:29:47 +0200 Subject: [PATCH 33/39] Update design-anti-patterns.md --- .../anti-patterns/design-anti-patterns.md | 56 ++++++++----------- 1 file changed, 24 insertions(+), 32 deletions(-) diff --git a/lib/elixir/pages/anti-patterns/design-anti-patterns.md b/lib/elixir/pages/anti-patterns/design-anti-patterns.md index bdce2976bc6..5e955ce0327 100644 --- a/lib/elixir/pages/anti-patterns/design-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/design-anti-patterns.md @@ -138,77 +138,69 @@ Using multi-clause functions in Elixir, to group functions of the same name, is #### Example -A frequent example of this usage of multi-clause functions is when developers mix unrelated business logic into the function definitions. This makes it difficult to read and understand the logic involved in the functions, which may impair code maintainability. Some developers use documentation mechanisms such as `@doc` annotations to compensate for poor code readability, but unfortunately, with a multi-clause function, we can only use these annotations once per function name, particularly on the first or header function. As shown next, all other variations of the function need to be documented only with comments, a mechanism that cannot automate tests, leaving the code prone to bugs. +A frequent example of this usage of multi-clause functions is when developers mix unrelated business logic into the same function definition. Such functions often have generic names or too broad specifications, making it difficult for maintainers and users of said functions to maintain and understand them. + +Some developers may use documentation mechanisms such as `@doc` annotations to compensate for poor code readability, however the documentation itself may end-up full of conditionals to describe how the function behaves for each different argument combination. ```elixir @doc """ -Updates "sharp" product with `0` or empty count. +Updates a struct. -## Examples +If given a "sharp" product (metal or glass with empty count), +it will... - iex> Namespace.Module.update(...) - ... +If given a blunt product, it will... +If given an animal, it will... """ -def update(%Product{count: nil, material: material}) when material in ["metal", "glass"] do +def update(%Product{count: nil, material: material}) + when material in ["metal", "glass"] do # ... end -# Updates "blunt" product -def update(%Product{count: count, material: material}) when count > 0 and material in ["metal", "glass"] do +def update(%Product{count: count, material: material}) + when count > 0 and material not in ["metal", "glass"] do # ... end -# Updates animal -def update(%Animal{count: 1, skin: skin}) when skin in ["fur", "hairy"] do +def update(%Animal{count: 1, skin: skin}) + when skin in ["fur", "hairy"] do # ... end ``` #### Refactoring -As shown below, a possible solution to this anti-pattern is to break the business rules that are mixed up in a single unrelated multi-clause function in several different simple functions. Each function can have a specific `@doc`, describing its behavior and parameters received. While this refactoring sounds simple, it can have a lot of impact on the function's current users, so be careful! +As shown below, a possible solution to this anti-pattern is to break the business rules that are mixed up in a single unrelated multi-clause function in several different simple functions. More precise names make the scope of the function clear. Each function can have a specific `@doc`, describing its behavior and parameters received. While this refactoring sounds simple, it can have a lot of impact on the function's current users, so be careful! ```elixir @doc """ Updates a "sharp" product. -## Examples - - iex> Namespace.Module.update_sharp_product(%Product{...}) - ... - +It will... """ -@spec update_sharp_product(Product.t()) :: term() -def update_sharp_product(struct) do +def update_sharp_product(%Product{count: nil, material: material}) + when material in ["metal", "glass"] do # ... end @doc """ Updates a "blunt" product. -## Examples - - iex> Namespace.Module.update_blunt_product(%Product{...}) - ... - +It will... """ -@spec update_blunt_product(Product.t()) :: term() -def update_blunt_product(struct) do +def update_blunt_product(%Product{count: count, material: material}) + when count > 0 and material not in ["metal", "glass"] do # ... end @doc """ Updates an animal. -## Examples - - iex> Namespace.Module.update_animal(%Animal{...}) - ... - +It will... """ -@spec update_animal(Animal.t()) :: term() -def update_animal(struct) do +def update_animal(%Animal{count: 1, skin: skin}) + when skin in ["fur", "hairy"] do # ... end ``` From 9c2728cc30726b9b56b72531b5fe201c15a13768 Mon Sep 17 00:00:00 2001 From: Lucas Francisco da Matta Vegi Date: Mon, 18 Sep 2023 09:01:57 -0300 Subject: [PATCH 34/39] "Using application configuration for libraries" added (Anti-patterns documentation) --- .../anti-patterns/design-anti-patterns.md | 57 ++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/lib/elixir/pages/anti-patterns/design-anti-patterns.md b/lib/elixir/pages/anti-patterns/design-anti-patterns.md index 5e955ce0327..8680e5691fc 100644 --- a/lib/elixir/pages/anti-patterns/design-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/design-anti-patterns.md @@ -279,4 +279,59 @@ A common practice followed by the community is to make the non-raising version t ## Using application configuration for libraries -TODO +#### Problem + +The *[Application Environment](https://hexdocs.pm/elixir/1.15.5/Config.html)* is a mechanism that can be used to parameterize global values that will be used in several different places in a system implemented in Elixir. This parameterization mechanism can be very useful and therefore is not considered an anti-pattern by itself. However, when *Application Environments* are used as a mechanism for configuring a library's functions, this can make these functions less flexible, making it impossible for a library-dependent application to reuse their functions with different behaviors in different places in the code. Libraries are created to foster code reuse, so this limitation imposed by this parameterization mechanism can be problematic in this scenario. + +#### Example + +The `DashSplitter` module represents a library that configures the behavior of its functions through the global *Application Environment* mechanism. These configurations are concentrated in the *config/config.exs* file, shown below: + +```elixir +import Config + +config :app_config, + parts: 3 + +import_config "#{config_env()}.exs" +``` + +One of the functions implemented by the `DashSplitter` library is `split/1`. This function aims to separate a string received via a parameter into a certain number of parts. The character used as a separator in `split/1` is always `"-"` and the number of parts the string is split into is defined globally by the *Application Environment*. This value is retrieved by the `split/1` function by calling `Application.fetch_env!/2`, as shown next: + +```elixir +defmodule DashSplitter do + def split(string) when is_binary(string) do + parts = Application.fetch_env!(:app_config, :parts) # <= retrieve parameterized value + String.split(string, "-", parts: parts) # <= parts: 3 + end +end +``` + +Due to this parameterized value used by the `DashSplitter` library, all applications dependent on it can only use the `split/1` function with identical behavior about the number of parts generated by string separation. Currently, this value is equal to 3, as we can see in the use examples shown below: + +```elixir +iex> DashSplitter.split("Lucas-Francisco-Vegi") +["Lucas", "Francisco", "Vegi"] +iex> DashSplitter.split("Lucas-Francisco-da-Matta-Vegi") +["Lucas", "Francisco", "da-Matta-Vegi"] +``` + +#### Refactoring + +To remove this anti-pattern and make the library more adaptable and flexible, this type of configuration must be performed via parameters in function calls. The code shown below performs the refactoring of the `split/1` function by adding a new optional parameter of type *Keyword list*. With this new parameter, it is possible to modify the default behavior of the function at the time of its call, allowing multiple different ways of using `split/2` within the same application: + +```elixir +defmodule DashSplitter do + def split(string, opts \\ []) when is_binary(string) and is_list(opts) do + parts = Keyword.get(opts, :parts, 2) # <= default config of parts == 2 + String.split(string, "-", parts: parts) + end +end +``` + +```elixir +iex> DashSplitter.split("Lucas-Francisco-da-Matta-Vegi", [parts: 5]) +["Lucas", "Francisco", "da", "Matta", "Vegi"] +iex> DashSplitter.split("Lucas-Francisco-da-Matta-Vegi") #<= default config is used! +["Lucas", "Francisco-da-Matta-Vegi"] +``` From 4e70b0bbc1ee5cd4269ef27cc9f71c1ac4ea003b Mon Sep 17 00:00:00 2001 From: Andrea Leopardi Date: Mon, 18 Sep 2023 17:40:48 +0200 Subject: [PATCH 35/39] Update lib/elixir/pages/anti-patterns/design-anti-patterns.md --- lib/elixir/pages/anti-patterns/design-anti-patterns.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/elixir/pages/anti-patterns/design-anti-patterns.md b/lib/elixir/pages/anti-patterns/design-anti-patterns.md index 8680e5691fc..146fca8a1f9 100644 --- a/lib/elixir/pages/anti-patterns/design-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/design-anti-patterns.md @@ -281,7 +281,7 @@ A common practice followed by the community is to make the non-raising version t #### Problem -The *[Application Environment](https://hexdocs.pm/elixir/1.15.5/Config.html)* is a mechanism that can be used to parameterize global values that will be used in several different places in a system implemented in Elixir. This parameterization mechanism can be very useful and therefore is not considered an anti-pattern by itself. However, when *Application Environments* are used as a mechanism for configuring a library's functions, this can make these functions less flexible, making it impossible for a library-dependent application to reuse their functions with different behaviors in different places in the code. Libraries are created to foster code reuse, so this limitation imposed by this parameterization mechanism can be problematic in this scenario. +The [*application environment*](https://hexdocs.pm/elixir/1.15/Application.html#module-the-application-environment) can be used to parameterize global values that can be used in an Elixir system. This parameterization mechanism can be very useful and therefore is not considered an anti-pattern by itself. However, library authors should avoid using the application environment to configure their library. The reason is exactly that the application environment is a **global** state, so there can only be a single value for each key in the environment for an application. This makes it impossible for multiple applications depending on the same library to configure the same aspect of the library in different ways. #### Example From 0648c47fe39d57ebea522ad955bc82dbf9a85f87 Mon Sep 17 00:00:00 2001 From: Lucas Francisco da Matta Vegi Date: Mon, 18 Sep 2023 14:33:02 -0300 Subject: [PATCH 36/39] application environment format changes --- lib/elixir/pages/anti-patterns/design-anti-patterns.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/elixir/pages/anti-patterns/design-anti-patterns.md b/lib/elixir/pages/anti-patterns/design-anti-patterns.md index 146fca8a1f9..c20f4071cd2 100644 --- a/lib/elixir/pages/anti-patterns/design-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/design-anti-patterns.md @@ -285,7 +285,7 @@ The [*application environment*](https://hexdocs.pm/elixir/1.15/Application.html# #### Example -The `DashSplitter` module represents a library that configures the behavior of its functions through the global *Application Environment* mechanism. These configurations are concentrated in the *config/config.exs* file, shown below: +The `DashSplitter` module represents a library that configures the behavior of its functions through the global application environment mechanism. These configurations are concentrated in the *config/config.exs* file, shown below: ```elixir import Config @@ -296,7 +296,7 @@ config :app_config, import_config "#{config_env()}.exs" ``` -One of the functions implemented by the `DashSplitter` library is `split/1`. This function aims to separate a string received via a parameter into a certain number of parts. The character used as a separator in `split/1` is always `"-"` and the number of parts the string is split into is defined globally by the *Application Environment*. This value is retrieved by the `split/1` function by calling `Application.fetch_env!/2`, as shown next: +One of the functions implemented by the `DashSplitter` library is `split/1`. This function aims to separate a string received via a parameter into a certain number of parts. The character used as a separator in `split/1` is always `"-"` and the number of parts the string is split into is defined globally by the application environment. This value is retrieved by the `split/1` function by calling `Application.fetch_env!/2`, as shown next: ```elixir defmodule DashSplitter do From 4aaa1e4bf5fe6cc266abdce2ccddfd64ebf58aa7 Mon Sep 17 00:00:00 2001 From: Lucas Francisco da Matta Vegi Date: Mon, 18 Sep 2023 14:35:56 -0300 Subject: [PATCH 37/39] application environment format changes --- lib/elixir/pages/anti-patterns/design-anti-patterns.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/elixir/pages/anti-patterns/design-anti-patterns.md b/lib/elixir/pages/anti-patterns/design-anti-patterns.md index c20f4071cd2..a92a7e60b95 100644 --- a/lib/elixir/pages/anti-patterns/design-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/design-anti-patterns.md @@ -285,7 +285,7 @@ The [*application environment*](https://hexdocs.pm/elixir/1.15/Application.html# #### Example -The `DashSplitter` module represents a library that configures the behavior of its functions through the global application environment mechanism. These configurations are concentrated in the *config/config.exs* file, shown below: +The `DashSplitter` module represents a library that configures the behavior of its functions through the global application environment. These configurations are concentrated in the *config/config.exs* file, shown below: ```elixir import Config From a178d6c7163a1206187b9ffdca25e03d0271c26f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 21 Sep 2023 12:55:09 +0200 Subject: [PATCH 38/39] Update lib/elixir/pages/anti-patterns/design-anti-patterns.md --- lib/elixir/pages/anti-patterns/design-anti-patterns.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/elixir/pages/anti-patterns/design-anti-patterns.md b/lib/elixir/pages/anti-patterns/design-anti-patterns.md index a92a7e60b95..54298fe99ff 100644 --- a/lib/elixir/pages/anti-patterns/design-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/design-anti-patterns.md @@ -318,7 +318,7 @@ iex> DashSplitter.split("Lucas-Francisco-da-Matta-Vegi") #### Refactoring -To remove this anti-pattern and make the library more adaptable and flexible, this type of configuration must be performed via parameters in function calls. The code shown below performs the refactoring of the `split/1` function by adding a new optional parameter of type *Keyword list*. With this new parameter, it is possible to modify the default behavior of the function at the time of its call, allowing multiple different ways of using `split/2` within the same application: +To remove this anti-pattern and make the library more adaptable and flexible, this type of configuration must be performed via parameters in function calls. The code shown below performs the refactoring of the `split/1` function by accepting [keyword lists](`Keyword`) as a new optional parameter. With this new parameter, it is possible to modify the default behavior of the function at the time of its call, allowing multiple different ways of using `split/2` within the same application: ```elixir defmodule DashSplitter do From 37be21718846d8f1b5b167638746b17ab2961be5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 21 Sep 2023 12:56:24 +0200 Subject: [PATCH 39/39] Update lib/elixir/pages/anti-patterns/design-anti-patterns.md --- lib/elixir/pages/anti-patterns/design-anti-patterns.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/elixir/pages/anti-patterns/design-anti-patterns.md b/lib/elixir/pages/anti-patterns/design-anti-patterns.md index 54298fe99ff..c866062ac3e 100644 --- a/lib/elixir/pages/anti-patterns/design-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/design-anti-patterns.md @@ -281,7 +281,7 @@ A common practice followed by the community is to make the non-raising version t #### Problem -The [*application environment*](https://hexdocs.pm/elixir/1.15/Application.html#module-the-application-environment) can be used to parameterize global values that can be used in an Elixir system. This parameterization mechanism can be very useful and therefore is not considered an anti-pattern by itself. However, library authors should avoid using the application environment to configure their library. The reason is exactly that the application environment is a **global** state, so there can only be a single value for each key in the environment for an application. This makes it impossible for multiple applications depending on the same library to configure the same aspect of the library in different ways. +The [*application environment*](https://hexdocs.pm/elixir/Application.html#module-the-application-environment) can be used to parameterize global values that can be used in an Elixir system. This mechanism can be very useful and therefore is not considered an anti-pattern by itself. However, library authors should avoid using the application environment to configure their library. The reason is exactly that the application environment is a **global** state, so there can only be a single value for each key in the environment for an application. This makes it impossible for multiple applications depending on the same library to configure the same aspect of the library in different ways. #### Example