Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add elixir version (without tests) #47

Merged
merged 4 commits into from Jan 8, 2024
Merged

Conversation

kerryb
Copy link
Contributor

@kerryb kerryb commented Dec 19, 2023

See PR #46 for version with tests.

@codecop
Copy link
Contributor

codecop commented Dec 20, 2023

Should the _build folder be part of the commit?

@codecop
Copy link
Contributor

codecop commented Dec 20, 2023

The readme is very comprehensive. I am not sure we need that information after the pr has been merged. Most translation only have a readme for how to build and run tests. So please put the porting related information into the PR descriptions and keep the readme short, only listening what is needed, if at all, like mix command. The main branch should also not write about testing aspects.

@codecop
Copy link
Contributor

codecop commented Dec 20, 2023

Also the gitignore seems to be missing from this PR.

@kerryb
Copy link
Contributor Author

kerryb commented Dec 20, 2023

Should the _build folder be part of the commit?

Oops, no, definitely not! Looks like I lost the.gitignore while copying between branches.

@kerryb
Copy link
Contributor Author

kerryb commented Dec 20, 2023

Above comments addressed.

discount = nil
x = 1

{discount, x} =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I do not know much about Elexir.... Is the cond expression strictly necessary here? In the other languages this is the first step people have to consolidate the code which is more imperative and having different updates in the branches.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that in Elixir variables are immutable (but can be rebound, and shadowed in nested scopes). So if we reproduced the shape of the java code along these lines (there’s no elseif, but cond is similar):

x = 1
cond do
  offer.offer_type == :three_for_two) ->
    x = 3
  offer.offer_type == :two_for_amount ->
    x = 2
    # etc
end

… then x would always end up with the value of 1, because the x = 3 and x = 2 are creating separate variables in their own block scopes. The only way to set a variable based on a conditional is to assign the result of the conditional to the variable.

@codecop
Copy link
Contributor

codecop commented Jan 4, 2024

Thank you looks much better now. I had a closer look at the code and have a question regarding one construct. Please see comment in code shopping_cart.ex

@codecop
Copy link
Contributor

codecop commented Jan 6, 2024

Thank you. Looks good then. This could be merged @emilybache

@emilybache emilybache merged commit ecefce3 into emilybache:main Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants