TECHNOLOGY

Catalog of Elixir-particular code smells

Run in Livebook

GitHub last commit
Twitter URL

Table of Contents

Introduction

Elixir is a recent functional programming language whose reputation is rising within the industry link. Alternatively, there are few works within the scientific literature targeted on studying the inner quality of systems applied in this language.

In snarl to better realize the forms of sub-optimum code structures that would maybe harm the inner quality of Elixir systems, we scoured net sites, blogs, forums, and movies (grey literature overview), taking a put a query to for particular code smells for Elixir which can maybe be mentioned by its builders.

Attributable to this investigation, now we non-public initially proposed a catalog of 18 recent smells which can maybe be particular to Elixir systems. Assorted smells are being suggested by the neighborhood, so this catalog is continuously being as a lot as this point (currently 22 smells). These code smells are classified into two varied groups (non-public-connected and low-stage considerations), in response to the accomplish of affect and code extent they’ve an affect on. This catalog of Elixir-particular code smells is supplied below. Every code scent is documented the spend of the next structure:

  • Identify: Outlandish identifier of the code scent. This title is foremost to facilitate conversation between builders;
  • Class: The a part of code tormented by scent and its severity;
  • Disaster: How the code scent can harm code quality and what impacts this can non-public for builders;
  • Example: Code and textual descriptions to illustrate the occurrence of the code scent;
  • Refactoring: Strategies to replace stinky code in snarl to increase its qualities. Examples of refactored code are supplied to illustrate these adjustments.

The target of this catalog of code smells is to instigate the arrive of the quality of code developed in Elixir. That’s the reason, we are eager in vivid Elixir’s neighborhood conception about these code smells: Develop you agree that these code smells will be bad? Comprise you viewed any of them in manufacturing code? Develop you non-public any strategies about some Elixir-particular code scent no longer cataloged by us?…

Please in actuality be tickled to discover pull requests and systems (Points tab). We desire to listen to from you!

▲ lend a hand to Index

Invent-connected smells

Invent-connected smells are extra complicated, non-public an attach on a hideous-grained code ingredient, and are therefore extra powerful to detect. On this allotment, 13 varied smells classified as non-public-connected are explained and exemplified:

GenServer Envy

  • Class: Invent-connected scent.

  • Disaster: In Elixir, processes will be primitively created by Kernel.spawn/1, Kernel.spawn/3, Kernel.spawn_link/1 and Kernel.spawn_link/3 functions. Even though it’s far doubtless to carry out them this methodology, it’s far extra normal to spend abstractions (e.g., Agent, Project, and GenServer) equipped by Elixir to carry out processes. The utilization of every and each particular abstraction will not be any longer a code scent in itself; on the opposite hand, there will be disaster when both a Project or Agent is ancient past its suggested functions, being treated love a GenServer.

  • Example: As confirmed subsequent, Agent and Project are abstractions to carry out processes with in actuality educated functions. In distinction, GenServer is a extra generic abstraction ancient to carry out processes for many varied functions:

    • Agent: As Elixir works on the precept of immutability, by default no fee is shared between quite loads of areas of code, enabling read and write as in a world variable. An Agent is a straightforward task abstraction targeted on fixing this limitation, enabling processes to part insist.
    • Project: This task abstraction is ancient when we easiest need to enact some particular action asynchronously, in general in an isolated methodology, without conversation with varied processes.
    • GenServer: Here is the most generic task abstraction. The foremost merit of this abstraction is explicitly segregating the server and the buyer roles, thus offering an even bigger API for the group of processes conversation. In addition to that, a GenServer can additionally encapsulate insist (love an Agent), provide sync and async calls (love a Project), and additional.

    Examples of this code scent appear when Brokers or Responsibilities are ancient for general functions and no longer lawful for in actuality educated ones reminiscent of their documentation suggests. As an example some scent occurrences, we can cite two particular scenarios. 1) When a Project is ancient no longer easiest to async enact an action, however additionally to generally alternate messages with varied processes; 2) When an Agent, beside sharing some global fee between processes, is additionally generally ancient to enact isolated projects which can maybe be no longer of passion to varied processes.

  • Refactoring: When an Agent or Project goes past its suggested spend conditions and becomes painful, it’s far good to refactor it real into a GenServer.

▲ lend a hand to Index


Agent Obsession

  • Class: Invent-connected scent.

  • Disaster: In Elixir, an Agent is a task abstraction targeted on sharing files between processes thru message passing. It is a straightforward wrapper around shared files, thus facilitating its read and replace from anywhere within the code. The utilization of an Agent to part files will not be any longer a code scent in itself; on the opposite hand, when the responsibility for interacting without lengthen with an Agent is unfold all over your complete machine, this would possibly maybe be problematic. This contaminated apply can magnify the risk of code upkeep and discover the code extra liable to bugs.

  • Example: The following code seeks to illustrate this scent. The responsibility for interacting without lengthen with the Agent is unfold all over four varied modules (i.e, A, B, C, and D).

    %{a: scream material} terminate)
    #…
    terminate
    terminate”>

    defmodule B enact
      #...
      def replace(pid) enact
        #...
        Agent.replace(pid, fn scream material -> %{a: scream material} terminate)
        #...
      terminate
    terminate
    scream material terminate)
    #…
    terminate
    terminate”>

    defmodule D enact
      #...
      def discover(pid) enact
        #...
        Agent.discover(pid, fn scream material -> scream material terminate)
        #...
      terminate
    terminate

    This spreading of responsibility can generate duplicated code and discover code upkeep extra hard. Also, due to lack of lend a hand an eye on over the structure of the shared files, complicated silent files will be shared. This freedom to spend any structure of files is bad and can induce builders to introduce bugs.

    Refactoring: Pretty than spreading order discover admission to to an Agent over many areas within the code, it’s far good to refactor this code by centralizing the responsibility for interacting with an Agent in a single module. This refactoring improves the maintainability by eliminating duplicated code; it additionally means that you just can limit the well-liked structure for shared files, cutting back malicious program-proneness. As confirmed below, the module KV.Bucket is centralizing the responsibility for interacting with the Agent. Any varied place within the code that desires to discover admission to shared files need to now delegate this action to KV.Bucket. Also, KV.Bucket now easiest permits files to be shared in Scheme structure.

    %{} terminate)
    terminate

    @doc “””
    Gets a fee from the `bucket` by `key`.
    “””
    def discover(bucket, key) enact
    Agent.discover(bucket, &Scheme.discover(&1, key))
    terminate

    @doc “””
    Places the `fee` for the given `key` within the `bucket`.
    “””
    def put(bucket, key, fee) enact
    Agent.replace(bucket, &Scheme.put(&1, key, fee))
    terminate
    terminate”>

    defmodule KV.Bucket enact
      spend Agent
    
      @doc """
      Starts a recent bucket.
      """
      def start_link(_opts) enact
        Agent.start_link(fn -> %{} terminate)
      terminate
    
      @doc """
      Gets a fee from the `bucket` by `key`.
      """
      def discover(bucket, key) enact
        Agent.discover(bucket, &Scheme.discover(&1, key))
      terminate
    
      @doc """
      Places the `fee` for the given `key` within the `bucket`.
      """
      def put(bucket, key, fee) enact
        Agent.replace(bucket, &Scheme.put(&1, key, fee))
      terminate
    terminate

    The following are examples of pointers on how to delegate discover admission to to shared files (equipped by an Agent) to KV.Bucket.

    , 3) iex(3)> KV.Bucket.put(bucket, "beer", 7) # gaining access to shared files of particular keys iex(4)> KV.Bucket.discover(bucket, "beer") 7 iex(5)> KV.Bucket.discover(bucket, "milk") 3

    These examples are in response to code written in Elixir’s legit documentation. Source: link

▲ lend a hand to Index


Unsupervised task

  • Class: Invent-connected scent.

  • Disaster: In Elixir, making a task open air a supervision tree will not be any longer a code scent in itself. Alternatively, when code creates a gargantuan quantity of prolonged-working processes open air a supervision tree, this would possibly maybe discover visibility and monitoring of those processes hard, preventing builders from entirely controlling their applications.

  • Example: The following code example seeks to illustrate a library to blame for affirming a numerical Counter thru a GenServer task open air a supervision tree. Multiple counters will be created simultaneously by a consumer (one task for each and each counter), making these unsupervised processes hard to lend a hand an eye on. It will space off considerations with the initialization, restart, and shutdown of a machine.

    Counter.open up(0)
    {:okay, #PID<0.115.0>}

    iex(2)> Counter.discover()
    0

    iex(3)> Counter.open up(15, C2)
    {:okay, #PID<0.120.0>}

    iex(4)> Counter.discover(C2)
    15

    iex(5)> Counter.bump(-3, C2)
    12

    iex(6)> Counter.bump(7)
    7″>

    defmodule Counter enact
      spend GenServer
    
      @moduledoc """
        Global counter applied thru a GenServer task
        open air a supervision tree.
      """
    
      @doc """
        Characteristic to carry out a counter.
          initial_value: any integer fee.
          pid_name: no longer obligatory parameter to elaborate the task title.
                    Default is Counter.
      """
      def open up(initial_value, pid_name \ __MODULE__)
        when is_integer(initial_value) enact
        GenServer.open up(__MODULE__, initial_value, title: pid_name)
      terminate
    
      @doc """
        Characteristic to discover the counter's recent fee.
          pid_name: no longer obligatory parameter to uncover the task title.
                    Default is Counter.
      """
      def discover(pid_name \ __MODULE__) enact
        GenServer.call(pid_name, :discover)
      terminate
    
      @doc """
        Characteristic to adjustments the counter's recent fee.
        Returns the as a lot as this point fee.
          fee: quantity to be added to the counter.
          pid_name: no longer obligatory parameter to uncover the task title.
                    Default is Counter.
      """
      def bump(fee, pid_name \ __MODULE__) enact
        GenServer.call(pid_name, {:bump, fee})
        discover(pid_name)
      terminate
    
      ## Callbacks
    
      @impl lawful
      def init(counter) enact
        {:okay, counter}
      terminate
    
      @impl lawful
      def handle_call(:discover, _from, counter) enact
        {:answer, counter, counter}
      terminate
    
      def handle_call({:bump, fee}, _from, counter) enact
        {:answer, counter, counter + fee}
      terminate
    terminate
    
    #...Consume examples...
    
    iex(1)> Counter.open up(0)
    {:okay, #PID<0.115.0>}
    
    iex(2)> Counter.discover()
    0
    
    iex(3)> Counter.open up(15, C2)
    {:okay, #PID<0.120.0>}
    
    iex(4)> Counter.discover(C2)
    15
    
    iex(5)> Counter.bump(-3, C2)
    12
    
    iex(6)> Counter.bump(7)
    7
  • Refactoring: To make certain that that that purchasers of a library non-public chubby lend a hand an eye on over their systems, in spite of the amount of processes ancient and the lifetime of every and everyone, all processes need to be began inner a supervision tree. As confirmed below, this code makes spend of a Supervisor link as a supervision tree. When this Elixir utility is began, two varied counters (Counter and C2) are additionally began as child processes of the Supervisor named App.Supervisor. Every are initialized with zero. Via this supervision tree, it’s far doubtless to lend a hand an eye on the lifecycle of all child processes (e.g., stopping or restarting each and everyone), bettering the visibility of your complete app.

    These examples are in response to codes written in Elixir’s legit documentation. Source: link

▲ lend a hand to Index


Immense messages between processes

  • Class: Invent-connected scent.

  • Disaster: In Elixir, processes bustle in an isolated procedure, in general at the same time as with varied Elixir. Conversation between varied processes is performed through message passing. The alternate of messages between processes will not be any longer a code scent in itself; on the opposite hand, when processes alternate messages, their contents are copied between them. That’s the reason, if a wide structure is despatched as a message from one task to 1 more, the sender can become blocked, compromising efficiency. If these gargantuan message exchanges happen generally, the prolonged and frequent blocking off of processes can space off a machine to behave anomalously.

  • Example: The following code consists of two modules that can each and each bustle in a varied task. As the names counsel, the Sender module has a purpose to blame for sending messages from one task to 1 more (i.e., send_msg/3). The Receiver module has a purpose to carry out a task to discover messages (i.e., carry out/0) and one more one to contend with the received messages (i.e., bustle/0). If a wide structure, reminiscent of a listing with 1_000_000 varied values, is despatched generally from Sender to Receiver, the impacts of this scent would be felt.

    msg_received
    {_, _} -> “would maybe no longer match”
    terminate
    terminate

    @doc “””
    Originate a task to discover a message.
    Messages are received within the bustle() purpose of Receiver.
    “””
    def carry out enact
    spawn(Receiver, :bustle, [])
    terminate
    terminate”>

    defmodule Receiver enact
      @doc """
        Characteristic for receiving messages from processes.
      """
      def bustle enact
        discover enact
          {:msg, msg_received} -> msg_received
          {_, _} -> "would maybe no longer match"
        terminate
      terminate
    
      @doc """
        Originate a task to discover a message.
        Messages are received within the bustle() purpose of Receiver.
      """
      def carry out enact
        spawn(Receiver, :bustle, [])
      terminate
    terminate

    defmodule Sender enact
      @doc """
        Characteristic for sending messages between processes.
          pid_receiver: message recipient.
          msg: messages of any form and size will be despatched.
          id_msg: ancient by receiver to settle what to enact
                  when a message arrives.
                  Default is the atom :msg
      """
      def send_msg(pid_receiver, msg, id_msg \ :msg) enact
        send(pid_receiver, {id_msg, msg})
      terminate
    terminate

    Examples of gargantuan messages between processes:

    ", to: "#PID<0.144.0>" }}

    This instance is in response to a usual code by Samuel Mullen. Source: link

▲ lend a hand to Index


Advanced multi-clause purpose

  • Class: Invent-connected scent.

  • Disaster: The utilization of multi-clause functions in Elixir, to community functions of the same title, will not be any longer a code scent in itself. Alternatively, due to wide flexibility equipped by this programming characteristic, some builders would maybe abuse the amount of guard clauses and pattern fits to community unrelated functionality.

  • Example: A recurrent example of abusive spend of the multi-clause functions is when we’re trying to combine too a lot industrial common sense into the aim definitions. This makes it hard to read and realize the common sense focused on the functions, that would maybe impair code maintainability. Some builders spend documentation mechanisms reminiscent of @doc annotations to make amends for terrible code readability, however unfortunately, with a multi-clause purpose, we can easiest spend these annotations as soon as per purpose title, significantly on the first or header purpose. As confirmed subsequent, all varied adaptations of the aim need to be documented easiest with comments, a mechanism that would maybe no longer automate tests, leaving the code liable to bugs.

    Namespace.Module.replace(…)
    expected outcome…
    “””
    def replace(%Product{depend: nil, subject fabric: subject fabric})
    when subject fabric in [“metal”, “glass”] enact
    # …
    terminate

    # replace blunt product
    def replace(%Product{depend: depend, subject fabric: subject fabric})
    when depend > 0 and subject fabric in [“metal”, “glass”] enact
    # …
    terminate

    # replace animal…
    def replace(%Animal{depend: 1, skin: skin})
    when skin in [“fur”, “hairy”] enact
    # …
    terminate”>

    @doc """
      Exchange interesting product with 0 or empty depend
    
      ## Examples
    
        iex> Namespace.Module.replace(...)
        expected outcome...
    """
    def replace(%Product{depend: nil, subject fabric: subject fabric})
      when subject fabric in ["metal", "glass"] enact
      # ...
    terminate
    
    # replace blunt product
    def replace(%Product{depend: depend, subject fabric: subject fabric})
      when depend > 0 and subject fabric in ["metal", "glass"] enact
      # ...
    terminate
    
    # replace animal...
    def replace(%Animal{depend: 1, skin: skin})
      when skin in ["fur", "hairy"] enact
      # ...
    terminate
  • Refactoring: As confirmed below, a doubtless respond to this scent is to interrupt the industrial rules which can maybe be blended up in a single complicated multi-clause purpose in quite loads of assorted straightforward functions. Every purpose can non-public a particular @doc, describing its habits and parameters received. Whereas this refactoring sounds straightforward, it would possibly maybe actually non-public quite loads of affect on the aim’s recent purchasers, so be cautious!

    Namespace.Module.update_sharp_product(%Product{…})
    expected outcome…
    “””
    def update_sharp_product(struct) enact
    # …
    terminate

    @doc “””
    Exchange blunt product

    ## Parameter
    struct: %Product{…}

    ## Examples

    iex> Namespace.Module.update_blunt_product(%Product{…})
    expected outcome…
    “””
    def update_blunt_product(struct) enact
    # …
    terminate

    @doc “””
    Exchange animal

    ## Parameter
    struct: %Animal{…}

    ## Examples

    iex> Namespace.Module.update_animal(%Animal{…})
    expected outcome…
    “””
    def update_animal(struct) enact
    # …
    terminate”>

    @doc """
      Exchange interesting product
    
      ## Parameter
        struct: %Product{...}
    
      ## Examples
    
        iex> Namespace.Module.update_sharp_product(%Product{...})
        expected outcome...
    """
    def update_sharp_product(struct) enact
      # ...
    terminate
    
    @doc """
      Exchange blunt product
    
      ## Parameter
        struct: %Product{...}
    
      ## Examples
    
        iex> Namespace.Module.update_blunt_product(%Product{...})
        expected outcome...
    """
    def update_blunt_product(struct) enact
      # ...
    terminate
    
    @doc """
      Exchange animal
    
      ## Parameter
        struct: %Animal{...}
    
      ## Examples
    
        iex> Namespace.Module.update_animal(%Animal{...})
        expected outcome...
    """
    def update_animal(struct) enact
      # ...
    terminate

    This instance is in response to a usual code by Syamil MJ (@syamilmj). Source: link

▲ lend a hand to Index


Advanced extraction in clauses

  • Class: Invent-connected scent.

  • Swear: This scent turned into suggested by the neighborhood through factors (#9).

  • Disaster: After we spend multi-clause functions, it’s far doubtless to extract values within the clauses for additional utilization and for pattern matching/guard checking. This extraction itself would now not signify a code scent, however when you happen to’ve got too many clauses or too many arguments, it becomes laborious to clutch which extracted aspects are ancient for pattern/guards and what’s ancient easiest contained within the aim body. This scent is expounded to Advanced multi-clause purpose, however with implications of its non-public. It impairs the code readability in a varied methodology.

  • Example: The following code, although straightforward, tries to illustrate the occurrence of this code scent. The multi-clause purpose pressure/1 is extracting fields of an %User{} struct in its clauses for additional utilization (title) and for pattern/guard checking (age).

    def pressure(%User{title: title, age: age}) when age >= 18 enact
      "#{title} can pressure"
    terminate
    
    def pressure(%User{title: title, age: age}) when age < 18 enact
      "#{title} can no longer pressure"
    terminate

    Whereas the example is small and appears to be like to be love a transparent code, strive to imagine a subject the place pressure/1 turned into extra complicated, having many extra clauses, arguments, and extractions. Here is the in actuality stinky code!

  • Refactoring: As confirmed below, a doubtless respond to this scent is to extract easiest pattern/guard connected variables within the signature whenever you non-public many arguments or quite loads of clauses:

    def pressure(%User{age: age} = particular person) when age >= 18 enact
      %User{title: title} = particular person
      "#{title} can pressure"
    terminate
    
    def pressure(%User{age: age} = particular person) when age < 18 do
      %User{name: name} = user
      "#{name} cannot drive"
    end

    This example and the refactoring are proposed by José Valim (@josevalim)

▲ back to Index


Complex branching

  • Category: Design-related smell.

  • Note: Formerly known as “Complex API error handling”.

  • Problem: When a function assumes the responsibility of handling multiple errors alone, it can increase its cyclomatic complexity (metric of control-flow) and become incomprehensible. This situation can configure a specific instance of “Long function”, a traditional code smell, but has implications of its own. Under these circumstances, this function could get very confusing, difficult to maintain and test, and therefore bug-proneness.

  • Example: An example of this code smell is when a function uses the case control-flow structure or other similar constructs (e.g., cond, or receive) to handle multiple variations of response types returned by the same API endpoint. This practice can make the function more complex, long, and difficult to understand, as shown next.

    {:ok, body}
    {:ok, %Tesla.Env{body: body}} -> {:error, body}
    {:error, _} = varied -> varied
    terminate
    terminate”>

    def get_customer(customer_id) enact
      case discover("/prospects/#{customer_id}") enact
        {:okay, %Tesla.Env{space: 200, body: body}} -> {:okay, body}
        {:okay, %Tesla.Env{body: body}} -> {:error, body}
        {:error, _} = varied -> varied
      terminate
    terminate

    Even though get_customer/1 will not be any longer in actuality prolonged in this instance, it would maybe also be. Focused on this extra complicated subject, the place a gargantuan quantity of assorted responses will be equipped to the same endpoint, will not be any longer a correct conception to pay attention all on a single purpose. Here is a dangerous subject, the place a cramped bit typo, or any danger launched by the programmer in handling a response form, would maybe finally compromise the handling of all responses from the endpoint (if the aim raises an exception, as an illustration).

  • Refactoring: As confirmed below, in this subject, in desire to concentrating all handlings within the same purpose, making a posh branching, it’s far good to delegate each and each branch (handling of a response form) to a varied inner most purpose. On this methodology, the code will doubtless be cleaner, extra concise, and readable.

    success_api_response(body)
    {:okay, %Tesla.Env{body: body}} -> x_error_api_response(body)
    {:error, _} = varied -> y_error_api_response(varied)
    terminate
    terminate

    defp success_api_response(body) enact
    {:okay, body}
    terminate

    defp x_error_api_response(body) enact
    {:error, body}
    terminate

    defp y_error_api_response(varied) enact
    varied
    terminate”>

    def get_customer(customer_id) when is_integer(customer_id) enact
      case discover("/prospects/#{customer_id}") enact
        {:okay, %Tesla.Env{space: 200, body: body}} -> success_api_response(body)
        {:okay, %Tesla.Env{body: body}} -> x_error_api_response(body)
        {:error, _} = varied -> y_error_api_response(varied)
      terminate
    terminate
    
    defp success_api_response(body) enact
      {:okay, body}
    terminate
    
    defp x_error_api_response(body) enact
      {:error, body}
    terminate
    
    defp y_error_api_response(varied) enact
      varied
    terminate

    Whereas this instance of refactoring get_customer/1 would possibly maybe appear moderately extra verbose than the authentic code, endure in strategies to imagine a subject the place get_customer/1 is to blame for handling a quantity a lot higher than three varied forms of doubtless responses. Here is the stinky subject!

    This instance is in response to code written by Zack MrDoops and Dimitar Panayotov dimitarvp. Source: link. We bought strategies from José Valim (@josevalim) on the refactoring.

▲ lend a hand to Index


Advanced else clauses in with

  • Class: Invent-connected scent.

  • Swear: This scent turned into suggested by the neighborhood through factors (#7).

  • Disaster: This code scent refers to with statements that flatten all its error clauses real into a single complicated else block. This subject is bad to the code readability and maintainability due to hard to clutch from which clause the error fee came.

  • Example: An example of this code scent, as confirmed below, is a purpose open_decoded_file/1 that read a bad 64 encoded string scream material from a file and returns a decoded binary string. This purpose makes spend of a with commentary that desires to contend with two doubtless errors, all of that are concentrated in a single complicated else block.

    Refactoring: As confirmed below, in this subject, in desire to concentrating all error handlings within a single complicated else block, it’s far good to normalize the return forms specifically inner most functions. On this methodology, due to its group, the code will doubtless be cleaner and additional readable.

    This instance and the refactoring are proposed by José Valim (@josevalim)

▲ lend a hand to Index


Exceptions for lend a hand an eye on-waft

  • Class: Invent-connected scent.

  • Disaster: This scent refers to code that forces builders to contend with exceptions for lend a hand an eye on-waft. Exception handling itself would now not signify a code scent, however this would possibly maybe restful no longer be the most easy alternative on hand to builders to contend with an error in consumer code. When builders discover no longer non-public any freedom to settle if an error is extra special or no longer, that is regarded as as a code scent.

  • Example: An example of this code scent, as confirmed below, is when a library (e.g. MyModule) forces its purchasers to spend strive .. rescue statements to settle and preserve into consideration errors. This library would now not allow builders to settle if an error is extra special or no longer in their applications.

    defmodule MyModule enact
      def janky_function(fee) enact
        if is_integer(fee) enact
          #...
          "Consequence..."
        else
          elevate RuntimeError, message: "invalid argument. Isn't any longer integer!"
        terminate
      terminate
    terminate

    reason = e.message
    “Uh oh! #{reason}.”
    terminate
    terminate

    terminate

    #…Consume examples…

    iex(1)> Client.foo(1)
    “All correct! Consequence….”

    iex(2)> Client.foo(“lucas”)
    “Uh oh! invalid argument. Isn’t any longer integer!.””>

    defmodule Client enact
    
      # Client forced to spend exceptions for lend a hand an eye on-waft.
      def foo(arg) enact
        strive enact
          fee = MyModule.janky_function(arg)
          "All correct! #{fee}."
        rescue
          e in RuntimeError ->
            reason = e.message
            "Uh oh! #{reason}."
        terminate
      terminate
    
    terminate
    
    #...Consume examples...
    
    iex(1)> Client.foo(1)
    "All correct! Consequence...."
    
    iex(2)> Client.foo("lucas")
    "Uh oh! invalid argument. Isn't any longer integer!."
  • Refactoring: Library authors would maybe restful dispute that purchasers are no longer required to spend exceptions for lend a hand an eye on-waft in their applications. As confirmed below, this would possibly maybe be performed by refactoring the library MyModule, offering two variations of the aim that forces purchasers to spend exceptions for lend a hand an eye on-waft (e.g., janky_function). 1) a model with the raised exceptions would maybe restful non-public the same title as the stinky one, however with a trailing ! (i.e., janky_function!); 2) Yet any other model, without raised exceptions, would maybe restful non-public a title same to the authentic model (i.e., janky_function), and will return the outcome wrapped in a tuple.


    outcome
    {:error, message} ->
    elevate RuntimeError, message: message
    terminate
    terminate
    terminate”>

    defmodule MyModule enact
      @moduledoc """
        Refactored library
      """
    
      @doc """
        Refactored model without exceptions for lend a hand an eye on-waft.
      """
      def janky_function(fee) enact
        if is_integer(fee) enact
          #...
          {:okay, "Consequence..."}
        else
          {:error, "invalid argument. Isn't any longer integer!"}
        terminate
      terminate
    
      def janky_function!(fee) enact
        case janky_function(fee) enact
          {:okay, outcome} ->
            outcome
          {:error, message} ->
            elevate RuntimeError, message: message
        terminate
      terminate
    terminate

    This refactoring presents purchasers extra freedom to settle pointers on how to proceed within the event of errors, defining what’s extra special or no longer in varied scenarios. As confirmed subsequent, when an error will not be any longer powerful, purchasers can spend particular lend a hand an eye on-waft structures, reminiscent of the case commentary alongside with pattern matching.

    #{fee}." {:error, reason} -> "Uh oh! #{reason}." terminate terminate terminate #...Consume examples... iex(1)> Client.foo(1) "All correct! Consequence...." iex(2)> Client.foo("lucas") "Uh oh! invalid argument. Isn't any longer integer!."

    This instance is in response to code written by Tim Austin neenjaw and Angelika Tyborska angelikatyborska. Source: link

▲ lend a hand to Index


Untested polymorphic habits

  • Class: Invent-connected scent.

  • Disaster: This code scent refers to functions that non-public protocol-dependent parameters and are therefore polymorphic. A polymorphic purpose itself would now not signify a code scent, however some builders enforce these generic functions without accompanying guard clauses, allowing to droop parameters that enact no longer enforce the required protocol or that discover no longer non-public any that arrangement.

  • Example: An event of this code scent occurs when a purpose makes spend of to_string() to rework files received by parameter. The aim to_string() makes spend of the protocol String.Chars for conversions. Many Elixir files forms (e.g., BitString, Integer, Drift, URI) enforce this protocol. Alternatively, as confirmed below, varied Elixir files forms (e.g., Scheme) enact no longer enforce it and can space off an error in dasherize/1 purpose. Looking on the topic, this habits will be desired or no longer. In addition to that, it would maybe no longer discover sense to dasherize a URI or a quantity as confirmed subsequent.

    CodeSmells.dasherize(URI.parse(“http://www.code_smells.com”)) #<= Makes sense? "http://www.code-smells.com" iex(4)> CodeSmells.dasherize(%{last_name: “vegi”, first_name: “lucas”})
    (Protocol.UndefinedError) protocol String.Chars no longer applied
    for %{first_name: “lucas”, last_name: “vegi”} of form Scheme”>

    defmodule CodeSmells enact
      def dasherize(files) enact
        to_string(files)
        |> String.replace("_", "-")
      terminate
    terminate
    
    #...Consume examples...
    
    iex(1)> CodeSmells.dasherize("Lucas_Vegi")
    "Lucas-Vegi"
    
    iex(2)> CodeSmells.dasherize(10)  #<= Makes sense?
    "10"
    
    iex(3)> CodeSmells.dasherize(URI.parse("http://www.code_smells.com")) #<= Makes sense?
    "http://www.code-smells.com"
    
    iex(4)> CodeSmells.dasherize(%{last_name: "vegi", first_name: "lucas"})
     (Protocol.UndefinedError) protocol String.Chars no longer applied
    for %{first_name: "lucas", last_name: "vegi"} of form Scheme
  • Refactoring: There are two foremost doubtless choices to increase code tormented by this scent. 1) You would possibly both preserve away the protocol spend (i.e., to_string/1), by adding multi-clauses on dasherize/1 or lawful preserve away it; or 2) You would possibly doc that dasherize/1 makes spend of the protocol String.Chars for conversions, showing its consequences. As confirmed subsequent, we refactored the spend of the first alternative, eliminating the protocol and proscribing dasherize/1 parameter easiest to desired files forms (i.e., BitString and Atom). In addition to that, we spend @doc to validate dasherize/1 for desired inputs and to doc the habits to some forms that we mediate discover no longer discover sense for the aim (e.g., Integer and URI).

    CodeSmells.dasherize(:lucas_vegi)
    “lucas-vegi”

    iex> CodeSmells.dasherize(“Lucas_Vegi”)
    “Lucas-Vegi”

    iex> CodeSmells.dasherize(%{last_name: “vegi”, first_name: “lucas”})
    (FunctionClauseError) no purpose clause matching in CodeSmells.dasherize/1

    iex> CodeSmells.dasherize(URI.parse(“http://www.code_smells.com”))
    (FunctionClauseError) no purpose clause matching in CodeSmells.dasherize/1

    iex> CodeSmells.dasherize(10)
    (FunctionClauseError) no purpose clause matching in CodeSmells.dasherize/1
    “””
    def dasherize(files) when is_atom(files) enact
    dasherize(Atom.to_string(files))
    terminate

    def dasherize(files) when is_binary(files) enact
    String.replace(files, “_”, “-“)
    terminate
    terminate

    #…Consume examples…

    iex(1)> CodeSmells.dasherize(:lucas_vegi)
    “lucas-vegi”

    iex(2)> CodeSmells.dasherize(“Lucas_Vegi”)
    “Lucas-Vegi”

    iex(3)> CodeSmells.dasherize(10)
    (FunctionClauseError) no purpose clause matching in CodeSmells.dasherize/1″>

    defmodule CodeSmells enact
      @doc """
        Characteristic that converts underscores to dashes.
    
        ## Parameter
          files: easiest BitString and Atom are supported.
    
        ## Examples
    
            iex> CodeSmells.dasherize(:lucas_vegi)
            "lucas-vegi"
    
            iex> CodeSmells.dasherize("Lucas_Vegi")
            "Lucas-Vegi"
    
            iex> CodeSmells.dasherize(%{last_name: "vegi", first_name: "lucas"})
            (FunctionClauseError) no purpose clause matching in CodeSmells.dasherize/1
    
            iex> CodeSmells.dasherize(URI.parse("http://www.code_smells.com"))
            (FunctionClauseError) no purpose clause matching in CodeSmells.dasherize/1
    
            iex> CodeSmells.dasherize(10)
            (FunctionClauseError) no purpose clause matching in CodeSmells.dasherize/1
      """
      def dasherize(files) when is_atom(files) enact
        dasherize(Atom.to_string(files))
      terminate
    
      def dasherize(files) when is_binary(files) enact
        String.replace(files, "_", "-")
      terminate
    terminate
    
    #...Consume examples...
    
    iex(1)> CodeSmells.dasherize(:lucas_vegi)
    "lucas-vegi"
    
    iex(2)> CodeSmells.dasherize("Lucas_Vegi")
    "Lucas-Vegi"
    
    iex(3)> CodeSmells.dasherize(10)
     (FunctionClauseError) no purpose clause matching in CodeSmells.dasherize/1

    This instance is in response to code written by José Valim (@josevalim). Source: link

▲ lend a hand to Index


Different return forms

  • Class: Invent-connected scent.

  • Swear: This scent turned into suggested by the neighborhood through factors (#6).

  • Disaster: This code scent refers to functions that discover choices (e.g., keyword checklist) parameters that significantly substitute its return form. Attributable to choices are no longer obligatory and generally space dynamically, if they substitute the return form it will be laborious to model what the aim if truth be told returns.

  • Example: An example of this code scent, as confirmed below, is when a library (e.g. AlternativeInteger) has a multi-clause purpose parse/2 with many alternative return forms. Looking on the decisions received as a parameter, the aim can non-public a varied return form.

    ) {13, "..."} iex(2)> AlternativeInteger.parse("13", discard_rest: lawful) 13 iex(3)> AlternativeInteger.parse("13", discard_rest: false) {13, "..."}
  • Refactoring: To refactor this scent, as confirmed subsequent, it’s better to be able to add within the library a particular purpose for each and each return form (e.g., parse_no_rest/1), no longer delegating this to an choices parameter.

    ) {13, "..."} iex(2)> AlternativeInteger.parse_no_rest("13") 13

    This instance and the refactoring are proposed by José Valim (@josevalim)

▲ lend a hand to Index


Code group by task

  • Class: Invent-connected scent.

  • Disaster: This scent refers to code that is unnecessarily organized by processes. A task itself would now not signify a code scent, however it absolutely would maybe restful easiest be ancient to model runtime properties (e.g., concurrency, discover admission to to shared sources, event scheduling). When a task is ancient for code group, it would maybe carry out bottlenecks within the machine.

  • Example: An example of this code scent, as confirmed below, is a library that implements arithmetic operations (e.g., add, subtract) thru a GenSever tasklink. If the amount of calls to this single task grows, this code group can compromise the machine efficiency, therefore becoming a bottleneck.

    {:okay, pid} = GenServer.start_link(Calculator, :init)
    {:okay, #PID<0.132.0>}

    #…Consume examples…
    iex(2)> Calculator.add(1, 5, pid)
    6

    iex(3)> Calculator.subtract(2, 3, pid)
    -1″>

    defmodule Calculator enact
      spend GenServer
    
      @moduledoc """
        Calculator that performs two normal arithmetic operations.
        This code is unnecessarily organized by a GenServer task.
      """
    
      @doc """
        Characteristic to discover the sum of two values.
      """
      def add(a, b, pid) enact
        GenServer.call(pid, {:add, a, b})
      terminate
    
      @doc """
        Characteristic to discover subtraction of two values.
      """
      def subtract(a, b, pid) enact
        GenServer.call(pid, {:subtract, a, b})
      terminate
    
      def init(init_arg) enact
        {:okay, init_arg}
      terminate
    
      def handle_call({:add, a, b}, _from, insist) enact
        {:answer, a + b, insist}
      terminate
    
      def handle_call({:subtract, a, b}, _from, insist) enact
        {:answer, a - b, insist}
      terminate
    terminate
    
    # Initiate a generic server task
    iex(1)> {:okay, pid} = GenServer.start_link(Calculator, :init)
    {:okay, #PID<0.132.0>}
    
    #...Consume examples...
    iex(2)> Calculator.add(1, 5, pid)
    6
    
    iex(3)> Calculator.subtract(2, 3, pid)
    -1
    
  • Refactoring: In Elixir, as confirmed subsequent, code group need to be performed easiest by modules and functions. At any time when doubtless, a library would maybe restful no longer impose particular habits (reminiscent of parallelization) on its purchasers. It’s better to delegate this behavioral decision to the builders of purchasers, thus rising the opportunity of code reuse of a library.

    This instance is in response to code equipped in Elixir’s legit documentation. Source: link

▲ lend a hand to Index


Data manipulation by migration

  • Class: Invent-connected scent.

  • Disaster: This code scent refers to modules that discover each and each files and structural adjustments in a database schema through Ecto.Migrationlink. Migrations need to be ancient completely to modify a database schema over time (e.g., by including or other than columns and tables). When this responsibility is blended with files manipulation code, the module becomes much less cohesive, extra hard to examine, and therefore extra liable to bugs.

  • Example: An example of this code scent is when an Ecto.Migration is ancient simultaneously to alter a desk, adding a recent column to it, and additionally to replace all pre-reward files in that desk, assigning a fee to this recent column. As confirmed below, to boot to adding the is_custom_shop column within the guitars desk, this Ecto.Migration adjustments the pricetag of this column for some particular guitar models.

    Enum.plot(&update_guitars/1)
    terminate

    @doc “””
    A purpose that updates values of column “is_custom_shop” to lawful.
    “””
    defp update_guitars({discover, model, year}) enact
    from(g in Guitar,
    the place: g.discover == ^discover and g.model == ^model and g.year == ^year,
    rob: g
    )
    |> Repo.update_all(space: [is_custom_shop: true])
    terminate

    @doc “””
    Characteristic that defines which guitar models that must non-public the values
    of the “is_custom_shop” column as a lot as this point to lawful.
    “””
    defp custom_shop_entries() enact
    [
    {“Gibson”, “SG”, 1999},
    {“Fender”, “Telecaster”, 2020}
    ] terminate
    terminate”>

    defmodule GuitarStore.Repo.Migrations.AddIsCustomShopToGuitars enact
      spend Ecto.Migration
    
      import Ecto.Ask
      alias GuitarStore.Stock.Guitar
      alias GuitarStore.Repo
    
      @doc """
        A purpose that modifies the structure of desk "guitars",
        adding column "is_custom_shop" to it. By default, all files
        pre-kept in this desk can non-public the fee false kept
        in this recent column.
    
        Also, this purpose updates the "is_custom_shop" column fee
        of some guitar models to lawful.
      """
      def substitute enact
        alter desk("guitars") enact
          add :is_custom_shop, :boolean, default: false
        terminate
        carry out index("guitars", ["is_custom_shop"])
    
        custom_shop_entries()
        |> Enum.plot(&update_guitars/1)
      terminate
    
      @doc """
        A purpose that updates values of column "is_custom_shop" to lawful.
      """
      defp update_guitars({discover, model, year}) enact
        from(g in Guitar,
          the place: g.discover == ^discover and g.model == ^model and g.year == ^year,
          rob: g
        )
        |> Repo.update_all(space: [is_custom_shop: true])
      terminate
    
      @doc """
        Characteristic that defines which guitar models that must non-public the values
        of the "is_custom_shop" column as a lot as this point to lawful.
      """
      defp custom_shop_entries() enact
        [
          {"Gibson", "SG", 1999},
          {"Fender", "Telecaster", 2020}
        ]
      terminate
    terminate

    You would possibly bustle this stinky migration above by going to the root of your mission and typing the next train through console:

  • Refactoring: To preserve away this code scent, it’s far serious to separate the guidelines manipulation in a mix task link varied from the module that performs the structural adjustments within the database through Ecto.Migration. This separation of responsibilities is an excellent apply for rising code testability. As confirmed below, the module AddIsCustomShopToGuitars now spend Ecto.Migration easiest to discover structural adjustments within the database schema:

    defmodule GuitarStore.Repo.Migrations.AddIsCustomShopToGuitars enact
      spend Ecto.Migration
    
      @doc """
        A purpose that modifies the structure of desk "guitars",
        adding column "is_custom_shop" to it. By default, all files
        pre-kept in this desk can non-public the fee false kept
        in this recent column.
      """
      def substitute enact
        alter desk("guitars") enact
          add :is_custom_shop, :boolean, default: false
        terminate
    
        carry out index("guitars", ["is_custom_shop"])
      terminate
    terminate

    Furthermore, the recent mix task PopulateIsCustomShop, confirmed subsequent, has easiest the responsibility to discover files manipulation, thus bettering testability:

    Enum.plot(&update_guitars/1)
    terminate

    defp update_guitars({discover, model, year}) enact
    from(g in Guitar,
    the place: g.discover == ^discover and g.model == ^model and g.year == ^year,
    rob: g
    )
    |> Repo.update_all(space: [is_custom_shop: true])
    terminate

    defp custom_shop_entries() enact
    [
    {“Gibson”, “SG”, 1999},
    {“Fender”, “Telecaster”, 2020}
    ] terminate
    terminate”>

    defmodule Mix.Responsibilities.PopulateIsCustomShop enact
      @shortdoc "Populates is_custom_shop column"
    
      spend Mix.Project
    
      import Ecto.Ask
      alias GuitarStore.Stock.Guitar
      alias GuitarStore.Repo
    
      @requirements ["app.start"]
    
      def bustle(_) enact
        custom_shop_entries()
        |> Enum.plot(&update_guitars/1)
      terminate
    
      defp update_guitars({discover, model, year}) enact
        from(g in Guitar,
          the place: g.discover == ^discover and g.model == ^model and g.year == ^year,
          rob: g
        )
        |> Repo.update_all(space: [is_custom_shop: true])
      terminate
    
      defp custom_shop_entries() enact
        [
          {"Gibson", "SG", 1999},
          {"Fender", "Telecaster", 2020}
        ]
      terminate
    terminate

    You would possibly bustle this mix task above by typing the next train through console:

      mix populate_is_custom_shop

    This instance is in response to code initially written by Carlos Souza. Source: link

▲ lend a hand to Index

Low-stage considerations smells

Low-stage considerations smells are extra straightforward than non-public-connected smells and non-public an attach on a small section of the code. Next, all 9 varied smells classified as low-stage considerations are explained and exemplified:

Working with invalid files

  • Class: Low-stage considerations smells.

  • Disaster: This code scent refers to a purpose that would now not validate its parameters’ forms and therefore can discover inner non-predicted habits. When an error is raised inner a purpose due to an invalid parameter fee, this would possibly maybe confuse the builders and discover it extra powerful to to find and fix the error.

  • Example: An example of this code scent is when a purpose receives an invalid parameter after which passes it to a purpose from a third-discover collectively library. This would space off an error (raised deep contained within the library purpose), which can maybe be complicated for the developer who’s working with invalid files. As confirmed subsequent, the aim foo/1 is a consumer of a third-discover collectively library and would now not validate its parameters at the boundary. On this methodology, it’s far doubtless that invalid files will doubtless be handed from foo/1 to the library, causing a mysterious error.

    ) (ArithmeticError) contaminated argument in arithmetic expression: 1 + "Lucas" :erlang.+(1, "Lucas") library.ex: 3: ThirdPartyLibrary.sum/2
  • Refactoring: To preserve away this code scent, consumer code need to validate enter parameters at the boundary with the actual person, through guard clauses or pattern matching. This would prevent errors from going down deeply, making them more straightforward to model. This refactoring will additionally allow libraries to be applied without caring about creating inner safety mechanisms. The subsequent code illustrates the refactoring of foo/1, eliminating this scent:

    ) (FunctionClauseError) no purpose clause matching in MyApp.foo/1 The following arguments were given to MyApp.foo/1: # 1 "Lucas" my_app.ex: 6: MyApp.foo/1

    This instance is in response to code equipped in Elixir’s legit documentation. Source: link

▲ lend a hand to Index


Scheme/struct dynamic discover admission to

  • Class: Low-stage considerations smells.

  • Disaster: In Elixir, it’s far doubtless to discover admission to values from Maps, that are key-fee files structures, both strictly or dynamically. When trying to dynamically discover admission to the pricetag of a key from a Scheme, if the educated key would now not exist, a null fee (nil) will doubtless be returned. This return will be complicated and would now not allow builders to enact whether or no longer the foremost is non-existent within the Scheme or lawful has no dash fee. On this methodology, this code scent would maybe space off bugs within the code.

  • Example: The code confirmed below is an example of this scent. The aim purpose/1 tries to arrangement a graphic to indicate the place of some extent in a cartesian airplane. This purpose receives a parameter of Scheme form with the purpose attributes, which in general is some extent of a 2D or 3D cartesian coordinate machine. To settle if some extent is 2D or 3D, this purpose makes spend of dynamic discover admission to to retrieve values of the Scheme keys:

    As will be viewed within the example above, even when the foremost :z would now not exist within the Scheme (point_2d), dynamic discover admission to returns the fee nil. This return will be bad thanks to its ambiguity. It will not be any longer doubtless to enact from it whether or no longer the Scheme has the foremost :z or no longer. If the aim depends on the return fee to discover decisions about pointers on how to purpose some extent, this would possibly maybe be problematic and even space off errors when trying out the code.

  • Refactoring: To preserve away this code scent, every time a Scheme has keys of Atom form, replace the dynamic discover admission to to its values per strict discover admission to. When a non-existent key is exactly accessed, Elixir raises an error straight away, allowing builders to gain bugs faster. The subsequent code illustrates the refactoring of purpose/1, eliminating this scent:

    As confirmed below, one more alternative to refactor this scent is to replace a Scheme with a struct (named plot). By default, structs easiest beef up strict discover admission to to values. On this methodology, accesses will continuously return definite and purpose outcomes:

    defmodule Point enact
      @enforce_keys [:x, :y]
      defstruct [x: nil, y: nil]
    terminate
    
    #...Consume examples...
    iex(1)> point = %Point{x: 2, y: 3}
    %Point{x: 2, y: 3}
    
    iex(2)> point.x   # <= strict access to use point values
    2
    
    iex(3)> point.z   # <= trying to access a non-existent key
     (KeyError) key :z not found in: %Point{x: 2, y: 3}
    
    iex(4)> point[:x] # <= by default, struct does not support dynamic access
     (UndefinedFunctionError) ... (Point does not implement the Access behaviour)

    These examples are based on code written by José Valim (@josevalim). Source: link

▲ back to Index


Unplanned value extraction

  • Category: Low-level concerns smells.

  • Problem: In Elixir, there are many ways to extract key-related values from a URL query string. However, when pattern matching is not used for this purpose, depending on the format of the URL query string, the code may have undesired behavior, such as being able to extract unplanned values instead of forcing a crash. This unplanned value extraction can give a false impression that the code is working correctly, causing bugs.

  • Example: The code shown below is an example of this smell. 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:


    key_value = String.split(pair, “=”)
    Enum.at(key_value, 0) == desired_key && Enum.at(key_value, 1)
    end)
    end

    end

    #…Use examples…

    # URL query string according to with the planned format – OK!
    iex(1)> Extract.get_value(“title=Lucas&college=UFMG&lab=ASERG”, “lab”)
    “ASERG”

    iex(2)> Extract.get_value(“title=Lucas&college=UFMG&lab=ASERG”, “college”)
    “UFMG”

    # Unplanned URL put a query to string structure – Unplanned fee extraction!
    iex(3)> Extract.get_value(“title=Lucas&college=institution=UFMG&lab=ASERG”, “college”)
    “institution” # <= why not "institution=UFMG"? or only "UFMG"?">

    defmodule Extract enact
    
      @doc """
        Extract fee from a key in a URL put a query to string.
      """
      def get_value(string, desired_key) enact
        aspects = String.split(string, "&")
        Enum.find_value(aspects, fn pair ->
          key_value = String.split(pair, "=")
          Enum.at(key_value, 0) == desired_key && Enum.at(key_value, 1)
        terminate)
      terminate
    
    terminate
    
    #...Consume examples...
    
    # URL put a query to string in response to with the deliberate structure - OK!
    iex(1)> Extract.get_value("title=Lucas&college=UFMG&lab=ASERG", "lab")
    "ASERG"
    
    iex(2)> Extract.get_value("title=Lucas&college=UFMG&lab=ASERG", "college")
    "UFMG"
    
    # Unplanned URL put a query to string structure - Unplanned fee extraction!
    iex(3)> Extract.get_value("title=Lucas&college=institution=UFMG&lab=ASERG", "college")
    "institution"   # <= why no longer "institution=UFMG"? or easiest "UFMG"?
  • Refactoring: To preserve away this code scent, get_value/2 will be refactored thru the spend of pattern matching. So, if an unexpected URL put a query to string structure is ancient, the aim will doubtless be demolish in desire to returning an invalid fee. This habits, confirmed below, will allow purchasers to settle pointers on how to contend with these errors and will no longer give a false influence that the code is working accurately when unexpected values are extracted:


    [key, value] = String.split(pair, “=”) # <= pattern matching key == desired_key && value end) end end #...Use examples... # URL query string according to with the planned format - OK! iex(1)> Extract.get_value(“title=Lucas&college=UFMG&lab=ASERG”, “title”)
    “Lucas”

    # Unplanned URL put a query to string structure – Smash explaining the risk to the buyer!
    iex(2)> Extract.get_value(“title=Lucas&college=institution=UFMG&lab=ASERG”, “college”)
    (MatchError) no match of lawful hand side fee: [“university”, “institution”, “UFMG”] extract.ex:7: anonymous fn/2 in Extract.get_value/2 # <= left hand: [key, value] pair iex(3)> Extract.get_value(“title=Lucas&college&lab=ASERG”, “college”)
    (MatchError) no match of lawful hand side fee: [“university”] extract.ex:7: anonymous fn/2 in Extract.get_value/2 # <= left hand: [key, value] pair">

    defmodule Extract enact
    
      @doc """
        Extract fee from a key in a URL put a query to string.
        Refactored by the spend of pattern matching.
      """
      def get_value(string, desired_key) enact
        aspects = String.split(string, "&")
        Enum.find_value(aspects, fn pair ->
          [key, value] = String.split(pair, "=") # <= pattern matching
          key == desired_key && value
        end)
      end
    
    end
    
    #...Use examples...
    
    # URL query string according to with the planned format - OK!
    iex(1)> Extract.get_value("title=Lucas&college=UFMG&lab=ASERG", "title")
    "Lucas"
    
    # Unplanned URL put a query to string structure - Smash explaining the risk to the buyer!
    iex(2)> Extract.get_value("title=Lucas&college=institution=UFMG&lab=ASERG", "college")
     (MatchError) no match of lawful hand side fee: ["university", "institution", "UFMG"]
      extract.ex: 7: anonymous fn/2 in Extract.get_value/2 # <= left hand: [key, value] pair
    
    iex(3)> Extract.get_value("title=Lucas&college&lab=ASERG", "college")
    (MatchError) no match of lawful hand side fee: ["university"]
      extract.ex: 7: anonymous fn/2 in Extract.get_value/2 # <= left hand: [key, value] pair

    These examples are based on code written by José Valim (@josevalim). Source: link

▲ back to Index


Modules with identical names

  • Category: Low-level concerns smells.

  • Problem: This code smell is related to possible module name conflicts that can occur when a library is implemented. Due to a limitation of the Erlang VM (BEAM), also used by Elixir, only one instance of a module can be loaded at a time. If there are name conflicts between more than one module, they will be considered the same by BEAM and only one of them will be loaded. This can cause unwanted code behavior.

  • Example: The code shown below is an example of this smell. Two different modules were defined with identical names (Foo). When BEAM tries to load both simultaneously, only the module defined in the file (module_two.ex) stay loaded, redefining the current version of Foo (module_one.ex) in memory. That makes it impossible to call from_module_one/0, for example:

    defmodule Foo do
      @moduledoc """
        Defined in `module_one.ex` file.
      """
      def from_module_one do
        "Function from module one!"
      end
    end

    defmodule Foo do
      @moduledoc """
        Defined in `module_two.ex` file.
      """
      def from_module_two do
        "Function from module two!"
      end
    end

    When BEAM tries to load both simultaneously, the name conflict causes only one of them to stay loaded:

    iex(1)> c("module_one.ex")
    [Foo]
    
    iex(2)> c("module_two.ex")
    warning: redefining module Foo (recent model defined in memory)
    module_two.ex: 1
    [Foo]
    
    iex(3)> Foo.from_module_two()
    "Characteristic from module two!"
    
    iex(4)> Foo.from_module_one()  # <= impossible to call due to name conflict
     (UndefinedFunctionError) function Foo.from_module_one/0 is undefined...
  • Refactoring: To remove this code smell, a library must standardize the naming of its modules, always using its own name as a prefix (namespace) for all its module’s names (e.g., LibraryName.ModuleName). When a module file is within subdirectories of a library, the names of the subdirectories must also be used in the module naming (e.g., LibraryName.SubdirectoryName.ModuleName). In the refactored code shown below, this module naming pattern was used. For this, the Foo module, defined in the file module_two.ex, was also moved to the utils subdirectory. This refactoring, in addition to eliminating the internal conflict of names within the library, will prevent the occurrence of name conflicts with client code:

    defmodule MyLibrary.Foo do
      @moduledoc """
        Defined in `module_one.ex` file.
        Name refactored!
      """
      def from_module_one do
        "Function from module one!"
      end
    end

    defmodule MyLibrary.Utils.Foo do
      @moduledoc """
        Defined in `module_two.ex` file.
        Name refactored!
      """
      def from_module_two do
        "Function from module two!"
      end
    end

    When BEAM tries to load them simultaneously, both will stay loaded successfully:

    ) [MyLibrary.Foo] iex(2)> c("module_two.ex") [MyLibrary.Utils.Foo] iex(3)> MyLibrary.Foo.from_module_one() "Characteristic from module one!" iex(4)> MyLibrary.Utils.Foo.from_module_two() "Characteristic from module two!"

    This instance is in response to the outline equipped in Elixir’s legit documentation. Source: link

▲ lend a hand to Index


Pointless macro

  • Class: Low-stage considerations smells.

  • Disaster: Macros are powerful meta-programming mechanisms that will be ancient in Elixir to lengthen the language. Whereas implementing macros will not be any longer a code scent in itself, this meta-programming mechanism would maybe restful easiest be ancient when entirely foremost. At any time when a macro is applied, and it turned into doubtless to resolve the same danger the spend of functions or varied pre-reward Elixir structures, the code becomes unnecessarily extra complicated and much less readable. Attributable to macros are extra hard to enforce and realize, their indiscriminate spend can compromise the evolution of a machine, cutting back its maintainability.

  • Example: The code confirmed below is an example of this scent. The MyMacro module implements the sum/2 macro to discover the sum of two numbers received as parameters. Whereas this code has no syntax errors and will be performed accurately to discover the specified outcome, it’s far unnecessarily extra complicated. By implementing this functionality as a macro in place of a frail purpose, the code turned much less definite and much less purpose:

    Refactoring: To preserve away this code scent, the developer need to replace the pointless macro with structures which can maybe be extra effective to write and realize, reminiscent of named functions. The code confirmed below is the outcome of the refactoring of the old example. In most cases, the sum/2 macro has been transformed real into a frail named purpose:

    This instance is in response to the outline equipped in Elixir’s legit documentation. Source: link

▲ lend a hand to Index


Immense code generation

  • Class: Low-stage considerations smells.

  • Swear: This scent turned into suggested by the neighborhood through factors (#13).

  • Disaster: This code scent is expounded to macros that generate too a lot code. When a macro presents a gargantuan code generation, it impacts how the compiler or the runtime works. The explanation within the lend a hand of that is that Elixir will non-public to discover bigger, bring collectively, and enact a code quite loads of times, that can discover compilation slower.

  • Example: The code confirmed below is an example of this scent. Believe you would possibly maybe be defining a router for a net based utility, the place you would possibly maybe non-public macros love discover/2. On every invocation of the macro, which can maybe be hundreds, the code inner discover/2 will doubtless be expanded and compiled, which can generate a gargantuan volume of code in full.

    defmodule Routes enact
      ...
    
      defmacro discover(route, handler) enact
        quote enact
          route = unquote(route)
          handler = unquote(handler)
    
          if no longer is_binary(route) enact
            elevate ArgumentError, "route need to be a binary"
          terminate
    
          if no longer is_atom(handler) enact
            elevate ArgumentError, "route need to be a module"
          terminate
    
          @store_route_for_compilation {route, handler}
        terminate
      terminate
    terminate
  • Refactoring: To preserve away this code scent, the developer need to simplify the macro, delegating to varied functions section of its work. As confirmed below, by encapsulating within the aim __define__/3 the functionality pre-reward contained within the quote, we cut back the code that is expanded and compiled on every invocation of the macro, and as a replacement we dispatch to a purpose to enact the majority of the work.

    defmodule Routes enact
      ...
    
      defmacro discover(route, handler) enact
        quote enact
          Routes.__define__(__MODULE__, unquote(route), unquote(handler))
        terminate
      terminate
    
      def __define__(module, route, handler) enact
    
        if no longer is_binary(route) enact
          elevate ArgumentError, "route need to be a binary"
        terminate
    
        if no longer is_atom(handler) enact
          elevate ArgumentError, "route need to be a module"
        terminate
    
        Module.put_attribute(module, :store_route_for_compilation, {route, handler})
      terminate
    terminate

    This instance and the refactoring are proposed by José Valim (@josevalim)

▲ lend a hand to Index


App configuration for code libs

  • Class: Low-stage considerations smells.

  • Disaster: The Utility Ambiance link is a world configuration mechanism and therefore will be ancient to parameterize values that will doubtless be ancient in quite loads of assorted areas in a machine applied in Elixir. This parameterization mechanism will be very helpful and therefore will not be any longer regarded as as a code scent by itself. Alternatively, when Utility Environments are ancient as a mechanism for configuring a library’s functions, this would possibly maybe discover these functions much less versatile, making it unimaginable for a library-dependent utility to reuse its functions with varied behaviors in varied areas within the code. Libraries are created to foster code reuse, so this accomplish of limitation imposed by global configurations will be problematic in this subject.

  • Example: The DashSplitter module represents a library that configures the habits of its functions thru the global Utility Ambiance mechanism. These configurations are concentrated within the config/config.exs file, confirmed below:

    import Config
    
    config :app_config,
      aspects: 3
    
    import_config "#{config_env()}.exs"

    One in every of the functions applied by the DashSplitter library is split/1. This purpose has the cause of environment apart a string received through parameter real into a definite quantity of aspects. The personality ancient as a separator in split/1 is continuously "-" and the amount of aspects the string is split into is defined globally by the Utility Ambiance. This fee is retrieved by the split/1 purpose by calling Utility.fetch_env!/2, as confirmed subsequent:

    defmodule DashSplitter enact
      def split(string) when is_binary(string) enact
        aspects = Utility.fetch_env!(:app_config, :aspects) # <= retrieve global config
        String.split(string, "-", aspects: aspects)             # <= parts: 3
      end
    end

    Due to this type of global configuration used by the DashSplitter library, all applications dependent on it can only use the split/1 function with identical behavior in relation to 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:

    ) ["Lucas", "Francisco", "Vegi"] iex(2)> DashSplitter.split("Lucas-Francisco-da-Matta-Vegi") ["Lucas", "Francisco", "da-Matta-Vegi"]
  • Refactoring: To preserve away this code scent and discover the library extra adaptable and versatile, this accomplish of configuration need to be performed through parameters in purpose calls. The code confirmed below performs the refactoring of the split/1 purpose by adding a recent no longer obligatory parameter of form Keyword checklist. With this recent parameter it’s far doubtless to modify the default habits of the aim at the time of its call, allowing quite loads of assorted ways of the spend of split/2 within the same utility:

    DashSplitter.split(“Lucas-Francisco-da-Matta-Vegi”, [parts: 5])
    [“Lucas”, “Francisco”, “da”, “Matta”, “Vegi”]

    iex(2)> DashSplitter.split(“Lucas-Francisco-da-Matta-Vegi”) #<= default config is used! ["Lucas", "Francisco-da-Matta-Vegi"]">

    defmodule DashSplitter enact
      def split(string, opts \ []) when is_binary(string) and is_list(opts) enact
        aspects = Keyword.discover(opts, :aspects, 2) # <= default config of parts == 2
        String.split(string, "-", parts: parts)
      end
    end
    
    #...Use examples...
    
    iex(1)> DashSplitter.split("Lucas-Francisco-da-Matta-Vegi", [parts: 5])
    ["Lucas", "Francisco", "da", "Matta", "Vegi"]
    
    iex(2)> DashSplitter.split("Lucas-Francisco-da-Matta-Vegi") #<= default config is ancient!
    ["Lucas", "Francisco-da-Matta-Vegi"]

    These examples are in response to code equipped in Elixir’s legit documentation. Source: link

▲ lend a hand to Index


Collect-time app configuration

  • Class: Low-stage considerations smells.

  • Disaster: As explained within the outline of App configuration for code libs, the Utility Ambiance will be ancient to parameterize values in an Elixir machine. Even though it’s far rarely a correct apply to spend this mechanism within the implementation of libraries, generally this would possibly maybe be unavoidable. If these parameterized values are assigned to module attributes, it will be especially problematic. As module attribute values are defined at bring collectively-time, when trying to assign Utility Ambiance values to those attributes, warnings or errors will be caused by Elixir. This occurs due to, when defining module attributes at bring collectively time, the Utility Ambiance will not be any longer yet on hand in memory.

  • Example: The DashSplitter module represents a library. This module has an attribute @aspects that has its constant fee defined at bring collectively-time by calling Utility.fetch_env!/2. The split/1 purpose, applied by this library, has the cause of environment apart a string received through parameter real into a definite quantity of aspects. The personality ancient as a separator in split/1 is continuously "-" and the amount of aspects the string is split into is defined by the module attribute @aspects, as confirmed subsequent:

    defmodule DashSplitter enact
      @aspects Utility.fetch_env!(:app_config, :aspects) # <= elaborate module attribute
                                                            # at bring collectively-time
      def split(string) when is_binary(string) enact
        String.split(string, "-", aspects: @aspects) #<= reading from a module attribute
      terminate
    
    terminate

    Resulting from this bring collectively-time configuration in response to the Utility Ambiance mechanism, Elixir can elevate warnings or errors, as confirmed subsequent, throughout compilation:

    warning: Application.fetch_env!/2 is melancholy in the module body,
    spend Utility.compile_env/3 instead...
    
     (ArgumentError) would maybe no longer rating utility ambiance :aspects
    for utility :app_config due to the utility turned into no longer loaded nor
    configured
  • Refactoring: To preserve away this code scent, when it’s far entirely unavoidable to spend the Utility Ambiance mechanism to configure library functions, this would possibly maybe restful be performed at runtime and no longer throughout compilation. That is, in desire to calling Utility.fetch_env!(:app_config, :aspects) at bring collectively-time to space @aspects, this purpose need to be called at runtime within split/1. This would mitigate the trouble that Utility Ambiance will not be any longer yet on hand in memory when it’s far serious to spend it. Yet any other doubtless refactoring, as confirmed below, is to replace the spend of the Utility.fetch_env!/2 purpose to elaborate @aspects, with the Utility.compile_env/3. The third parameter of Utility.compile_env/3 defines a default fee that is returned every time that Utility Ambiance will not be any longer on hand in memory throughout the definition of @aspects. This prevents Elixir from elevating an error at bring collectively-time:

    defmodule DashSplitter enact
      @aspects Utility.compile_env(:app_config, :aspects, 3) # <= default fee 3 prevents an error!
    
      def split(string) when is_binary(string) enact
        String.split(string, "-", aspects: @aspects) #<= reading from a module attribute
      terminate
    
    terminate

    These examples are in response to code equipped in Elixir’s legit documentation. Source: link

  • Observation: This code scent will be detected by Credo, a static code evaluation instrument. In some unspecified time in the future of its checks, Credo raises this warning when this scent is chanced on.

▲ lend a hand to Index


Dependency with “spend” when an “import” is enough

  • Class: Low-stage considerations smells.

  • Disaster: Elixir has mechanisms reminiscent of import, alias, and spend to attach dependencies between modules. Organising dependencies permits a module to call functions from varied modules, facilitating code reuse. A code applied with these mechanisms would now not signify a scent by itself; on the opposite hand, while the import and alias directives non-public lexical scope and easiest facilitate that a module to spend functions of one more, the spend directive has a broader scope, one thing that will be problematic. The spend directive permits a module to inject any accomplish of code into one more, including propagating dependencies. On this methodology, the spend of the spend directive makes code readability worse, due to to model exactly what will happen when it references a module, it’s far serious to non-public files of the inner small print of the referenced module.

  • Example: The code confirmed below is an example of this scent. Three varied modules were defined — ModuleA, Library, and ClientApp. ClientApp is reusing code from the Library through the spend directive, however is blind to its inner small print. Resulting from this truth, when Library is referenced by ClientApp, it injects into ClientApp the total scream material tag in its __using__/1 macro. Resulting from the reduced readability of the code and the shortcoming of skills of the inner small print of the Library, ClientApp defines a neighborhood purpose foo/0. This would generate a battle as ModuleA additionally has a purpose foo/0; when ClientApp referenced Library through the spend directive, it has a dependency for ModuleA propagated to itself:

    defmodule ModuleA enact
      def foo enact
        "From Module A"
      terminate
    terminate

    defmodule Library enact
      defmacro __using__(_opts) enact
        quote enact
          import ModuleA  # <= propagating dependencies!
    
          def from_lib enact
            "From Library"
          terminate
        terminate
      terminate
    
      def from_lib enact
        "From Library"
      terminate
    terminate
    ” – ” <> foo()
    terminate

    terminate”>

    defmodule ClientApp enact
      spend Library
    
      def foo enact
        "Local purpose from consumer app"
      terminate
    
      def from_client_app enact
        from_lib() <> " - " <> foo()
      terminate
    
    terminate

    After we strive to bring collectively ClientApp, Elixir will detect the battle and throw the next error:

    ) (CompileError) client_app.ex: 4: imported ModuleA.foo/0 conflicts with local purpose
  • Refactoring: To preserve away this code scent, it will be doubtless to replace spend with alias or import when making a dependency between an utility and a library. This would discover code habits clearer, due to improved readability. Within the next code, ClientApp turned into refactored in this methodology, and with that, the battle as previously confirmed no longer exists:

    ” – ” <> foo()
    terminate

    terminate

    #…Makes spend of example…

    iex(1)> ClientApp.from_client_app()
    “From Library – Local purpose from consumer app””>

    defmodule ClientApp enact
      import Library
    
      def foo enact
        "Local purpose from consumer app"
      terminate
    
      def from_client_app enact
        from_lib() <> " - " <> foo()
      terminate
    
    terminate
    
    #...Makes spend of example...
    
    iex(1)> ClientApp.from_client_app()
    "From Library - Local purpose from consumer app"

    These examples are in response to code equipped in Elixir’s legit documentation. Source: link

▲ lend a hand to Index

About

This catalog turned into proposed by Lucas Vegi and Marco Tulio Valente, from ASERG/DCC/UFMG.

For added files leer the next paper:

Please in actuality be tickled to discover pull requests and systems (Points tab).

▲ lend a hand to Index

  1. These code smells were suggested by the Elixir neighborhood. 2 3 4

Related Articles

Leave a Reply

Your email address will not be published.

Back to top button