trjhtr

I'm using 2 objects inside my class. My actions are almost identical, however I want to return 2 different data types and both are slightly different from each other.

import requests


class coins:

 def coins_arr(self):
 params = 'limit': 10, 'convert': 'USD'
 data = requests.get('https://api.coinmarketcap.com/v1/ticker/?%s' % params).json()
 coinsData = 
 for objects in data:
 for k in objects.keys():
 if k == 'id':
 coinsData += objects[k].split()
 #returns array of coin names 
 return sorted(coinsData)



 def coins_dict(self):
 params = 'limit': 10, 'convert': 'USD'
 data = requests.get('https://api.coinmarketcap.com/v1/ticker/?%s' % params).json()
 coinsDict = 
 for objects in data:
 for k,v in objects.items():
 if k == 'id':
 coinsDict.update(objects[k]:objects['price_usd'])
 #returns dictionary with name:price 
 return coinsDict

d = coins()

print (d.coins_arr())

I thought of returning the coins_dict.keys(), but it won't return me it in a sorted order. I am not sure if I can implement a lambda function when I call the class object.

You don't need a class

The point of a class is so that you can make an instance of it which represents some kind of object which can be passed around and can hold internal state.

This isn't the case here. The class coins has no use except to be a namespace in which two functions are defined (which is subverted by prefixing the function names by the class name).

An indicator for this is that the self argument of the two functions is never used.

You don't need two functions

The two functions essentially do the same thing, except that one function (coins_arr returns a sorted list of coin names) is a specialized version of the other function (coins_dict returns a mapping from coin names to their prices).

There should be only the coins_dict function, and if absolutely necessary the coins_arr function could be defined in terms of coins_dict, but not by repeating a variation of the same code.

Bug

As already noted in another answer, the two parameters limit and convert have no effect because they are passed to request.get in the wrong way:

>>> params = 'limit': 10, 'convert': 'USD'
>>> print('https://api.coinmarketcap.com/v1/ticker/?%s' % params)
https://api.coinmarketcap.com/v1/ticker/?'convert': 'USD', 'limit': 10

I'm surprised that this works, but in any case the request.get function accepts the parameters as a second argument and does the correct thing:

>>> response = requests.get('https://api.coinmarketcap.com/v1/ticker/', params)
>>> print(response.url)
https://api.coinmarketcap.com/v1/ticker/?convert=USD&limit=10

Improved code

The function name could be improved and a docstring could be added to better describe what the function does.

The limit and convert parameters could be made into function arguments so that different parameters can be used without having to change the function definition each time.

The response status code could be checked in order to provide a more helpful error message in case the URL is wrong or the service isn't available, etc., instead of trying to process a meaningless result.

Assembling the return value can be simplified by using a dictionary comprehension.

def fetch_coin_prices(**kwargs):
 """Retrieve cryptocurrency data from CoinMarketCap and return a dictionary
 containing coin names with their current prices.

 Keyword arguments to this function are mapped to the CoinMarketCap API,
 refer to their documentation for their meaning:
 https://coinmarketcap.com/api/
 """
 response = requests.get(
 'https://api.coinmarketcap.com/v1/ticker/',
 params=kwargs
 )
 response.raise_for_status()
 coin_data = response.json()
 currency = kwargs.get('convert', 'USD')
 return 
 coin['id']: float(coin['price_'.format(currency.lower())])
 for coin in coin_data
 

The coins_arr function can be easily emulated by using the keys() of the dictionary returned by fetch_coin_prices:

>>> coins = fetch_coin_prices(limit=5)
>>> print(sorted(coins.keys()))
['bitcoin', 'bitcoin-cash', 'cardano', 'ethereum', 'ripple']

First things first, the convention is to use uppercase-initial for class names.

All modules, functions, classes and methods should have docstrings.

According to their API documentation, if limit is not specified, the default is 100 and by default convert is 'USD'. Your coins_arr() method is returning 100 coins, not 10, which I think is due to an incorrect formatting of the placeholder in the URL you are passing to requests.get(). You are splitting each coin and concatenating it to coinsData, which is not necessary since you just need to append it to the list.

The coins_dict() method is also returning 100 coins, not 10.

There is also some repetition in the two methods, which you could avoid.

Here I would write the class, like so:

import requests

class Coins:
 """This is a class for cryptocurrencies."""

 def __init__(self, url, limit=10):
 self.url = url 
 self.limit = limit
 self.data = requests.get(self.url.format(self.limit)).json()

 def get_coins(self):
 """Return a sorted list of cryptocurrencies.""" 

 coins = sorted(v for obj in self.data 
 for k, v in obj.items() if k == 'id') # sorted() returns a list

 return coins

 def get_coins_prices(self):
 """Return a dictionary of cryptocurrencies and their prices.""" 

 coins_prices = obj[k]: obj['price_usd'] for obj in self.data
 for k, v in obj.items() if k == 'id'

 return coins_prices

if __name__ == '__main__':
 url = 'https://api.coinmarketcap.com/v1/ticker/?limit='

 obj_a = Coins(url)
 print(obj_a.get_coins())

 obj_b = Coins(url)
 print(obj_b.get_coins_prices())

 obj_c = Coins(url, limit=5) # Setting limit to 5 results
 print(obj_c.get_coins_prices())

The Python version used to test the above code is 3.6.

Hope that helps.

I suspect there is more, relevant code, than what I see here, so take my suggestions with a grain of salt.

Updated Organization

It sounds like you need both. I've had a similar problem before, where I needed an array and a dictionary. I don't write a lot of python, so I am going to write this in JSON. I would just put them on the same object. And let the client code call coins.arr or coins.dict.

var coins = 
 arr: [coin1, coin2, coin3],
 dict: 
 'coin1': coin1,
 'coin2': coin2,
 'coin3': coin3
 
;

Updated Optimization

I would be careful that the individual objects between arr and dict referred to the same objects in memory, and I would make sure to walk over your response from the server just once and perform the insertions into the array and dictionary at the same time.

Previous Organization

I would recommend considering using 'composure' if you are looking to reuse code between these objects. Possibly, let's see what else is going on.

Previous Optimization

• What are we using these dictionaries and arrays for?




• How are they being used?




• Do you really want to re-request the data fresh every time?

I suspect that it would be reasonable to request the data once, and store the data in a primary format (array, dictionary, or other) and then convert to the other when necessary. But I would need more information to make an educated guess there.

1. Classes should be in CamelCase - Coins, rather than coins.


2. You can merge the requests call into one function, so your code is DRY.


3. Don't use CamelCase for variables, coins_dict is better. I'd just use coins.


4. You loop through the entire of objects to find the key 'id'. Instead, if it's a dictionary, use dict.get.


5. 
   

   If you use collections.OrderedDict then you can store everything in one object, and extract the extra information out when you need too.

   
   
   

   However you don't even need to do this, as you don't care if your input to your array is ordered.

   
   



6. You use objects.items, but don't use the v, instead you use objects[k]. Don't do this.



7. Don't put a space between print and the parentheses.

And so I'd use something like:

import requests


class Coins:
 def _get_coins(self):
 params = 'limit': 10, 'convert': 'USD'
 return requests.get('https://api.coinmarketcap.com/v1/ticker/?%s' % params).json()

 def coins(self):
 coins = 
 null = object()
 for objects in self._get_coins():
 id_ = objects.get('id', null)
 if id_ is not null:
 coins[id_] = objects['price_usd']
 return coins

 @staticmethod
 def coins_it(coins):
 return sorted(
 v
 for value in coins.values()
 for v in value.split()
 )

d = Coins().coins()
print(d)
print(list(Coins.coins_it(d)))