search

Wizcorp / node-iap

In-app purchase validation for Apple, Google, Amazon, Roku
262 stars 92 forks source link

reduce boilerplate in main module methods #57

Closed adrian-gierakowski closed 6 years ago

adrian-gierakowski commented 6 years ago

following a to discussion in #55

since both verifyPayment and cancelSubscription in index.js were basically identical, their implementation has been abstracted into a helper function called makeMethod, which given a methods name, generates the body of the method and adds it to exports

The syncError helper has been renamed to asyncError and now accepts an error message instead of an Error object to reduce boilerplate when creating errors.

ronkorving commented 6 years ago

I can appreciate the drive to reduce duplicate code/patterns, but makeMethod seems a bit overkill for me. It also killed a line of code that we don't want to lose (see other comment). I would argue we should be a little bit less overarchitecting things.

"Don't be smart, be simple".

adrian-gierakowski commented 6 years ago

@ronkorving I've addressed the 3 issues mentioned in the inline comments

however I will try to convince you regarding makeMethod

the methodName argument is used in the same spirit as the platform, with the difference that methodName is pre-applied and platform is provided dynamically by the user of the lib.

the original verifyPayment function, apart from dispatching to the desired engine, does a couple of things, regardless of the engine:

  1. "validates" the input (payment)
  2. modifies the result (adds platform prop)

I'd say that's the main reason why this wrapper exists in the first place, so that what is common to all engines stays in one place and is not repeated across the codebase. Now, initially there was only one method in each engine, so the method name (verifyPayment) was hardcoded. But since a second method was added it makes sense to adapt the wrapper so that it can by used with any method, since we want to do the 2 things mentioned above for all methods and all engines. If a third method is ever added in the future, we'd only need to add 1 line of code in the top level index.js.

also consider the following:

_ = require 'lodash'

var callMethod = _.curry(function(methodName, platform, payment, cb) {
  ...
})

exports.someMethod = callMethod('someMethod');
ronkorving commented 6 years ago

You tried valiantly, but I'm sorry to say I'm sticking with my original position. I think this is overarchitecting. We are not winning anything (maybe a few loc?), and at the same time we are losing something in clarity. "If a third method is ever ..." is really just an "if". I'm a big proponent of optimizing for what we know today, not for what may happen in the future (because predictions are usually wrong). Sorry :-/

adrian-gierakowski commented 6 years ago

@ronkorving sorry for the delay. I'm closing this PR since you've rejected its main premise. I have created another PR (#58) which fixes an actual bug you've spotted while reviewing this one.