Approve code review #6

peter279k · 2018-06-01T01:38:14Z

@Bilge , this is the second code review after modifying this.

Bilge · 2018-06-01T07:56:55Z

src/Collection/SpecificRateRecord.php

+        int $count,
+        ProviderResource $resource
+    ) {
+        parent::__construct($providerRecords, $count, $resource);


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.

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

Bilge · 2018-06-01T08:01:59Z

src/Resource/GetAllRate.php

-
-        return new AllRatesRecord($time, $base, $quote, $rate);
+
+        $rate = function () use ($rates) {


This closure seems to just copy the array into an iterator. You can replace this entire code block with new ArrayIterator($response['rates']).

Bilge · 2018-06-01T08:03:37Z

src/Collection/AllRatesRecord.php

    {
-        return $this->rate;
+        return iterator_to_array($this);


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.

Bilge · 2018-06-01T08:06:20Z

src/Resource/GetSpecificRate.php

-        $time = $response['time'];
-
-        return new SpecificRateRecord($time, $base, $quote, $rate);
+        $rate = function () use ($response) {


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.

Bilge · 2018-06-01T08:06:46Z

tests/Functional/GetRateTest.php

@@ -21,10 +21,13 @@ public function testGetSpecificRateRecords()
        $rates = FixtureFactory::createPorter()->import(new ImportSpecification(new GetSpecificRate($this->apiKey)))
            ->findFirstCollection();


This should not be necessary.

Bilge · 2018-06-01T08:07:27Z

tests/Functional/GetRateTest.php

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


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

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

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

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

Thanks.

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).

Bilge · 2018-06-01T08:08:43Z

tests/Functional/GetRateTest.php

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


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.

Can you show some example approach for this?

Thanks.

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

Bilge · 2018-06-01T08:10:11Z

tests/Functional/GetRateTest.php

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


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.

peter279k · 2018-06-01T13:50:47Z

@Bilge, I almost do the suggestion that you recommend.
I also refer the Steam provider to complete my test approach.
Please review the latest commit.

Thanks.

Bilge · 2018-06-01T13:54:42Z

I'm sorry but this is completely wrong. It seems you are doing your own thing instead of following my guidelines.

Bilge · 2018-06-01T15:59:45Z

src/Resource/GetSpecificRate.php

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

    public function fetch(ImportConnector $connector): \Iterator
    {
-        $response = \json_decode(
+        $response[] = \json_decode(


This is wrapping your record in an outer array, which is not correct. This should just be $response instead of $response[].

Bilge · 2018-06-01T16:00:11Z

src/Resource/GetSpecificRate.php

-        };
-
-        return new SpecificRateRecord($rate(), count($response), $this);
+        return yield $response;


return does nothing here, since yield $response has no return value. Should just be yield $response.

Bilge · 2018-06-01T16:00:54Z

tests/Functional/GetRateTest.php


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


This is unnecessary because importOne guarantees the iterator only contains one result. If it contains more it throws an exception.

Bilge · 2018-06-01T16:03:55Z

tests/Functional/GetRateTest.php


-        $this->assertSame('BTC', $rates->getBase());
+        $this->assertCount(1267, $rates);


This magic number is too brittle. The test needs to prove the count is correct without breaking whenever a rate is added or removed. You have two options, either:

Count the number of records during iteration and ensure it matches the reported count. To do this, the assertion would have to be moved after the loop.

Estimate the number of expected values, e.g. self::assertGreaterThan(1200, count($rates)).

peter279k · 2018-06-05T13:30:46Z

@Bilge, I've approved the recommendations that you suggest.
If having any problem, please add the comment to let me know.

Thanks.

Approve code review

adbdeab

peter279k referenced this pull request Jun 1, 2018

Reverse

4246a99

Bilge suggested changes Jun 1, 2018

View reviewed changes

Approve the code review PR #6

fc13c82

peter279k added 3 commits June 1, 2018 22:51

Approve second code review PR #6

2cc0eff

Merge remote-tracking branch to master

a3b1487

Remove SpecificRateRecord

9c41678

Bilge suggested changes Jun 1, 2018

View reviewed changes

peter279k added 2 commits June 2, 2018 11:59

Approve the code review #6 for small thing

358db04

Fix the count of rates

b445fe0

peter279k merged commit 5dad284 into master Jun 7, 2018

peter279k mentioned this pull request Jun 7, 2018

Code review #3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Approve code review #6

Approve code review #6

peter279k commented Jun 1, 2018 •

edited

Bilge Jun 1, 2018

peter279k Jun 1, 2018

Bilge Jun 1, 2018

Bilge Jun 1, 2018

Bilge Jun 1, 2018

Bilge Jun 1, 2018

Bilge Jun 1, 2018

peter279k Jun 1, 2018

Bilge Jun 1, 2018

peter279k Jun 1, 2018

Bilge Jun 1, 2018 •

edited

Bilge Jun 1, 2018

peter279k Jun 1, 2018

Bilge Jun 1, 2018

Bilge Jun 1, 2018

peter279k commented Jun 1, 2018

Bilge commented Jun 1, 2018

Bilge Jun 1, 2018

Bilge Jun 1, 2018

Bilge Jun 1, 2018

Bilge Jun 1, 2018

peter279k commented Jun 5, 2018


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

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

		@@ -21,10 +21,13 @@ public function testGetSpecificRateRecords()
		$rates = FixtureFactory::createPorter()->import(new ImportSpecification(new GetSpecificRate($this->apiKey)))
		->findFirstCollection();

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


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


		$this->assertSame('BTC', $rates->getBase());
		$this->assertCount(1267, $rates);

Approve code review #6

Approve code review #6

Conversation

peter279k commented Jun 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bilge Jun 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peter279k commented Jun 1, 2018

Bilge commented Jun 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peter279k commented Jun 5, 2018

peter279k commented Jun 1, 2018 •

edited

Bilge Jun 1, 2018 •

edited