Emily

Emily OP

Well, after the mess my last project in Python ended up being, I’m committed this time to make the cleanest code possible.

Before I get any bad habits, I’d appreciate a second set of eyes on the code below.

If you see any bad directions I’m going down, or ways to make the code cleaner, I’d really appreciate being set on the right track now.

(See code in full context below.)

How would I make this code cleaner? (Pipeline operator? Recursion maybe?)

def calc_pyramid_prices_percents(prices, initial_percent) do

    total_column_percent = calc_total_column_percent(prices, initial_percent)
    total_pyramid_percent = calc_total_pyramid_percent(total_column_percent)
    [ initial_price | _ ] = prices
    total_distance = calc_total_distance(prices, initial_price)

    Enum.map(prices, fn(price) -> calc_price_percent(price, initial_price, \
        total_distance, initial_percent, total_pyramid_percent) end)
end

Is there a cleaner way to do this?

def calc_max_initial_percent(prices) do
    inverted_prices = Enum.reverse(prices)
    [d] = Enum.take((calc_pyramid_prices_percents(inverted_prices, 0)), -1) # get last price percent.
    d # Return only the number.
end

Do you see any red flags or better coding practices that should have been followed in the code in full context?

defmodule PyramidCalculator do
    @moduledoc """
    Calculates a price percents adding up to 100 that form a pyramid shape
    based on the relative distance of each price point from the initial price point.

    If initial percent is greater then number of prices / 100, an inverted pyramid is returned.

    ## IMPORTANT
    The first price in the price list must be the highest or lowest price in the list.
    List is returned unsorted.  Sort list ascending or descending order before invoking the pyramid calculator.
    """

    @doc """
    Returns a list of percents for each price point that would look like a pyramid
    based on the relative distance of each price point from the initial price point.
    
    ## Examples:

        ## Normal pyramid example:
        iex> prices = [1, 2, 3]
        iex> initial_percent = 10
        iex> PyramidCalculator.calc_pyramid_prices_percents(prices, initial_percent)
        [10.0, 33.33333333333333, 56.666666666666664]

        ## Inverted pyramid example:
        iex> prices = [1, 2, 3]
        iex> initial_percent = 40
        iex> PyramidCalculator.calc_pyramid_prices_percents(prices, initial_percent)
        [40.0, 33.333333333333336, 26.666666666666668]

        ## Asymentrical pyramid price point distances example:
        iex> prices = [1, 3, 7, 22]
        iex> initial_percent = 5
        iex> PyramidCalculator.calc_pyramid_prices_percents(prices, initial_percent)
        [5.0, 10.517241379310345, 21.551724137931036, 62.93103448275862]

    """
    def calc_pyramid_prices_percents(prices, initial_percent) do

        total_column_percent = calc_total_column_percent(prices, initial_percent)
        total_pyramid_percent = calc_total_pyramid_percent(total_column_percent)
        [ initial_price | _ ] = prices
        total_distance = calc_total_distance(prices, initial_price)

        Enum.map(prices, fn(price) -> calc_price_percent(price, initial_price, \
            total_distance, initial_percent, total_pyramid_percent) end)
    end

    @doc """
    Calculates the maximum the initial percent can be so that the final
    price point percent in the prices list is zero.

    ## Examples:

        iex> prices = [1, 2, 3]
        iex> PyramidCalculator.calc_max_initial_percent(prices)
        66.66666666666666

        iex> prices = [5, 10, 15, 20]
        iex> PyramidCalculator.calc_max_initial_percent(prices)
        50.0
    """
    def calc_max_initial_percent(prices) do
        inverted_prices = Enum.reverse(prices)
        [d] = Enum.take((calc_pyramid_prices_percents(inverted_prices, 0)), -1) # get last price percent.
        d # Return only the number.
    end

    @doc """
    Calculates a perfect column-shaped price percents based on number of price points.

    ## Examples:

        iex> prices = [1, 2, 3]
        iex> PyramidCalculator.calc_column_percent(prices)
        33.333333333333336

        iex> prices = [5, 11, 25, 44]
        iex> PyramidCalculator.calc_column_percent(prices)
        25.0
    """
    def calc_column_percent(prices), do: 100 / Enum.count(prices)

    defp calc_total_column_percent(prices, initial_percent), do: Enum.count(prices) * initial_percent
    
    # In case of an inverted pyramid, returns a negative total.
    # Which means pyramid percent will be subtracted from the
    # total column percent rather then added, on the final 
    # price percent calculation.
    defp calc_total_pyramid_percent(total_column_percent), do: 100 - total_column_percent
    
    defp calc_price_percent(price, initial_price, total_distance, \
        initial_percent, total_pyramid_percent) do
     
        ratio = calc_price_pyramid_ratio(initial_price, price, total_distance)  
        price_pyramid_percent = calc_price_pyramid_percent(ratio, total_pyramid_percent)

        initial_percent + price_pyramid_percent
    end

    defp calc_price_pyramid_ratio(initial_price, price, total_distance),  \
        do: abs(price - initial_price) / total_distance

    defp calc_price_pyramid_percent(ratio, total_pyramid_percent), do: ratio * total_pyramid_percent

    defp calc_total_distance(prices, initial_price), do: _sum_distance(prices, initial_price, 0)
    
    defp _sum_distance([], _initial_price, total_distance), do: total_distance
        
    defp _sum_distance( [ price_head | price_tail ], initial_price, total_distance) do
        _sum_distance(price_tail, initial_price, (total_distance + abs(price_head - initial_price)))
    end
end

First 10 of 13 Posts Switch mode

yurko

yurko

I don’t see any red flags but here are couple refactor suggestions: total_column_percent seems to be a temporary variable that can be replaced by pipe:

total_pyramid_percent = prices 
  |> calc_total_column_percent(initial_percent)
  |> total_pyramid_percent = calc_total_pyramid_percent()

similar issue with initial_price sorry missed it in the final function, this one is needed

total_distance = calc_total_distance(prices, h(prices))

and then I might move the map logic into another smaller function.

These changes are not critical but might reduce the “noice” and make things a bit simpler.

bbense

bbense

You’ve written a perfectly straightforward translation of a typical procedural language module into Elixir. So as far as that goes I can’t find much to “fix” in the module.

However, there’s a kind of larger problem at the architectural level. The way that I find most useful to design Elixir programs is to think of data and the transformations required on that data. What data needs to be “together” to flow through the program? Where can I cleanly split the data groupings into smaller data groups with their own local transformations?

One of the outcomes of thinking this way is to have modules with a specified purpose and function signatures that follow the pattern of

def transformation( thing_to_transform, data_needed_to_specify_the_transform) 

Function signatures ( or arities ) much higher than 3 or 4 or lots of foo/3, foo/4 functions in a module are a warning sign that you might need to re-think how you’re constructing your code.[1] Without the larger context of the application which uses your module, it’s hard to say for sure, but to me it really feels like there is an abstraction one level above that is needed to clean up this code. Or it could just be me overthinking it, with too much coffee and too much time waiting for S3 to sync.

This is a great post about design in Elixir.

Latest Erlangist Blog Post

One minor last point, you don’t need to start private functions with an underscore. Since
the underscore has special meaning on variable names, I’d encourage you not to use
it elsewhere.

[1]- Or it might mean you’re writing some truly generic functionality like GenServer.

delameko

delameko

Although you probably could use recursion for this function, I think you’d probably have to move some of the other code around to make the most it. A couple of small tweaks:

def pyramid_price_percents([initial_price|prices], initial_percent) do
  total_pyramid_percent =
    prices
    |> calc_total_column_percent(initial_percent)
    |> calc_total_pyramid_percent
  total_distance = calc_total_distance(prices, initial_price)

  Enum.map(prices, fn(price) -> calc_price_percent(price, initial_price, \
    total_distance, initial_percent, total_pyramid_percent) end)
end

Personally, I’d turn this in to a pipe, something like:

def calc_max_initial_percent(prices) do
  prices
  |> Enum.reverse
  |> calc_pyramid_prices_percents(0)
  |> List.last
end

It’s just easier to read.

Emily

Emily OP

Thanks for all the input. I have refactored the module accordingly.

…still wrapping my head around bbense post, and how I would architect this with 1-3 arity on the last steps.

Seems I would have to break up the process into several more parts. My concern is that would add processing overhead,.

I’m finding to save processing time I tend to compute those common variables upfront, then apply a map. I’m not sure if this is the way to do things in Elixir, or if recursion style code is preferred.

globalkeith

globalkeith

I would lose all the calc_ prefixes wherever possible. For me, at least, it makes the code harder to read, with no significant benefit. :smiley:

OvermindDL1

OvermindDL1

Eh, I kind of like those kind of prefixes myself, they are descriptive…

DianaOlympos

DianaOlympos

well there is an easy way to check that : Try and benchmark :slight_smile:
in general, it should not matter too much.

There are quite a bit of time where i would have looked at the problem a bit differently. I agree with @delameko post on top to try to clean it a bit.

About recursion : in general in Elixir and in programming languages close to the functional style, recursion is a good way to go. At least to learn or when things get complex. On the long road, people tend to move toward transformations like the one you can find on the Enum module to get a bit of a higher level of abstraction. But recursion is completely good and how most things works under the hood.

globalkeith

globalkeith

As mentioned, its a personal preference. I find the general abstraction of “PyramidCalculator” gives enough context that the thing is a calculator…

globalkeith

globalkeith

Ok, basically everything @bbense already said, but felt it would be helpful
to provide an example, seeing as many people have done the same for me in the past. I had a play around with your code and came up with an alternative version; might give you some ideas; I don’t claim its better, but for me, the reason I prefer it is that I find it easier to follow;

JEG2

JEG2

Author of Designing Elixir Systems with OTP

I just felt compelled to point out that this is a life long mission. It’s often a worthy road to travel, but don’t be frustrated if you never arrive. The journey’s the important bit.

Also, there are sometimes good reasons to sacrifice clean code. Try not to get to hung up on the path to purity.

:slight_smile:

Where Next? Top

Trending in Questions Top

stjefim
Hello! Suppose you are building workflow (order / task / payment) processing system with the following requirements: Each workflow con...
New
jonnycharles
I’m in search of an Elixir library that offers PDF generation capabilities similar to Ruby’s Prawn. While there have been discussions abo...
New
spammy
I’m looking to build a personal workflow to quickly deploy web applications written in elixir/phoenix, for local consumption (ie not on t...
New
dli
Before I dive in myself, did anyone successfully sprinkle Hologram into their existing LiveView app? Looking for hints regarding: Addi...
New
bottlenecked
Hi all, I wanted to ask how the community is dealing with post-release steps. Today we have Ecto migrations, which make sure that the db...
New
roeland
Kia ora, We have been using elixir-google-api to connect to Google Drive. However, with the updates to Tesla due to CVEs this is now bro...
New
rahultumpala
Hello, I have an Elixir backend that implements a custom protocol over TCP. I want to load test the backend and assess the performance o...
New

Other Trending Topics Top

JesseHerrick
Hey, I’m Jesse and I’m the main contributor behind Dexter, a full-featured, lightning-fast Elixir LSP optimized for large codebases. It s...
New
jimsynz
Beam Bots (or just BB for short) is a framework for building fault-tolerant robotics applications in Elixir using familiar OTP patterns. ...
New
Damirados
Hello everyone. After busy few months I am happy to announce v0.1.0 of Emerge & Solve. They are GUI (Emerge) and State management (S...
New
netoum
Corex is an accessible, unstyled UI component library for Phoenix that integrates Zag.js state machines using Vanilla JavaScript and Live...
New
ausimian
Emily is an Elixir library that runs Nx computations on Apple’s MLX. Install it as the default Nx backend and Nx, defn, Axon, Nx.Serving,...
New
juhalehtonen
There has been a thread to discuss the Stack Overflow Developer Survey on this forum every year since 2018, so here’s yet another one for...
New

We're in Beta

About us Mission Statement