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.

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

Enhance example and add assets api #8

Merged
merged 1 commit into from
Jun 14, 2018
Merged

Enhance example and add assets api #8

merged 1 commit into from
Jun 14, 2018

Conversation

peter279k
Copy link
Member

Changed log

  • Enhance the sample code and add the Assets API.

@peter279k peter279k merged commit 9ae1948 into master Jun 14, 2018
@coveralls
Copy link

coveralls commented Jun 14, 2018

Pull Request Test Coverage Report for Build 36

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 33: 0.0%
Covered Lines: 54
Relevant Lines: 54

💛 - Coveralls

@peter279k
Copy link
Member Author

Hi @josantonius, thank you for your reply.
I also let the different API be the different resources.
You can see more details about this folder.

I think we have to separate the APIs for the different resources because each one resource has the different response JSON collection.

It's necessary to separate that and another reason is convenient to do the debug work.
Also, we can just focus on implementing the response JSON records.
If we implement all works in CryptoMonitor class, it's not maintainable and violates the single-responsibility principle of developing classes.

@josantonius
Copy link
Member

Hello Peter, how are you?

Sorry for the delay, between work and holidays I haven't had much time.

I wasn't referring specifically to the class structure, which is pretty good, but to the use that the end user will make of it.

Now we would:

use Joomla\DI\Container;
use ScriptFUSION\Porter\Porter;
use PayCrypto\CryptoMonitor;
use PayCrypto\Connector\CryptoConnector;
use PayCrypto\Resource\GetSpecificRate;
use PayCrypto\Resource\GetAllRate;
use PayCrypto\Resource\GetAssets;
use ScriptFUSION\Porter\Specification\ImportSpecification;

// Get specific rate

$apiKey = 'your-coin-api-key';

$container = new Container;
$container->set(CryptoMonitor::class, new CryptoMonitor(new CryptoConnector($apiKey)));
$porter = new Porter($container);
$specificRate = new GetSpecificRate($apiKey, 'BTC', 'USD');

$rates = $porter->importOne(new ImportSpecification($specificRate));

var_dump($rates);

I think we could do that internally and the end use would be much more simplified:

  • Create an object to which we pass the provider and the API key.

  • Use that object to get the rate.

Which would lead to something like that:

use PayCrypto\Foo;

$Foo = new Foo('provide-namer', 'your-coin-api-key'); // or using setters

$Foo->getSpecificRate('BTC', 'USD');

Basically the idea would be to create an extra class to take care of this (Container, Porter...) and this simplify the final use of the library.

What do you think?

@peter279k
Copy link
Member Author

peter279k commented Sep 22, 2018

Hi @josantonius, I glad that I see your comment again 😄.
Yeah. Your suggestion looks good. How about overriding the CryptoMonitor class constructor to complete your recommendation?

public function __construct(Connector $connector) { ... }

If possible, can you create the issue about this? Thanks.

@peter279k peter279k deleted the enhance_example branch September 22, 2018 09:01
@josantonius
Copy link
Member

Yeah, give me a few days to catch up and I see how we do it 😉.

I also have the README file almost ready, but I stopped at the methods because it was a little confusing this way.

@josantonius
Copy link
Member

Hi @peter279k,

I've uploaded my suggestion to the next branch.

It could be polished further, but the idea would be something like that.

@peter279k
Copy link
Member Author

Hi @josantonius, thank you for your reply.

I also create the realted PR #9.

Please review this code at your available time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants