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 the
password, but a setter! This means that
malevolent/broken code can do
aBankAccount password: 'PWNED!'.
and then supply the new password 'PWNED!'
whenever a password is needed.
> The password must be supplied when a
BankAccount is changed and it should not
be settable afterwards (except perhaps with
a master key).
- #addToBankAccount: is an unhelpful name; we
know it's adding to a BankAccount because that
is where it is. But what is it adding?
(Hint: an account might have more than one
customer in the real world, so we might need
to distinguish between #addCustomer: and)
> #addTransaction: , which would be a better name.
- There is a missing method. When you ask a bank
account to report a secret, *it* should check
the password.
> add
queryBalance: pwd
^password = pwd
ifTrue: [transactions detectSum: [:each | each amount]]
ifFalse: [self error: 'wrong password']
Transactions
- This too should be singular, not plural.
> Rename it to Transaction.
- You know it is unfinished.
Customer
- You know it is unfinished.
- Smalltalk uses baStudlyCaps style. The instance
variable '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 begin
with 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' is
ignored, while the 'bankaccounts' collection is added
TO ITSELF. This is one of the rare occasions when a
type checker would have caught a mistake.
> Revise to
addBankAccount:aBankAccount [
bankAccounts addLast: aBankAccount.
]
- You have a setter, #bankaccounts:. WHY? This should be
a completely private variable, initialised when the
Customer 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.wob...@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:
>
> https://github.com/rwobben/bankaccount
> 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 is
> worth mentioning.
>
> It is not encapsulated.
>
> Let's start with the most obvious one.
> A bank account has a password, and you are
> supposed to provide the right password in order
> to get information from it.
>
> Your interface *reveals* the password, so
> malicious code can do
> var pass = theBankAccount.password;
> and then provide the password back. What's
> needed is something like
>
> var balance = theBankAccount.queryBalance(pass);
>
> where passwords go *in* to theBankAccount but never ever
> ever come *out*.
>
> Another thing is that a BankAccount belongs to a specific
> Customer. There's a good way and a bad way to manage this.
> The bad way is for the caller to create a new account that
> belongs to nobody, and then *tell* the Customer "here is a
> new account". With this interface, you could tell any
> number of Customers that they own the account. That's
> possible in the real world, but in the challenge, it isn't.
> The good way is to *ask* the Customer "please create a new
> account and tell me what it is", and have no way to force
> a Customer to accept an account willy nilly. That way a
> Customer can be certain that the accounts it is holding
> belong to it and it alone.
>
> Or it could be were it not for a major encapsulation issue.
> The idea of encapsulation is that
> EVERY CHANGE TO AN OBJECT'S STATE MUST BE MADE BY THAT OBJECT
> including the start of owned containers.
>
> In Java, for example, you might have
> public 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 collection
> but provide queries that extract just the information you
> want 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, and
> it was MUCH harder for students to grasp encapsulation than
> to grasp recursion. I have no idea why. Just pretend that you
> are a nice little object surrounded by malevolent pranksters
> who can force you to do anything in your interface, and make
> sure they cannot harm you.
>
> Oh, when I asked about the source of the challenge, what I was
> getting at was "is this your paraphrase of the challenge, in
> which case please tell us to find the original, or is this
> the 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 did
> a little over 10 years ago, which I found in a Java-based book
> whose details I unfortunately forgot to record. Right down to
> the names of the classes! That is why I am pretty sure that
> there is a description of the problem somewhere that makes sense.
>
>
>
