Skip to content

Commit

Permalink
Reverse
Browse files Browse the repository at this point in the history
  • Loading branch information
peter279k committed May 31, 2018
1 parent c550cd3 commit 4246a99
Show file tree
Hide file tree
Showing 8 changed files with 320 additions and 0 deletions.
46 changes: 46 additions & 0 deletions src/Collection/AllRatesRecord.php
@@ -0,0 +1,46 @@
<?php
declare(strict_types=1);

namespace PayCrypto\Collection;

use ScriptFUSION\Porter\Collection\CountableProviderRecords;
use ScriptFUSION\Porter\Provider\Resource\ProviderResource;

class AllRatesRecord extends CountableProviderRecords
{
private $base;

private $time;

private $quote;

private $rate;

public function __construct(array $time, string $base, array $quote, array $rate)

This comment has been minimized.

Copy link
@Bilge

Bilge May 31, 2018

This is invalid because it does not call the parent's constructor. It seems to me this custom collection class is not needed at all. Instead, just return the data as an array from the iterator. Recommend deleting this file and SpecificRateRecord, too.

{
$this->base = $base;
$this->time = $time;
$this->quote = $quote;
$this->rate = $rate;
}

public function getBase(): string
{
return $this->base;
}

public function getTime(): array
{
return $this->time;
}

public function getQuote(): array
{
return $this->quote;
}

public function getRate(): array
{
return $this->rate;
}
}
46 changes: 46 additions & 0 deletions src/Collection/SpecificRateRecord.php
@@ -0,0 +1,46 @@
<?php
declare(strict_types=1);

namespace PayCrypto\Collection;

use ScriptFUSION\Porter\Collection\CountableProviderRecords;
use ScriptFUSION\Porter\Provider\Resource\ProviderResource;

class SpecificRateRecord extends CountableProviderRecords
{
private $base;

private $time;

private $quote;

private $rate;

public function __construct(string $time, string $base, string $quote, float $rate)
{
$this->base = $base;
$this->time = $time;
$this->quote = $quote;
$this->rate = $rate;
}

public function getBase(): string
{
return $this->base;
}

public function getTime(): string
{
return $this->time;
}

public function getQuote(): string
{
return $this->quote;
}

public function getRate(): float
{
return $this->rate;
}
}
19 changes: 19 additions & 0 deletions src/Connector/CryptoConnector.php
@@ -0,0 +1,19 @@
<?php
declare(strict_types=1);

namespace PayCrypto\Connector;

use ScriptFUSION\Porter\Net\Http\HttpConnector;
use ScriptFUSION\Porter\Net\Http\HttpOptions;

class CryptoConnector extends HttpConnector
{
private $options;

public function __construct(string $apiKey)
{
$buildHeader = sprintf("X-CoinAPI-Key: %s", $apiKey);

parent::__construct($this->options = (new HttpOptions)->addHeader($buildHeader));
}
}
29 changes: 29 additions & 0 deletions src/CryptoMonitor.php
@@ -0,0 +1,29 @@
<?php
declare(strict_types=1);

namespace PayCrypto;

use ScriptFUSION\Porter\Connector\Connector;
use ScriptFUSION\Porter\Provider\Provider;

class CryptoMonitor implements Provider

This comment has been minimized.

Copy link
@Bilge

Bilge May 31, 2018

Consider marking this class final, since it is usually wrong to extend providers.

{
public const EXCHANGE_API_ENDPOINT = 'https://rest.coinapi.io/';

private $connector;

public function __construct(Connector $connector)
{
$this->connector = $connector;
}

public static function buildExchangeApiUrl(string $url): string
{
return self::EXCHANGE_API_ENDPOINT . $url;
}

public function getConnector(): Connector
{
return $this->connector;
}
}
54 changes: 54 additions & 0 deletions src/Resource/GetAllRate.php
@@ -0,0 +1,54 @@
<?php
declare(strict_types=1);

namespace PayCrypto\Resource;

use PayCrypto\CryptoMonitor;
use PayCrypto\Collection\AllRatesRecord;
use ScriptFUSION\Porter\Connector\ImportConnector;
use ScriptFUSION\Porter\Provider\Patreon\Collection\PledgeRecords;
use ScriptFUSION\Porter\Provider\Patreon\PatreonProvider;
use ScriptFUSION\Porter\Provider\Resource\ProviderResource;

class GetAllRate implements ProviderResource
{
private $apiKey;

public $base = 'BTC';

This comment has been minimized.

Copy link
@Bilge

Bilge May 31, 2018

All fields should be private. By setting this private and allowing it to be set only in the constructor, we create an immutable base field, which is highly desirable.


public function __construct(string $apiKey)
{
$this->apiKey = $apiKey;
}

public function getProviderClassName(): string
{
return CryptoMonitor::class;
}

public function fetch(ImportConnector $connector): \Iterator
{
$response = \json_decode(
(string) $connector->fetch(
CryptoMonitor::buildExchangeApiUrl(
sprintf("v1/exchangerate/%s", $this->base)
)
),
true
);

$rates = $response['rates'];
$base = $response['asset_id_base'];
$quote = [];
$rate = [];
$time = [];

foreach($rates as $key => $value) {
$quote[] = $rates[$key]['asset_id_quote'];

This comment has been minimized.

Copy link
@Bilge

Bilge May 31, 2018

It seems the API response is being transformed into a different data representation here, which you are strongly discouraged from doing. The resource should return a format that is as close to the original API as possible. It is up to consumers of the provider if and how they want to transform it. They can do so by attaching Porter transformers, such as Mapper.

Since this represents a collection of values, we should be returning a series of values. Here I recommend iterating over the collection of rates (foreach ($response['rates'] as $rate) { ... }) and yielding each rate (yield $rate).

$rate[] = $rates[$key]['rate'];
$time[] = $rates[$key]['time'];
}

return new AllRatesRecord($time, $base, $quote, $rate);

This comment has been minimized.

Copy link
@Bilge

Bilge May 31, 2018

Since we also know how many rates there are (because we loaded the whole collection into memory), it would be useful to return new CountableProviderRecords here. There is additional metadata in the API response, but it is just the base, which we should already know because it's assigned to this resource (and can be retrieved directly from the resource, if needed), so we still don't really need a custom collection class here.

}
}
50 changes: 50 additions & 0 deletions src/Resource/GetSpecificRate.php
@@ -0,0 +1,50 @@
<?php
declare(strict_types=1);

namespace PayCrypto\Resource;

use PayCrypto\CryptoMonitor;
use PayCrypto\Collection\SpecificRateRecord;
use ScriptFUSION\Porter\Connector\ImportConnector;
use ScriptFUSION\Porter\Provider\Patreon\Collection\PledgeRecords;
use ScriptFUSION\Porter\Provider\Patreon\PatreonProvider;
use ScriptFUSION\Porter\Provider\Resource\ProviderResource;

class GetSpecificRate implements ProviderResource
{
private $apiKey;

public $base = 'BTC';

public $quote = 'USD';

This comment has been minimized.

Copy link
@Bilge

Bilge May 31, 2018

Once again, this public fields should be private fields assigned only in the constructor.


public function __construct(string $apiKey)
{
$this->apiKey = $apiKey;
}

public function getProviderClassName(): string
{
return CryptoMonitor::class;
}

public function fetch(ImportConnector $connector): \Iterator
{
$response = \json_decode(
(string) $connector->fetch(
CryptoMonitor::buildExchangeApiUrl(
sprintf("v1/exchangerate/%s/%s", $this->base, $this->quote)
)
),
true
);

$base = $response['asset_id_base'];

$quote = $response['asset_id_quote'];
$rate = $response['rate'];
$time = $response['time'];

return new SpecificRateRecord($time, $base, $quote, $rate);

This comment has been minimized.

Copy link
@Bilge

Bilge May 31, 2018

Since this just represents a single record of data, I recommend replacing all this code with just yield $response; (from lines 42-48). You could also just yield the json_decode call (since $response becomes a redundant, single-use variable then).

}
}
28 changes: 28 additions & 0 deletions tests/FixtureFactory.php
@@ -0,0 +1,28 @@
<?php

namespace PayCrypto\Tests;

use Psr\Container\ContainerInterface;
use ScriptFUSION\Porter\Porter;
use PayCrypto\Connector\CryptoConnector;
use PayCrypto\CryptoMonitor;
use ScriptFUSION\StaticClass;

final class FixtureFactory
{
use StaticClass;

public static function createPorter(): Porter
{
return new Porter(
\Mockery::mock(ContainerInterface::class)
->shouldReceive('has')
->with(CryptoMonitor::class)
->andReturn(true)
->shouldReceive('get')
->with(CryptoMonitor::class)
->andReturn(new CryptoMonitor(new CryptoConnector('4E861687-19D6-4894-87B9-E785B1EE3900')))
->getMock()
);
}
}
48 changes: 48 additions & 0 deletions tests/Functional/GetRateTest.php
@@ -0,0 +1,48 @@
<?php
declare(strict_types=1);

namespace PayCrypto\Tests;

use PHPUnit\Framework\TestCase;
use PayCrypto\Resource\GetSpecificRate;
use PayCrypto\Resource\GetAllRate;
use PayCrypto\Collection\SpecificRateRecord;
use PayCrypto\Collection\AllRatesRecord;
use ScriptFUSION\Porter\Specification\ImportSpecification;

final class GetRateTest extends TestCase
{
/** @var $apiKey This is the Coin API Key for test environment*/
private $apiKey = '4E861687-19D6-4894-87B9-E785B1EE3900';

This comment has been minimized.

Copy link
@Bilge

Bilge May 31, 2018

It's not a good idea to commit private keys into a repository. This should be revoked and replaced with an environment variable or something (e.g. $_SERVER['COINAPI_KEY']).


public function testGetSpecificRateRecords()
{
/** @var SpecificRateRecord $rates */
$rates = FixtureFactory::createPorter()->import(new ImportSpecification(new GetSpecificRate($this->apiKey)))
->findFirstCollection();

This comment has been minimized.

Copy link
@Bilge

Bilge May 31, 2018

It should not be necessary to call findFirstCollection(). Since this tests a resource that is known to return a single record, importOne() should be used instead of import().


$this->assertSame('BTC', $rates->getBase());
$this->assertSame('USD', $rates->getQuote());
$this->assertLessThanOrEqual(time(), strtotime($rates->getTime()));
$this->assertInternalType('float', $rates->getRate());
}

public function testGetAllRateRecords()
{
/** @var AllRatesRecord $rates */
$rates = FixtureFactory::createPorter()->import(new ImportSpecification(new GetAllRate($this->apiKey)))
->findFirstCollection();

This comment has been minimized.

Copy link
@Bilge

Bilge May 31, 2018

It should not be necessary to call findFirstCollection() here. Since this tests a collection of records, the rates should be iterated, and assertions performed on every member of the collection.


$base = $rates->getBase();
$quote = $rates->getQuote();
$time = $rates->getTime();
$rate = $rates->getRate();

$this->assertSame('BTC', $base);
$this->assertContains('USD', $quote);
$this->assertContains('GBP', $quote);
$this->assertContains('EUR', $quote);
$this->assertLessThanOrEqual(time(), strtotime($time[0]));
$this->assertInternalType('float', $rate[0]);
}
}

5 comments on commit 4246a99

@Bilge
Copy link

@Bilge Bilge commented on 4246a99 May 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The biggest problem here is that the iterator contract has been broken: resources are required to return a functional iterator, but a fake iterator is being returned from these resources. This has been done by extending an iterator and then treating it as a regular object. This means Porter::import also returns a fake iterator that cannot be iterated and Porter::importOne will break. However, you are avoiding encountering these issues by calling findFirstCollection on the false iterator instead of Porter::importOne and then using object methods.

I understand you probably got this idea from looking at the source for the European Central Bank provider. Whilst you can create custom record collection and you can attach additional data to it, this is the metadata pattern, and it's usually wrong to have a collection that is only metadata. This data should be exposed as an array returned from the iterator instead.

@Bilge
Copy link

@Bilge Bilge commented on 4246a99 May 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've finished the review. There's some important changes to make. I'll take another look once you've addressed the issues. Let me know if you have any questions. Please submit any further changes in a separate PR branch, instead of committing to master.

@peter279k
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Bilge, thank you for your reply.
And I almost have done this request changes.
Please see the latest commit and write the comments if having any issues.
I appreciated your work again!

Thanks.

@Bilge
Copy link

@Bilge Bilge commented on 4246a99 May 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot review commits because they lack context. Please make a PR instead of committing to master.

@peter279k
Copy link
Member Author

@peter279k peter279k commented on 4246a99 Jun 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, it's my fault.
Please take a look at this PR and do code review.

Thanks.

Please sign in to comment.