[WIP] Implement exercise bank-account #757

thecouchcoder · 2017-10-07T01:59:30Z

Resolves #730

Closes #1260 (mutual-exclusion)

…nd adding a deposit

ilya-khadykin · 2017-10-11T18:40:30Z

config.json

+      "unlocked_by": null,
+      "difficulty": 4,
+      "topics": [
+        "Classes",


We've switched to snake_case naming convention for topics to increase consistency among tracks, so could you please choose them from the following list - https://github.com/exercism/problem-specifications/blob/master/TOPICS.txt

ilya-khadykin

Good work!

I left some comments, please take a look

ilya-khadykin · 2017-10-11T18:48:07Z

exercises/bank-account/example.py

+
+    def getBalance(self):
+        self.lock.acquire()
+        if (self.open is True):


Probably just:

if self.open: # do stuff

ilya-khadykin · 2017-10-11T18:51:31Z

exercises/bank-account/example.py

+            return self.balance
+        else:
+            self.lock.release()
+            raise Exception


Could you please throw a narrower Exception, or at least state the root cause by:

raise Exception('Describe what went wrong')

Perhaps ValueError would be better?

ilya-khadykin · 2017-10-11T18:54:00Z

exercises/bank-account/example.py

+
+class BankAccount(object):
+    def __init__(self):
+        self.lock = threading.Lock()


I think it would be a good idea to initialize open flag in constructor as well:

self.open = False

thecouchcoder · 2017-10-11T20:26:38Z

Thank you @m-a-ge I will get on these as soon as I have a chance :)

cmccandless

@aes421 Thanks for your PR! I've left some comments, please read them.

cmccandless · 2017-10-23T17:38:01Z

exercises/bank-account/bank_account_test.py

+        self.account.open()
+        self.account.close()
+
+        with self.assertRaises(Exception):


I would suggest something more specific than Exception; perhaps ValueError?

cmccandless · 2017-10-23T17:40:24Z

exercises/bank-account/example.py

+            return self.balance
+        else:
+            self.lock.release()
+            raise Exception


Perhaps ValueError would be better?

N-Parsons · 2017-11-14T23:58:53Z

Hi @aes421, are you still interesting in working on this?

thecouchcoder · 2017-11-16T20:35:33Z

@N-Parsons Life got in the way of coding for a few weeks. About to work on some of these fixes right now!

thecouchcoder · 2017-11-16T21:26:54Z

@cmccandless Is there anything else I need to do?

cmccandless · 2017-11-16T21:38:19Z

@aes421 Not as far as I can tell, but since @m-a-ge previously had comments on this, I'd like to give him the chance to comment again.

N-Parsons · 2017-11-17T11:16:56Z

exercises/bank-account/README.md

+
+It should be possible to close an account; operations against a closed account must fail.
+
+### Submitting Exercises


This should be at heading level 2 (##).

N-Parsons · 2017-11-17T11:26:18Z

exercises/bank-account/bank_account.py

+    def __init__(self):
+        pass
+
+    def getBalance(self):


Following PEP8 we use snake_case for function and method names, so this should be get_balance.

N-Parsons · 2017-11-17T11:32:29Z

exercises/bank-account/bank_account_test.py

+            thread = threading.Thread(target=increment_and_decrement,
+                                      args=(self, ))
+            threadlist.append(thread)
+            i += 1


This loop would be better implemented using for _ in range(100):.

N-Parsons

There are a couple of minor issues, but I think there's a very significant issue regarding concurrency.

N-Parsons · 2017-11-17T12:37:45Z

exercises/bank-account/bank_account_test.py

+        with self.assertRaises(ValueError):
+            self.account.deposit(-50)
+
+    def test_can_handle_concurrent_transactions(self):


@aes421 (cc: @cmccandless, @m-a-ge)

I'm not sure that this test is truly testing for concurrency - depositing and then immediately withdrawing inherently safe values within one function is never going to break as far as I can see. Further, I can remove all instances of self.lock from the example code and the tests still pass. Part of the issue is probably that the Global Interpreter Lock (GIL) already locks the BankAccount instance when a thread accesses it, and releases it once the thread ends. I'm not sure of the best way to get around this.

Currently, Python seems to be running these threads in order, depositing 10 and withdrawing 1, then moving on to the next thread. We can see this by storing a list of transaction values and using i+1 as a second argument for the increment_and_decrement function, depositing and withdrawing this value, and then printing the transaction list after all threads are done.

One possible solution to get disorder seems to be to add time.sleep(random.random()) before deposits or withdrawals, but I think the deposits and withdrawal should be decoupled and not part of the same function call - just joining a list of threads for each should be sufficient if there are random delays.

Concurrency was discussed a little in #1106. If we want to keep concurrency in this exercise (I think the exercise would be ok without it, but only as an early exercise about classes), I think that some consideration needs to be made for how to make the tests only pass if it is safe concurrent implementation.

Ultimately, for concurrency to make sense here, we need to be able to make it so that the concurrency matters in some way.

@N-Parsons (cc: @aes421, @m-a-ge)

I think the exercise would be ok without it

I disagree. It's a core part of the exercise. From the problem description:

Create an account that can be accessed from multiple threads/processes (terminology depends on your programming language).

If we marked another exercise as foregone simply because of the challenges of testing parallelism in Python, then I think we might need to consider foregoing this exercise as well. Either that, or we need to come up with another solution and apply it both to this exercise and the foregone parallel-letter-frequency.

If we want to be pedantic with this exercise, we may be able to do something similar to NumPy and write an extension in C that runs outside of the GIL. However we can only safely do that if we use no Python C API functions.

I also want to quickly put a note here because I don't think concurrency and parallelism have been discussed much in this repo. There is a difference between concurrency and parallelism.

"In programming, concurrency is the composition of independently executing processes, while parallelism is the simultaneous execution of (possibly related) computations." - from golang.org

https://vimeo.com/49718712 (Video of Rob Pike talking about the difference at Heroku)

We also might be able to use this:

https://docs.python.org/2/library/multiprocessing.html

stale · 2017-12-08T16:42:17Z

This issue has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

cmccandless · 2017-12-08T21:44:30Z

Pinning this (pending further discussion on concurrency).

ilya-khadykin changed the title ~~Implement exercise bank-account~~ [WIP] Implement exercise bank-account Oct 7, 2017

thecouchcoder force-pushed the bank-account branch 3 times, most recently from 1998c23 to dc4c1f5 Compare October 9, 2017 22:41

thecouchcoder added 8 commits October 9, 2017 17:51

bank-account: Implemented the most basic tests - opening an account a…

6bf3092

…nd adding a deposit

bank-account: Implement and test getting balance

5a41971

bank-account: implement and test withdrawing.

9c5a2de

bank-account: implement and test actions against closed accounts

97e823a

implement and test invalid transactions

fd061cb

implement and test multithreading

8335b9e

bank-account: exercise configuration

a80f196

bank-account: Formatting

34d7f5a

thecouchcoder force-pushed the bank-account branch from dc4c1f5 to 34d7f5a Compare October 9, 2017 23:07

ilya-khadykin added the please review label Oct 10, 2017

ilya-khadykin reviewed Oct 11, 2017

View reviewed changes

cmccandless suggested changes Oct 23, 2017

View reviewed changes

thecouchcoder added 2 commits November 16, 2017 14:40

bank-account: a few fixes

4550401

bank-account: fixes

a1b8599

cmccandless approved these changes Nov 16, 2017

View reviewed changes

cmccandless requested a review from ilya-khadykin November 16, 2017 21:37

N-Parsons reviewed Nov 17, 2017

View reviewed changes

N-Parsons suggested changes Nov 17, 2017

View reviewed changes

stale bot added the abandoned label Dec 8, 2017

cmccandless added the pinned label Dec 8, 2017

stale bot removed the abandoned label Dec 8, 2017

cmccandless added concurrency and removed please review labels Jan 9, 2018

mrcfps mentioned this pull request Jan 29, 2018

Implement exercise bank-account #1176

Closed

cmccandless mentioned this pull request Feb 15, 2018

Should we forgo any missing exercises #417

Closed

mrcfps mentioned this pull request Feb 15, 2018

Implement exercise bank-account #1260

Merged

cmccandless closed this in #1260 Apr 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Implement exercise bank-account #757

[WIP] Implement exercise bank-account #757

thecouchcoder commented Oct 7, 2017 •

edited by cmccandless

Loading

ilya-khadykin Oct 11, 2017

ilya-khadykin left a comment

ilya-khadykin Oct 11, 2017

ilya-khadykin Oct 11, 2017 •

edited

Loading

cmccandless Oct 23, 2017

ilya-khadykin Oct 11, 2017

thecouchcoder commented Oct 11, 2017

cmccandless left a comment •

edited

Loading

cmccandless Oct 23, 2017

cmccandless Oct 23, 2017

N-Parsons commented Nov 14, 2017

thecouchcoder commented Nov 16, 2017

thecouchcoder commented Nov 16, 2017

cmccandless commented Nov 16, 2017

N-Parsons Nov 17, 2017

N-Parsons Nov 17, 2017

N-Parsons Nov 17, 2017 •

edited

Loading

N-Parsons left a comment

N-Parsons Nov 17, 2017

cmccandless Nov 17, 2017

Dog Mar 1, 2018 •

edited

Loading

Dog Mar 1, 2018 •

edited

Loading

stale bot commented Dec 8, 2017

cmccandless commented Dec 8, 2017


		It should be possible to close an account; operations against a closed account must fail.

		### Submitting Exercises

[WIP] Implement exercise bank-account #757

[WIP] Implement exercise bank-account #757

Conversation

thecouchcoder commented Oct 7, 2017 • edited by cmccandless Loading

Choose a reason for hiding this comment

ilya-khadykin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilya-khadykin Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thecouchcoder commented Oct 11, 2017

cmccandless left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-Parsons commented Nov 14, 2017

thecouchcoder commented Nov 16, 2017

thecouchcoder commented Nov 16, 2017

cmccandless commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-Parsons Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

N-Parsons left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dog Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Dog Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

stale bot commented Dec 8, 2017

cmccandless commented Dec 8, 2017

thecouchcoder commented Oct 7, 2017 •

edited by cmccandless

Loading

ilya-khadykin Oct 11, 2017 •

edited

Loading

cmccandless left a comment •

edited

Loading

N-Parsons Nov 17, 2017 •

edited

Loading

Dog Mar 1, 2018 •

edited

Loading

Dog Mar 1, 2018 •

edited

Loading