Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Approve code review #6

Merged
merged 7 commits into from
Jun 7, 2018
Merged

Approve code review #6

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
adbdeab
Approve code review
peter279k Jun 1, 2018
fc13c82
Approve the code review PR #6
peter279k Jun 1, 2018
2cc0eff
Approve second code review PR #6
peter279k Jun 1, 2018
a3b1487
Merge remote-tracking branch to master
peter279k Jun 1, 2018
9c41678
Remove SpecificRateRecord
peter279k Jun 1, 2018
358db04
Approve the code review #6 for small thing
peter279k Jun 2, 2018
b445fe0
Fix the count of rates
peter279k Jun 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 9 additions & 22 deletions src/Collection/AllRatesRecord.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,37 +10,24 @@ class AllRatesRecord extends CountableProviderRecords
{
private $base;

private $time;
public function __construct(
\Iterator $providerRecords,
string $base,
int $count,
ProviderResource $resource
) {
parent::__construct($providerRecords, $count, $resource);

private $quote;

private $rate;

public function __construct(array $time, string $base, array $quote, array $rate)
{
$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
public function toAssociativeArray(): array
{
return $this->rate;
return iterator_to_array($this);
Copy link

Choose a reason for hiding this comment

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

This doesn't seem to add any value, and its implementation encourages using array instead of iterators. If the client want to use arrays it can call iterator_to_array itself, but Porter and her providers should not encourage this because it loads the entire data set into memory. In this case, since we're not streaming the response, it's already loaded into memory anyway, but it's still a bad idea to encourage this. Recommend removing this method.

}
}
39 changes: 8 additions & 31 deletions src/Collection/SpecificRateRecord.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,39 +8,16 @@

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 __construct(
\Iterator $providerRecords,
int $count,
ProviderResource $resource
) {
parent::__construct($providerRecords, $count, $resource);
Copy link

Choose a reason for hiding this comment

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

A constructor override that only calls the parent implementation is the same as having no constructor override. That is, this whole constructor can be deleted and have exactly the same effect. However, I also suggest (as previously), that this entire file should be deleted because custom collections are usually to supply metadata but this class is not adding any extra value, let alone supplying metadata.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. I will remove this collection record.

}

public function getRate(): float
public function toAssociativeArray(): array
{
return $this->rate;
return iterator_to_array($this);
}
}
24 changes: 12 additions & 12 deletions src/Resource/GetAllRate.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,17 @@ public function fetch(ImportConnector $connector): \Iterator
);

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

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

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

$rate = function () use ($rates) {
Copy link

Choose a reason for hiding this comment

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

This closure seems to just copy the array into an iterator. You can replace this entire code block with new ArrayIterator($response['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);
}
}
17 changes: 10 additions & 7 deletions src/Resource/GetSpecificRate.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,15 @@ public function fetch(ImportConnector $connector): \Iterator
true
);

$base = $response['asset_id_base'];

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

return new SpecificRateRecord($time, $base, $quote, $rate);
$rate = function () use ($response) {
Copy link

Choose a reason for hiding this comment

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

Again, this seems to be a very roundabout way of copying an array. Since we don't need the SpecificRateRecord collection, this entire code block (lines 42-51) can be replaced with yield $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);
}
}
29 changes: 14 additions & 15 deletions tests/Functional/GetRateTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,13 @@ public function testGetSpecificRateRecords()
$rates = FixtureFactory::createPorter()->import(new ImportSpecification(new GetSpecificRate($this->apiKey)))
->findFirstCollection();
Copy link

Choose a reason for hiding this comment

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

This should not be necessary.


$this->assertSame('BTC', $rates->getBase());
$this->assertSame('USD', $rates->getQuote());
$this->assertLessThanOrEqual(time(), strtotime($rates->getTime()));
$this->assertInternalType('float', $rates->getRate());
$rateRecords = $rates->toAssociativeArray();
Copy link

Choose a reason for hiding this comment

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

This should not be necessary. Use Porter::importOne() instead of import().

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you add the wrong place for this comment.
I cannot see any code that is related to the import().

Copy link

Choose a reason for hiding this comment

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

They're both related. This line should be deleted because it is not necessary if you use importOne.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you give some sample code about this?
I don't understand Porter::importOne() usage clearly.

Thanks.

Copy link

@Bilge Bilge Jun 1, 2018 •
edited

Choose a reason for hiding this comment

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

import() returns the collection from the resource. importOne() does the same thing, except it returns the first record from the collection instead of the collection itself (and ensures there is no more than one record in the collection).


$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]);
}

public function testGetAllRateRecords()
Expand All @@ -33,16 +36,12 @@ public function testGetAllRateRecords()
$rates = FixtureFactory::createPorter()->import(new ImportSpecification(new GetAllRate($this->apiKey)))
->findFirstCollection();
Copy link

Choose a reason for hiding this comment

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

findFirstCollection() should only be used when you need to get metadata. Call it later when testing getBase(), but do not call it here, before iteration has been tested.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you show some example approach for this?

Thanks.

Copy link

Choose a reason for hiding this comment

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

Just call findFirstCollection() later, after you've done all the other assertions.


$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]);
$this->assertSame('BTC', $rates->getBase());

$rateRecords = $rates->toAssociativeArray();

$this->assertArrayHasKey('asset_id_quote', $rateRecords[0]);
$this->assertArrayHasKey('rate', $rateRecords[0]);
$this->assertArrayHasKey('time', $rateRecords[0]);
Copy link

Choose a reason for hiding this comment

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

This is a bad pattern for testing iterators: we should ensure the whole iterator looks and works as we expect, instead of only looking at the first record. Use foreach to loop over each record and assert each record is correct. It's a good idea to make assertions on the data too, not just the keys. If you don't know what the data should look like, at least try a type assertion and maybe a not empty assertion if the data should be non-null.

}
}