Make changes to long-short.py to make it more Pythonic and beginner friendly #425
Conversation
…riendly As an example, it was destructive for a beginner to the Python language. This does not change the core design which is still flawed, but it allows the code to run fully. Summary of changes: - 4 spaces instead of 2 (PEP8: https://www.python.org/dev/peps/pep-0008/) - camelCase to snake_case (PEP8) - Remove parentheses from if statements (PEP8) - Add some type hints where useful (syntax) - Avoid using un-imported pandas (pd) library to create timestamps, instead use proper datetime.date (syntax) - Remove all threading usage as it is all unnecessary when threads are immediately joined (design) - Remove all passing of lists to functions to retrieve values and instead use return statements for said values (design) - Add URL wrapping on API initialization to comply with the type hints of the library (misc)
| order_side = 'sell' | ||
| else: | ||
| order_side = 'buy' | ||
| qty = abs(int(float(position.qty))) |
There was a problem hiding this comment.
Can you please explain why we need the float conversion here?
There was a problem hiding this comment.
There are a lot more similar double int(float(...)) conversions throughout the file.
| bars = self.alpaca.get_bars(stock[0], TimeFrame.Minute, | ||
| datetime.date.today().isoformat(), | ||
| datetime.date.today().isoformat(), limit=length, | ||
| adjustment='raw') |
There was a problem hiding this comment.
This will return the first 10 minute bars of today for the given symbol, instead of the last 10 minutes. I know it's also broken in the original but please fix it.
| datetime.date.today().isoformat(), | ||
| datetime.date.today().isoformat(), limit=length, | ||
| adjustment='raw') | ||
| self.all_stocks[i][1] = (bars[stock[0]][len(bars[stock[0]]) - 1].c - |
There was a problem hiding this comment.
[minor] You can simply use -1
| self.all_stocks[i][1] = (bars[stock[0]][len(bars[stock[0]]) - 1].c - | |
| self.all_stocks[i][1] = (bars[stock[0]][-1].c - |
| tSO = threading.Thread(target=self.submitOrder, args=[abs(int(float(position.qty))), position.symbol, side, respSO]) | ||
| tSO.start() | ||
| tSO.join() | ||
| if self.long.count(position.symbol) == 0: |
There was a problem hiding this comment.
[minor]
| if self.long.count(position.symbol) == 0: | |
| if position.symbol not in self.long.count(position.symbol): |
|
|
||
| resp_get_tp_short = self.get_total_price(self.short) | ||
|
|
||
| self.q_long = int(self.long_amount // resp_get_tp_long) |
There was a problem hiding this comment.
[minor] no need for the int conversion after //
| total_price = 0 | ||
| for stock in stocks: | ||
| bars = self.alpaca.get_bars(stock, TimeFrame.Minute, | ||
| datetime.date.today().isoformat(), | ||
| datetime.date.today().isoformat(), limit=1, | ||
| adjustment='raw') | ||
|
|
||
| total_price += bars[stock][0].c | ||
| return total_price |
There was a problem hiding this comment.
While this logic is correct, I think using the latest trade price is much more accurate. The price can move a lot since the last minute bars closing price.
| total_price = 0 | |
| for stock in stocks: | |
| bars = self.alpaca.get_bars(stock, TimeFrame.Minute, | |
| datetime.date.today().isoformat(), | |
| datetime.date.today().isoformat(), limit=1, | |
| adjustment='raw') | |
| total_price += bars[stock][0].c | |
| return total_price | |
| return sum(self.alpaca.get_latest_trade(stock).price for stock in stocks) |
| self.blacklist.add(position.symbol) | ||
|
|
||
| # Send orders to all remaining stocks in the long and short list. | ||
| resp_send_bo_long = self.send_batch_order(self.q_long, self.long, "buy") |
There was a problem hiding this comment.
[minor] tuple unpacking would be much nicer:
| resp_send_bo_long = self.send_batch_order(self.q_long, self.long, "buy") | |
| executed, incomplete = self.send_batch_order(self.q_long, self.long, "buy") |
There was a problem hiding this comment.
This PR is amazing, thank you very much! 🥇
I agree it's unfortunately not complete, but it's a huge step to the right direction. Still, I left some comments on issues I feel should be included, especially the ones that make the implementation incorrect (e.g. querying the first 10 minute bars of the day instead of the latest 10 minute bars).
As an example, it was destructive for a beginner to the Python language. This does not change the core design which is still flawed, but it allows the code to run fully.
Summary of changes:
This is by no means a complete fix of all issues.