Using dependency injection for Node payment system

Question

I would like some feedback on my code. I am currently building a payments solution in a serverless lambda project. This what I came up with (very basic code at the moment):

// "interface implementation"
export default class Stripe {
  constructor() {
    this.stripe = new Stripe()
  }

  async charge(data) {
    try {
      const result = await this.stripe.chargeCustomerByToken(data);
      return result;
    } catch (error) {
      throw error;
    }
  }
}

export default class PaymentRepository {
  constructor(PaymentInterface) {
    this.ipayment = PaymentInterface;
  }

  async charge(data) {
    try {
      const result = await this.ipayment.charge(data);
      return result;
    } catch (error) {
      throw error;
    }
  }
}

export const charge = async (repository, data) => {
  if (!repository || !data) {
    throw new InvalidParameterError('Missing dependencies');
  }

  try {
    const result = await repository.charge(data);
    return result;
  } catch (error) {
    throw error;
  }
};

I would then use it like this in my main handler: charge(new PaymentRepository(new Stripe())).

This is my current structure. It feels super repetitive but still flexible and is easy to test, and also easy to change payment API (maybe to PayPal or something). But I'm not really sure if I'm just over-complexing the whole project, or maybe this isn't even the Node/JS way to code.

Answer 1

It looks good to me, very SOLID.

Here are a couple of suggestions:

I'm not really seeing the value in the PaymentRepository layer. If the Stripe wrapper is following a strict interface then just import it directly as your payment gateway i.e.

import PaymentGateway from './stripe';
...
charge(new PaymentGateway());

This still gives you the flexibility & testability but with one less layer. If you introduced a new payment gateway all that changes is the import e.g.

import PaymentGateway from './worldpay.js'

Also, you may have gone a bit overboard with your use of async / await. This code:

async x() {
  try {
    const result = await promise();
    return result;
  } catch (e) {
    throw e;
  } 
}

Is effectively the same as:

x() { 
  return promise();
}

Because you don't actually use the result (or error) until further up the callstack. Although having said that, you might want to consider deserializing your Stripe API responses to avoid leaking internal data through the layers.

Answer 2

I don't see the point in using classes. Also it kinda sucks that the runtime usage of the code needs to pass dependencies around. It would be much more simpler to use factory functions.

// respository/stripe.js
export default () => {
  return {
    async charge(data) {
      return await this.stripe.chargeCustomerByToken(data);
    }
  };
};

 

// service/payment.js
export default (stripeRepository) => {
  if (!repository) {
    throw new InvalidParameterError('Missing dependencies');
  }
  return {
    async charge(data) {
      if (!data) {
        throw new InvalidParameterError('Missing data');
      }

      return await stripeRepository.charge(data);
    }
  };
};

 

// main.js
import StripeRepository from './repository/stripe';
import PaymentService from './service/payment';

// wire once at application startup
const paymentService = PaymentService(StripeRepository());

// some time later
paymentService.charge('some data');

See https://medium.com/@pyrolistical/factory-functions-pattern-in-depth-356d14801c91