On 18 Aug 2019, at 18:23, Roelof Wobben <r.wobben@home.nl> wrote:Thanks,
I implement almost everything now.
But I fail to see how to make for password a private variable without getters and setters
and still have different password for every bankaccount and can test against it.
So please help.
Roelof
Op 17-8-2019 om 12:48 schreef Roelof Wobben:
Op 17-8-2019 om 12:33 schreef Richard O'Keefe:
Good-oh.Looking at Bankaccounts- the name should be singular, not plural- Smalltalk uses baStudlyCaps> Call it BankAccount.
- you not only provide a getter for thepassword, but a setter! This means thatmalevolent/broken code can doaBankAccount password: 'PWNED!'.and then supply the new password 'PWNED!'whenever a password is needed.> The password must be supplied when aBankAccount is changed and it should notbe settable afterwards (except perhaps witha master key).
Thanks all changed.
- #addToBankAccount: is an unhelpful name; weknow it's adding to a BankAccount because thatis where it is. But what is it adding?(Hint: an account might have more than onecustomer in the real world, so we might needto distinguish between #addCustomer: and)> #addTransaction: , which would be a better name.
the method adds a bankaccount to the bankAccounts collection so maybe AddBankAccountToBankAccountsOfCustomer ?
- There is a missing method. When you ask a bankaccount to report a secret, *it* should checkthe password.> addqueryBalance: pwd
^password = pwd
ifTrue: [transactions detectSum: [:each | each amount]]
ifFalse: [self error: 'wrong password']
I know its missing. That is the part I still have to figure out.How can I make a collection of all transactions split by account and then the total of it.
Transactions- This too should be singular, not plural.> Rename it to Transaction.
- You know it is unfinished.
Changed the name and what is missing here then.
Customer- You know it is unfinished.
What is then missing here.
Done and Done.- Smalltalk uses baStudlyCaps style. The instancevariable 'bankaccounts' is two words run together.> Change it to 'bankAccounts'.
- The very first method has two style issues and a major flaw.AddBankAccountToCustomer: bankaccount [bankaccounts add: bankaccounts]- Style flaw one: method names are expected to beginwith a lower case letter, not a capital. (That's C#.)- Style flaw two; we *knew* it's adding to a customer,because that's where it is.> Rename to #addBankAccount:
- The major flaw is that the argument 'bankaccount' isignored, while the 'bankaccounts' collection is addedTO ITSELF. This is one of the rare occasions when atype checker would have caught a mistake.> Revise toaddBankAccount:aBankAccount [bankAccounts addLast: aBankAccount.]
- You have a setter, #bankaccounts:. WHY? This should bea completely private variable, initialised when theCustomer object is created, and never reassigned thereafter.> Delete that setter.
Customer >> AddBankAccountToCustomer: bankaccount [ bankaccounts add: bankaccounts. ]
On Sat, 17 Aug 2019 at 21:59, Roelof Wobben <r.wobben@home.nl> wrote:
Sorry, then a pointed to a wrong repo
this is the repo with smalltalk code :
https://github.com/rwobben/banking
Roelof
Op 17-8-2019 om 11:25 schreef Richard O'Keefe:
points to a repository with C# code, so I cannot comment on the Smalltalk.There is, however, a striking thing about the C# code, qua C#, that isworth mentioning.
It is not encapsulated.
Let's start with the most obvious one.A bank account has a password, and you aresupposed to provide the right password in orderto get information from it.
Your interface *reveals* the password, somalicious code can dovar pass = theBankAccount.password;and then provide the password back. What'sneeded is something like
var balance = theBankAccount.queryBalance(pass);
where passwords go *in* to theBankAccount but never everever come *out*.
Another thing is that a BankAccount belongs to a specificCustomer. There's a good way and a bad way to manage this.The bad way is for the caller to create a new account thatbelongs to nobody, and then *tell* the Customer "here is anew account". With this interface, you could tell anynumber of Customers that they own the account. That'spossible in the real world, but in the challenge, it isn't.The good way is to *ask* the Customer "please create a newaccount and tell me what it is", and have no way to forcea Customer to accept an account willy nilly. That way aCustomer can be certain that the accounts it is holdingbelong to it and it alone.
Or it could be were it not for a major encapsulation issue.The idea of encapsulation is thatEVERY CHANGE TO AN OBJECT'S STATE MUST BE MADE BY THAT OBJECTincluding the start of owned containers.
In Java, for example, you might havepublic class Customer {private final ArrayList<Account> accounts = new ArrayList<Account>();...public List<Account> getAccounts() {return Collections.unmodifiableList(accounts);}...public Account newAccount(...details...) {final Account a = new Account(...details...');accounts.add(a);return a;}...
}If you are going to provide access to a collection,(1) make the collection immutable in the first place, or(2) return an immutable view of it, or(3) best of all, DON'T provide access to the collectionbut provide queries that extract just the information youwant to make available.
This has nothing to do with Java or Smalltalk or C# as such;encapsulation is object-oriented-programming 102.
For what it's worth, I used to be a University lecturer, andit was MUCH harder for students to grasp encapsulation thanto grasp recursion. I have no idea why. Just pretend that youare a nice little object surrounded by malevolent pranksterswho can force you to do anything in your interface, and makesure they cannot harm you.
Oh, when I asked about the source of the challenge, what I wasgetting at was "is this your paraphrase of the challenge, inwhich case please tell us to find the original, or is thisthe actual unmodified original text, in which case --ing h---,you have my profound sympathy."
This looks like a garbled version of an exercise that I dida little over 10 years ago, which I found in a Java-based bookwhose details I unfortunately forgot to record. Right down tothe names of the classes! That is why I am pretty sure thatthere is a description of the problem somewhere that makes sense.