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

code refactoring #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

code refactoring #1

wants to merge 1 commit into from

Conversation

lorenzua02
Copy link

Hello, this PR is a refractor of your code. That means it won't change the behaviour of the code, but the code quality will be improved: writing nice code is important to collaborate with other people :)

I suppose you are new to python: for this reason, I am writing here some "mistakes" you've made with the solution & explanation. Hit me up for any question!


if status['status'] == 0:
    return True
else:
    return False

This part status['status'] == 0 is a boolean value already! You can simply return it. learn more

return status['status'] == 0

IMPORTANT: dictionary as default parameter

Dictionary as default values in functions might not work as you except. This is a serious issue!

If you change the dictionary, it stays like that also in the following calls.

I highly suggest to check out the "Default Parameter Values in Python" link you find on this question on stackoverflow.


open() has a context manager: it's suggested to perform file operations using it. In other words, I've replaced your old code...

f = open(...)
# something...
f.close()

...with the same thing, using context manager

with open(...) as f:
    # something... (file will be automatically closed)

Some lines could have be inserted in a for loop.

# old
dataframe_copy = ta_sma.calc_ta_sma(dataframe_copy, 5)
dataframe_copy = ta_sma.calc_ta_sma(dataframe_copy, 8)
dataframe_copy = ta_sma.calc_ta_sma(dataframe_copy, 15)
dataframe_copy = ta_sma.calc_ta_sma(dataframe_copy, 20)
dataframe_copy = ta_sma.calc_ta_sma(dataframe_copy, 50)
dataframe_copy = ta_sma.calc_ta_sma(dataframe_copy, 200)

# new
for val in [5, 8, 15, 20, 50, 200]:
    df = ta_sma.calc_ta_sma(df, val)

You should comment the meaning of the code, not the actual line! A quick example of comments I've deleted:

# Return updated order dataframe
return updated_order_dataframe

About if: Instead of using just the else part, you can reverse the condition, quick example

old:

if something:
    pass
else:
    # code

new:

if not something:
    # code

It's really a common practice (even in its documentation) to import pandas as pd and import numpy as np


In functions, you should totally avoid nested conditions & returns!
Old code:

if first_condition:
    if second_condition:
        if third_condition:
            return 0
        else:
            raise MyAwesomeException
    else:
        return -2
else:
    return -1

Refractored, more readable, new code:

if not first_condition:
    return -1
   
if not second_condition:
    return -2

if not third_condition:
    raise MyAwesomeException

return 0  # <- Gets here in case it's all good with the conditions above

If there is some code at the end that will get excecuted in any scenario, you can move it ouside. Example from your (old) code:

if paper:
    insert_paper...
    return True
else:
    insert_live...
    return True

new:

if paper:
    insert_paper...
else:
    insert_live...
   
return True

Tests in python should be like tests/test_trade_capture.py file, please move all the tests (functions) in a class Something(unittest.TestCase)


pythonic ways to do things

Old New
if x == True if x
if x == None if is None
x = x * (-1) x *= -1
if x == "a" or x == "b" or x == "c" if x in ["a", "b", "c"]
range(0, n) range(n)

Replace

if var == "M1":
    result = "foo"
elif var == "H6":
    result = "bar"
else:
    raise Exception

with

xyz = {"M1": "foo", "H6": "bar"}
# if var not in xyz:
#    raise Exception
result = xyz[var]

(or upgrade to python 3.10 and use the match)


You don't have to create SQL string manually to handble database-related stuffs, there are more pythonic ways to do that.

Check PEP8 style guide

There is a lot of duplicated code you can improve, maybe move it in a function and call it every time you need it. One function call is better then the same 5 lines everytime!

test and "real" API_KEYS (& info) should be changed in an .env file or something like that. The script shouldn't even know which is the current enviroment. You could have a PROD_ENV and a TEST_ENV files and just select what env file to use. There are a lot of options tho! .env library


I hope everything makes sense to you, feel free to comment on this PR and we will discuss :)

See you soon,

Lorenzo

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.

1 participant