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

Cut down on OOP fluff #272

Closed
orcinus opened this issue Sep 9, 2016 · 3 comments
Closed

Cut down on OOP fluff #272

orcinus opened this issue Sep 9, 2016 · 3 comments
Labels
V2 V2 Client

Comments

@orcinus
Copy link

orcinus commented Sep 9, 2016

The lib seems to have recently gone from sane levels of object-orientedness, to Java insanity levels. NOT EVERYTHING has to be an object. Cut down on superfluous crap, you're making it user unfriendly with no clear benefit to library maintenance. Not to mention your naming scheme is atrocious.

Consider this, for example:

$account->account_balance->get()->balance_in_cents->getCurrency("USD")->amount_in_cents;

Does that look like a sane way to get the account balance in cents?
It's an utter mess. Why isn't there a sane default for the currency? Why is the currency an object? There's absolutely no need for it to be, most cases will use a single currency throughout, why isn't it a state with a setter? Why does your account object have account in its property names? Why does "balance_in_cents" return a currency object that might not even have cents? Good grief.

@drewish
Copy link

drewish commented Sep 9, 2016

Sorry you found it frustrating. It's definitely not the interface I'd design with a clean slate but it was designed to reflect the underlying XML of the API. The names for account_balance, balance_in_cents and amount_in_cents all reflect the underlying element names. It definitely allows you construct some pretty long function call chains.

In this case the currency is an object because we support multiple currencies and while you might not use it many merchants do.

@orcinus
Copy link
Author

orcinus commented Sep 9, 2016

That's not the point, though.

First of all, there is no need for it to be an object. Second, a getter can have a default, and you can have a setter (chainable) for selection of a different currency, with the object storing the current selection as an internal state.

Second of all, why do you need the get method at all in this case? There's no shame in using magic methods to detect access to a property and automating these things. Finally, why have balance_in_cents have a getter that then gets a currency object, that then has a property. Needless overhead. There's no shame in having proxy methods either.

For most merchants, the call could simply be:

$account->account_balance->amount_in_cents();

For merchants requiring a different currency than the default, or multiple currencies, it could be:

$account->account_balance->amount_in_cents("EUR");

I understand that you're trying to keep it mapped 1:1 to the underlying XML responses and all the components as object-oriented and decoupled as possible, but there's no real need for that, especially for properties that are of an "end-of-the-line" type and there's no expectation of them growing or branching further.

Sorry if i sound frustrated and overly angry, but after a day of dealing with other assorted crap and then doing 5 var_dumps to assemble that monstrosity of a chain i just flipped.

@bhelx
Copy link
Contributor

bhelx commented Jan 19, 2017

@orcinus I think we all agree with the underlying points you are making and understand why it's frustrating. I'm of the same opinion as @drewish. If I got to design it from scratch it would be a lot different. Unfortunately, this library was designed in 2009 and hundreds of companies rely on it. We are committed to improving the library even at the cost of making API breaking changes (as long as they are properly versioned and communicated).

That being said, what from this discussion can we break down into separate issues that can be tackled? I'm willing to accept any PRs with the currency interface for instance. We can get them in a future branch that can be released on the next minor revision.

@bhelx bhelx closed this as completed Jan 19, 2017
@bhelx bhelx added the V2 V2 Client label Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
V2 V2 Client
Projects
None yet
Development

No branches or pull requests

3 participants