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

BankAccount: not scala-ish enough? #214

Closed
Trevoke opened this issue Nov 1, 2016 · 9 comments
Closed

BankAccount: not scala-ish enough? #214

Trevoke opened this issue Nov 1, 2016 · 9 comments
Labels

Comments

@Trevoke
Copy link
Contributor

Trevoke commented Nov 1, 2016

This is a very Java-ish test suite and the implementation follows. There are probably better ways to implement this in Scala... Should the test suite be updated?

@ricemery
Copy link
Member

ricemery commented Nov 1, 2016

What specifically is not Scala-ish? The exercise is testing an object with mutable state. How would you suggest this be done in a different manner?

I could see the exercise being written/solved using Actors. But, it seems like developers should know how to mutate state in a threaded environment without resorting to Actors.

Thanks!

@Trevoke
Copy link
Contributor Author

Trevoke commented Nov 2, 2016

Well, the option of not using mutable state, for instance -- or being able to return a new object every time.

Frankly I barely even registered the threads, since that need is contained to a very specific part of the system. I was referring to the behavior of open account / closed account, which, given the test suite, we can't really handle elegantly with type systems (or at least, I can't, because I've only been working in Scala for a couple of months). I went through this blog entry on types and couldn't find something that would be helpful here. Shadow types were almost helpful but didn't work.

@ricemery
Copy link
Member

ricemery commented Nov 3, 2016

Thanks @Trevoke

I agree that returning new immutable instances for each account operation is cleaner. But, I think the original intent of this exercise was to specifically force the implementer to deal with mutable state. At some point within a banking app mutable state must be dealt with..

I went and looked at the Haskell and Clojure examples for the same exercise. Both are also using mutable state within the sample solutions. So, I think the intent is right.

Looking at the scala example and tests, I do think that the fact that creating a new instance results in "opening" an account is odd. An openAccount function would probably make more sense.

I will be interested to see if anyone else chimes in on this issue report.

@ricemery
Copy link
Member

@abo64 , @ErikSchierboom

Gentlemen,

What do you think of this issue?

Thanks

@abo64
Copy link
Contributor

abo64 commented Nov 24, 2016

I also think that currently this exercise is about mutable state.
Although it might be an interesting idea to add another exercise for such an FP-style immutable bank account. Would be of great educational value.
Another idea could be to turn this into a little DSL: closeAccount returns an object that does not even contain an incrementBalance method. But again, this would IMO deviate too much from the original intentions.

But for this exercise we could indeed have a factory method BankAccount.openAccount.

@ErikSchierboom
Copy link
Member

I also feel that this exercise is about mutable state, and as such introduces people to the hybrid design of Scala. Doing a pure FP-style exercise would be nice, but I think the exercise is good the way it is at the moment. A factory method BankAccount.openAccount wouldn't hurt of course.

@ricemery
Copy link
Member

Thanks for the feedback. I'll put it on my todo list to add the BankAccount.openAccount function. But, otherwise leave the example as is.

ricemery added a commit to ricemery/xscala that referenced this issue Nov 30, 2016
ricemery added a commit to ricemery/xscala that referenced this issue Nov 30, 2016
@abo64
Copy link
Contributor

abo64 commented Dec 8, 2016

Can we close this issue?

@abo64 abo64 added the question label Dec 8, 2016
@ErikSchierboom
Copy link
Member

I believe so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants