Method refactoring?

Question

In the TokenRepository you can see 3 similar methods. It create new entry to the tokens table but each method has different fields.

How can I refactor this? Should I merge 3 methods into 1 method or should I use strategy pattern?

TokenRepository Class:

class TokenRepository
{
   public function createTokenDigitalOcean(User $user, $name, $accessToken, $refreshToken = null)
   {
        return $user->tokens()->create([
            'name'          => $name,
            'provider'      => 'digital_ocean',
            'access_token'  => $accessToken,
            'refresh_token' => $refreshToken,
        ]);
    }

    public function createTokenLinode(User $user, $name, $key)
    {
        return $user->tokens()->create([
            'name'       => $name,
            'provider'   => 'linode',
            'linode_key' => $key,
        ]);
    }

    public function createTokenAws(User $user, $name, $key, $secret)
    {
        return $user->tokens()->create([
            'name'       => $name,
            'provider'   => 'aws',
            'aws_key'    => $key,
            'aws_secret' => $secret,
        ]);
    }
}

I have 3 classes like DigitalOceanProvider, LinodeProvider and AwsProvider. For example of using LinodeProvider and AwsProvider class.

class LinodeProvider 
{
  public function callback()
  {
    $this->tokenRepo->createTokenLinode($user, $name, $key);
  }
}


class AwsProvider 
{
  public function callback()
  {
    $this->tokenRepo->createTokenAws($user, $name, $key, $secret);
  }
}

I don't see anything wrong with your code. It's not bad overall(it's not great, but it's not bad either, at least you get autocomplete in IDEs), the only thing I'd argue is worth doing would be some sort of syntactic sugar with method chaining. Along the lines of `$this->tokenRepo->create->aws()` for example. — Andrei, Nov 29 '16 at 15:36
I also agree that it's pretty good as is and unless you have a reason to refactor this might be fine. One thing I might consider doing is instead of having different fields like 'linode_key' and 'aws_key' etc, I'd just have a generic 'key', 'secret' etc. you actually already have a column for 'provider' so u don't need to make the other columns specific. This can help clean things up a bit and will also help in the future — tam5, Nov 29 '16 at 17:27

jardis · Answer 1 · 2017-01-30T17:43:53.763

This may be a bit overkill, but in order to make life a bit easier in the future, you could create separate implementations of each that extend an abstract class. This way you can unify and define the interface and easily add new token types.

<?php namespace Foo\Tokens;

abstract class Token
{
    protected $name = '';

    protected $key = '';

    protected $provider = '';

    public function __construct($name, $key)
    {
        $this->name = $name;
        $this->key = $key;
    }

    public function data()
    {
        return [
            'name' => $this->name,
            'provider' => $this->provider,
            'token' => $this->key
        ];
    }
}

Next, we create our Digital Ocean token class. This class can either use the default implementation or redefine it.

<?php namespace Foo\Tokens;

use Foo\Tokens\Token;

class DigitalOceanToken extends Token
{

    protected $provider = 'digital_ocean';

    public function __construct($name, $key, $refreshToken = null)
    {
        parent::__construct($name, $key);

        $this->refreshToken = $refreshToken;
    }

    public function data()
    {
        return [
            'name' => $this->name,
            'provider' => $this->provider,
            'key' => $this->key,
            'refreshToken' => $this->refreshToken
        ];
    }
}

The TokenRepository now merely cares about attaching a given token to a user.

<?php namespace Foo;

use User;
use Foo\Tokens\Token;

class TokenRepository
{
   public function createToken(User $user, Token $token)
   {
        return $user->tokens()->create(
            $token->data()
        );
    }
}

And your service providers are as simple as...

<?php 

use Foo\Tokens\AwsToken;

class AwsProvider
{
    public function callback()
    {
        $this->tokenRepo->createToken(
            $user, new AwsToken($name, $key, $secret)
        );
    }
}

This isn't working code, as I've not attempted to run it however it's just another idea of how you can organize and assign responsibility. Hope it helps, and welcome feedback from others.

score 0 · Answer 2 · answered Nov 29 '16 at 15:25

According to me you should implement it like this:

class TokenRepository
{
    public function createTokenForVendor(User $user, $inputs)
    {
        return $user->tokens()->create($inputs);
    }
}

and inside your callback:

class VendorProvider 
{
  public function callback()
  {
    switch($tokenType) {
        case 'DigitalOcean':
            $inputs = [
            'name'          => $name,
            'provider'      => 'digital_ocean',
            'access_token'  => $accessToken,
            'refresh_token' => $refreshToken,
        ];
        break;

      case 'Linode':
            $inputs = [
            'name'       => $name,
            'provider'   => 'linode',
            'linode_key' => $key,
        ];
        break;

      case 'Aws':
            $inputs = [
            'name'       => $name,
            'provider'   => 'aws',
            'aws_key'    => $key,
            'aws_secret' => $secret,
        ];
        break;
    }

    $this->tokenRepo->createTokenForVendor($user, $inputs);
  }
}

Hoping you should do some code structure revamp.

Hope this helps!

This is a debatable approach. Passing in strings to create tokens(or anything for that matter) is ambiguous at best. You have no IDE autocompletion, if I were to inherit this code I'd have no idea what string I'd need to pass in to actually get a token and other than digging thru the code I see no method of actually finding out. — Andrei, Nov 29 '16 at 15:33
This is worse than OP's code, and you don't really gain anything from this refactor, in fact you arguably lose quite a bit. — tam5, Nov 29 '16 at 17:54

Method refactoring?

2 Answers2