8
\$\begingroup\$

I wrote this in the last hour because I was bored. I was hoping I could get some feedback on this. It is a function that returns the price of a bitcoin and whether you should buy or sell it. I am thinking of setting it up as a function within a chat bot to spice up the chat a little.

import urllib2
import random
import json



def bitcoin(currency, amt = 1):
    url = "https://api.coindesk.com/v1/bpi/currentprice.json"
    request = urllib2.Request(url)
    response = urllib2.urlopen(request)
    the_page = response.read()
    data = json.loads(the_page)
    if currency in ["USD","US","dollar","$"]:
        conversion = data['bpi']['USD']['rate_float']
        symbol = "USD"
    elif currency in ["EUR","EU","euro","€"]:
        conversion = data['bpi']['EUR']['rate_float']
        symbol = "EUR"
    elif currency in ["GBP","GB","UK","pound","£"]:
        conversion = data['bpi']['GBP']['rate_float']
        symbol = "GBP"
    else:
        stop("Currency not recognized.")
    advice = random.choice(["It's so LOW!!!!!!! \r\nBuy!!! Buy!!! Buy!!!!!","It's so HIGH!!!!!! \r\nSell!!! Sell!!! Sell!!!!!"])
    return "{0} Bitcoin = {1} {2} \r\n{3}".format(amt,conversion * amt, symbol, advice)
Improve this question
\$\endgroup\$
3
  • 1
    \$\begingroup\$ the amt variable designates the amount of bitcoins to convert. This fact is not clear and I had to look into the string formatting in the last line to understand that. I suggest you either give it a more significant name or add a documentation. Also, what does stop do? \$\endgroup\$
    – njzk2
    Jan 9, 2016 at 4:55
  • \$\begingroup\$ Good points. Stop was R. Should be return. Would a comment '''amt=amount of bitcoin''' or a name "amount_bitcoin" be better? \$\endgroup\$
    – polka
    Jan 9, 2016 at 5:19
  • 1
    \$\begingroup\$ I always tend to favor self-explanatory variable names, but it is more a matter of preference \$\endgroup\$
    – njzk2
    Jan 10, 2016 at 1:40

2 Answers 2

Reset to default
11
\$\begingroup\$

This is Schrödinger code

It can be both correct and incorrect at the same time.

random.choice is not how we offer financial advice.

What I would suggest is to monitor previous data, the longer the better, and monitor the fluctuation in price.

See this graph (Note, a 5 year graph):

enter image description here

at two points on the chart you can see separate events that can be somewhat predicted by the speed at which it falls/rises in comparison to previous events.

Although this could potentially expand to be a large solution, it's about as close to accurate as you can get. (Not really, but I'm not a market analyst)

These guys are, though.

Note that the graph is 5 years; data doesn't fluctuate that much in small amounts.

Although Bitcoin can, given it's financially unstable and volatile nature.

Beyond that, your python code is nice :-)

I would suggest writing it to PEP8 however, as that is the official style guide of Python, and then you can use an online checker like pep8online.

Improve this answer
\$\endgroup\$
2
  • 4
    \$\begingroup\$ TIL, what "Schrödinger code" is, what "PEP8" is, and how to be a "market analyst". Thank You! \$\endgroup\$
    – polka
    Jan 9, 2016 at 1:06
  • 8
    \$\begingroup\$ random.choice is not how we offer financial advice. citation needed \$\endgroup\$
    – njzk2
    Jan 9, 2016 at 4:50
2
\$\begingroup\$

You should split your code into smaller functions, and give them significant names.

For instance

def bitcoin(currency, bitcoin_amount = 1):
    currency = normalize_symbol(currency) # returns EUR, USD, GBP or raises Error
    bitcoin_value = bitcoin_value_in(currency) # may raise an Error if currency is unsupported
    currency_amount = bitcoin_amount * bitcoin_value
    advice = random.choice(["It's so LOW!!!!!!! \r\nBuy!!! Buy!!! Buy!!!!!","It's so HIGH!!!!!! \r\nSell!!! Sell!!! Sell!!!!!"])
    return "{0} Bitcoin = {1} {2} \r\n{3}".format(bitcoin_amount, currency_amount, currency, advice)

normalize_symbol can be implemented with a dict of list: {'EUR': ['EUR', '€'] ...}

bitcoin_value_in is just a refactorization (move the url...data code into it)

advice has already been studied by @Quill.

Improve this answer
\$\endgroup\$
2
  • \$\begingroup\$ okay, so implementing the dict of list.. you have to return the key if the currency is in the list... I am thinking you have to use a list comprehension to do that... currency = [ki for ki in currencies.keys()if currency in currencies[ki]] ... but that returns a list and it searches the whole list for matches and if it finds a match immediately, then it doesn't stop. How would you recommend implementing this? \$\endgroup\$
    – polka
    Jan 12, 2016 at 0:40
  • 1
    \$\begingroup\$ As you mentioned, list comprehension will create a list. You should do a for symbol, abbreviations in currencies.items():. If currency is in the abbreviations, then you return the symbol. If the loop terminates, you should raise an exception. \$\endgroup\$
    – oliverpool
    Jan 12, 2016 at 8:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.