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 new practice exercise bank-account #708

Merged
merged 4 commits into from
Dec 11, 2024
Merged

Add new practice exercise bank-account #708

merged 4 commits into from
Dec 11, 2024

Conversation

jiegillet
Copy link
Contributor

This is week 41 of 48in24.

This exercise is not straightforward, there are different ways it could be approached. Here are some notes:

  • It is suggested that it's a good one for testing concurrency, but Elm can't do that on Exercism (since we need Task and a way to test them) so I did not implement the last test related to concurrency.
  • In some languages, a stateful approach could be taken (a bank account is created and can be accessed by different clients), but we can't do that in pure Elm either.
  • The way I would write this API in real life would be different, I would consider phantom types to only allow operations on accounts that are open. The problem with this approach is that it would get rid of a lot of tests that check for errors, and there are no way of looking at types with tests. We could do it with the analyzer, but that would be pretty unorthodox, and would require additional instructions. Doable though.
  • There were two tests that checked that an account not opened could not be closed, but that just doesn't make sense in a typed language so I did not implement them.
  • The API for the open function is quite unnatural, I wrote it this way to preserve as many test cases as possible and keep the spirit of the exercise as much as possible.
  • I'm very happy that this exercise is suitable to be our first one to practice the opaque-types concept, since we don't even need to export the type.

I'm happy to discuss or revisit all of these decisions. The code is very simple, but the API is not.

@mpizenberg
Copy link
Member

I just had a quick look and didn't find anything shocking with the code. But I don't know much of the context and objectives of the exercise regarding concurrency.

@jiegillet
Copy link
Contributor Author

The code is fine, it's quite simple, it's the API (function signatures) that could be built differently.
The canonical data is a bit vague with it's "operations".
The last test is the one talking about concurrency.

@jiegillet
Copy link
Contributor Author

@mpizenberg @ceddlyburge
I know that my description was long, but really if you don't have concerns, I'm fine with the exercise as it is. In a nutshell:

  • we don't have concurrency, but that's not a requirement, so it's not an issue
  • the API could be different, but this version allowed to keep the most exercises

@ceddlyburge
Copy link
Contributor

Hi @jiegillet , I left this to matthieu as he got their first, but if he doesn't get back to you in a few days I'll take a look

@jiegillet
Copy link
Contributor Author

We missed the 48in24 deadline, so this is not in any sort of rush at the moment, but @ceddlyburge if you have the time for a quick review, that would be nice.

Copy link
Contributor

@ceddlyburge ceddlyburge left a comment

Choose a reason for hiding this comment

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

I see what you mean about open being unnatural. I think it needs a comment or a mention in the instructions or something like that, otherwise we are teaching students bad practices.
Or we could split from the mainstream problem specification and create the problem in a functional programming way.
And it doesn't practice any race condition / parrallel execution stuff, so that part of the instructions is misleading.

Out of interest, is there data showing how many elm exercises are submitted as part of 48 in 24?

| DepositAmountNegative


open : Maybe BankAccount -> Result Error BankAccount
Copy link
Contributor

Choose a reason for hiding this comment

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

This is weird ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

@jiegillet
Copy link
Contributor Author

Out of interest, is there data showing how many elm exercises are submitted as part of 48 in 24?

I don't know, maybe we should ask about that on the Forum, I'm sure other tracks would also be interested in having a breakdown of exercises submitted. It might already exist.

Your task is to implement bank accounts supporting opening/closing, withdrawals, and deposits of money.

As bank accounts can be accessed in many different ways (internet, mobile phones, automatic charges), your bank software must allow accounts to be safely accessed from multiple threads/processes (terminology depends on your programming language) in parallel.
For example, there may be many deposits and withdrawals occurring in parallel; you need to ensure there are no [race conditions][wikipedia] between when you read the account balance and set the new balance.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ceddlyburge reading the instructions more closely, this whole exercise is really all about concurrency, for which Elm is poorly suited. This is the source of the unnatural API.

I don't think it's worth adapting to something Elm is suited for, it's not like the exercise has particularly interesting concepts outside of that (well, with the exception of being suited for having an opaque type).

Then instead, since this exercise is ill-suited for Elm, I propose to add it as well as parallel-letter-frequency to the list of "foregone" exercises, those are exercises that we choose not to implement. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think that this exercise is about practicing concurrency, which is barely a thing in Elm, and 100% not something we can do within exercism, so we might as well leave it out. And the exercise is not particularly interesting, as you say ...

@jiegillet jiegillet merged commit 3aaaf12 into main Dec 11, 2024
7 checks passed
@jiegillet jiegillet deleted the jie-bank-account branch December 11, 2024 09:37
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.

3 participants