search

byrokrat / giroapp

Command line app for managing autogiro donations.
GNU General Public License v3.0
4 stars 2 forks source link

adding a pre-existing donor throws an error #54

Closed nonbinary closed 7 years ago

nonbinary commented 7 years ago

Solution to bug #51

nonbinary commented 7 years ago

This does not, however, feel optimal. Since the DonorMapper->findByKey throws an exception if the donor does not exist, and I'd like the AddCommand to throw an exception... I predict clashes. Maybe we should add a new function to DonorMapper, or change DonorMapper's exception to something more specific?

hanneskod commented 7 years ago

Yupp. Logic looks good. Will merge when I get home to my computer..

hanneskod commented 7 years ago

Yes I agree. DonorMapper::hasKey().

Feel free to add in this pr. It's better logic and more readable. So go for it!

nonbinary commented 7 years ago

Hm. Now I did donorMapper->hasKey(). Is DonorMapper::hasKey() preferrable?

hanneskod commented 7 years ago

No no that's what I mean. Sorry about the confusion. It's de-facto standard to write $object->method() when you are referencing a concrete object, but ClassName::method() when you mean a method in a class.. but I see how this can be confused with static access..

You can drop the else keyword now as it doesn't fill any logical value..

And if you have time the new DonorMapperSpec..

nonbinary commented 7 years ago

Am working on the DonorMapperSpec. Am having some trouble with boolean return values. But I'll get there.

nonbinary commented 7 years ago

Here's what I'd thought the test should look like. But it confuses the return values here.

hanneskod commented 7 years ago

Cool. Merging!