Skip to content

Commit

Permalink
Approve the code review PR #6
Browse files Browse the repository at this point in the history
  • Loading branch information
@peter279k
peter279k committed Jun 1, 2018
1 parent adbdeab commit fc13c82
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 72 deletions.
5 changes: 0 additions & 5 deletions src/Collection/AllRatesRecord.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,4 @@ public function getBase(): string
{
return $this->base;
}

public function toAssociativeArray(): array
{
return iterator_to_array($this);
}
}
23 changes: 0 additions & 23 deletions src/Collection/SpecificRateRecord.php
View file

This file was deleted.

19 changes: 5 additions & 14 deletions src/Resource/GetAllRate.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,12 @@ class GetAllRate implements ProviderResource
{
private $apiKey;

public $base = 'BTC';
private $base;

public function __construct(string $apiKey)
public function __construct(string $apiKey, string $base = 'BTC')
{
$this->apiKey = $apiKey;
$this->base = $base;
}

public function getProviderClassName(): string
Expand All @@ -37,18 +38,8 @@ public function fetch(ImportConnector $connector): \Iterator
true
);

$rates = $response['rates'];
$rates = new \ArrayIterator($response['rates']);

$rate = function () use ($rates) {
foreach ($rates as $key => $value) {
yield [
'asset_id_quote' => $rates[$key]['asset_id_quote'],
'rate' => $rates[$key]['rate'],
'time' => $rates[$key]['time'],
];
}
};

return new AllRatesRecord($rate(), $response['asset_id_base'], count($rates), $this);
return new AllRatesRecord($rates, $this->base, count($response), $this);

This comment has been minimized.

Sign in to view

Copy link
@Bilge

Bilge Jun 1, 2018

There's no way count($response) can be correct. Your tests should catch this error. This has to be the count of the number of records.

This comment has been minimized.

Sign in to view

Copy link
@peter279k

peter279k Jun 1, 2018

Author Member

Should it be the count($rates)?

This comment has been minimized.

Sign in to view

Copy link
@Bilge

Bilge Jun 1, 2018

Yes, I think so, but don't forget to add a test that proves it is correct.
}
}
21 changes: 7 additions & 14 deletions src/Resource/GetSpecificRate.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,15 @@ class GetSpecificRate implements ProviderResource
{
private $apiKey;

public $base = 'BTC';
private $base;

public $quote = 'USD';
private $quote;

public function __construct(string $apiKey)
public function __construct(string $apiKey, string $base = 'BTC', string $quote = 'USD')
{
$this->apiKey = $apiKey;
$this->base = $base;
$this->quote = $quote;
}

public function getProviderClassName(): string
Expand All @@ -30,7 +32,7 @@ public function getProviderClassName(): string

public function fetch(ImportConnector $connector): \Iterator
{
$response = \json_decode(
$response[] = \json_decode(
(string) $connector->fetch(
CryptoMonitor::buildExchangeApiUrl(
sprintf("v1/exchangerate/%s/%s", $this->base, $this->quote)
Expand All @@ -39,15 +41,6 @@ public function fetch(ImportConnector $connector): \Iterator
true
);

$rate = function () use ($response) {
yield [
'asset_id_base' => $response['asset_id_base'],
'asset_id_quote' => $response['asset_id_quote'],
'rate' => $response['rate'],
'time' => $response['time'],
];
};

return new SpecificRateRecord($rate(), count($response), $this);
return new \ArrayIterator($response);

This comment has been minimized.

Sign in to view

Copy link
@Bilge

Bilge Jun 1, 2018

We do not need an ArrayIterator here, we can simply yield $response; (as mentioned three times now).
}
}
36 changes: 20 additions & 16 deletions tests/Functional/GetRateTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,31 +17,35 @@ final class GetRateTest extends TestCase

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

This comment has been minimized.

Sign in to view

Copy link
@Bilge

Bilge Jun 1, 2018 •
edited

This annotation doesn't make sense and is incorrect anyway. Should be removed.
$rate = FixtureFactory::createPorter()->import(new ImportSpecification(new GetSpecificRate($this->apiKey)));

This comment has been minimized.

Sign in to view

Copy link
@Bilge

Bilge Jun 1, 2018

Since it is know this is an iterator containing one and only one record, this call must be importOne() (as mentioned at least twice).

$rateRecords = $rates->toAssociativeArray();
$this->assertCount(1, $rate);

$this->assertCount(1, $rateRecords);
$this->assertArrayHasKey('asset_id_base', $rateRecords[0]);
$this->assertArrayHasKey('asset_id_quote', $rateRecords[0]);
$this->assertArrayHasKey('rate', $rateRecords[0]);
$this->assertArrayHasKey('time', $rateRecords[0]);
$rateRecords = iterator_to_array($rate);

This comment has been minimized.

Sign in to view

Copy link
@Bilge

Bilge Jun 1, 2018

This is not needed; should be removed. There is no reason why an iterator needs to be converted to an array to be iterated.

foreach($rateRecords as $rateRecord) {

This comment has been minimized.

Sign in to view

Copy link
@Bilge

Bilge Jun 1, 2018

There shouldn't be any need to iterate a single record collection.
$this->assertArrayHasKey('asset_id_base', $rateRecord);
$this->assertArrayHasKey('asset_id_quote', $rateRecord);
$this->assertArrayHasKey('rate', $rateRecord);
$this->assertArrayHasKey('time', $rateRecord);
}
}

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

$this->assertSame('BTC', $rates->getBase());
$rates = $rates->findFirstCollection();

This comment has been minimized.

Sign in to view

Copy link
@Bilge

Bilge Jun 1, 2018 •
edited

Again, this should be done last. Otherwise, you are iterating the underling iterator instead of the final iterator (which is a very bad idea; you would lose all transformations and other modifiers applied by Porter). I still don't think you need a custom collection just to store the base anyway, since the base can be derived from the iterator stack if you really need to.

This comment has been minimized.

Sign in to view

Copy link
@peter279k

peter279k Jun 1, 2018

Author Member

This code is placed in the last of this method.

$rates = $rates->findFirstCollection();
$this->assertSame('BTC', $rates->getBase());

I try to use getBase to assert this so I use the findFirstCollection.

How can I modify this?
$rateRecords = iterator_to_array($rates);

This comment has been minimized.

Sign in to view

Copy link
@Bilge

Bilge Jun 1, 2018

Again, no reason to convert an iterator to an array. This statement should be removed.

$rateRecords = $rates->toAssociativeArray();
$this->assertSame('BTC', $rates->getBase());

$this->assertArrayHasKey('asset_id_quote', $rateRecords[0]);
$this->assertArrayHasKey('rate', $rateRecords[0]);
$this->assertArrayHasKey('time', $rateRecords[0]);
foreach ($rateRecords as $rateRecord) {
$this->assertArrayHasKey('asset_id_quote', $rateRecord);
$this->assertArrayHasKey('rate', $rateRecord);
$this->assertArrayHasKey('time', $rateRecord);
}
}
}

1 comment on commit fc13c82

@Bilge
Copy link

@Bilge Bilge commented on fc13c82 Jun 1, 2018 •
edited

Choose a reason for hiding this comment

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

You need to understand:

  1. What an Iterator in PHP actually is. It is an object that may yield multiple results and can be iterated with foreach.
  2. You have two different types of resource: one yields a single record and one yields multiple records. The way you import them is different (import() vs importOne()) and the way you test them is different. Right now you're getting confused and trying to apply the same changes to both, as if they were the same thing. I suggest you split your tests into separate files because they are not related.

Please sign in to comment.