Stock statement generator in Python

Question

I applied to a startup recently, as a coding exercise they asked me to produce the output below with the given input, specifically making it easy to understand, extend, and without performing any unnecessary actions. I emailed this solution and got no response. The code works but I suspect there are problems with the design. Any and all criticisms are welcome, anything you could possibly object to, if it's awful by all means please let me know.

input:

actions = [{'date': '1992/07/14 11:12:30', 'action': 'BUY', 'price': '12.3', 'ticker': 'AAPL', 'shares': '500'}, 
 {'date': '1992/09/13 11:15:20', 'action': 'SELL', 'price': '15.3', 'ticker': 'AAPL', 'shares': '100'}, 
 {'date': '1992/10/14 15:14:20', 'action': 'BUY', 'price': '20', 'ticker': 'MSFT', 'shares': '300'}, 
 {'date': '1992/10/17 16:14:30', 'action': 'SELL', 'price': '20.2', 'ticker': 'MSFT', 'shares': '200'}, 
 {'date': '1992/10/19 15:14:20', 'action': 'BUY', 'price': '21', 'ticker': 'MSFT', 'shares': '500'}, 
 {'date': '1992/10/23 16:14:30', 'action': 'SELL', 'price': '18.2', 'ticker': 'MSFT', 'shares': '600'}, 
 {'date': '1992/10/25 10:15:20', 'action': 'SELL', 'price': '20.3', 'ticker': 'AAPL', 'shares': '300'}, 
 {'date': '1992/10/25 16:12:10', 'action': 'BUY', 'price': '18.3', 'ticker': 'MSFT', 'shares': '500'}]
stock_actions = [{'date': '1992/08/14', 'dividend': '0.10', 'split': '', 'stock': 'AAPL'}, 
 {'date': '1992/09/01', 'dividend': '', 'split': '3', 'stock': 'AAPL'}, 
 {'date': '1992/10/15', 'dividend': '0.20', 'split': '', 'stock': 'MSFT'},
 {'date': '1992/10/16', 'dividend': '0.20', 'split': '', 'stock': 'ABC'}]

output:

On 1992年07月14日, you have:
 - 500 shares of AAPL at 12ドル.30 per share
 - 0ドル of dividend income
 Transactions:
 - You bought 500 shares of AAPL at a price of 12ドル.30 per share
On 1992年08月14日, you have:
 - 500 shares of AAPL at 12ドル.30 per share
 - 50ドル.00 of dividend income
 Transactions:
 - AAPL paid out 0ドル.10 dividend per share, and you have 500 shares
On 1992年09月01日, you have:
 - 1500 shares of AAPL at 4ドル.10 per share
 - 50ドル.00 of dividend income
 Transactions:
 - AAPL split 3 to 1, and you have 1500 shares
On 1992年09月13日, you have:
 - 1400 shares of AAPL at 4ドル.10 per share
 - 50ドル.00 of dividend income
 Transactions:
 - You sold 100 shares of AAPL at a price of 15ドル.30 per share for a profit of 1120ドル.00
On 1992年10月14日, you have:
 - 1400 shares of AAPL at 4ドル.10 per share
 - 300 shares of MSFT at 20ドル.00 per share
 - 50ドル.00 of dividend income
 Transactions:
 - You bought 300 shares of MSFT at a price of 20ドル.00 per share
On 1992年10月15日, you have:
 - 1400 shares of AAPL at 4ドル.10 per share
 - 300 shares of MSFT at 20ドル.00 per share
 - 110ドル.00 of dividend income
 Transactions:
 - MSFT paid out 0ドル.20 dividend per share, and you have 300 shares
On 1992年10月17日, you have:
 - 1400 shares of AAPL at 4ドル.10 per share
 - 100 shares of MSFT at 20ドル.00 per share
 - 110ドル.00 of dividend income
 Transactions:
 - You sold 200 shares of MSFT at a price of 20ドル.20 per share for a profit of 40ドル.00
On 1992年10月19日, you have:
 - 1400 shares of AAPL at 4ドル.10 per share
 - 600 shares of MSFT at 20ドル.83 per share
 - 110ドル.00 of dividend income
 Transactions:
 - You bought 500 shares of MSFT at a price of 21ドル.00 per share
On 1992年10月23日, you have:
 - 1400 shares of AAPL at 4ドル.10 per share
 - 110ドル.00 of dividend income
 Transactions:
 - You sold 600 shares of MSFT at a price of 18ドル.20 per share for a loss of $-1578.00
On 1992年10月25日, you have:
 - 1100 shares of AAPL at 4ドル.10 per share
 - 500 shares of MSFT at 18ドル.30 per share
 - 110ドル.00 of dividend income
 Transactions:
 - You sold 300 shares of AAPL at a price of 20ドル.30 per share for a profit of 4860ドル.00
 - You bought 500 shares of MSFT at a price of 18ドル.30 per share

my code:

all_actions = actions + stock_actions
class Portfolio:
 ''' A portfolio of stocks. '''
 def __init__(self):
 ''' (Portfolio) -> NoneType
 Initialize a new portfolio object.
 '''
 self.stocks = [];
 self.dividend_income = "0";
 def buy_stock(self, stock, shares, price):
 ''' (Portfolio, str, str, str) -> str
 Buy shares number of shares of stock at price and return a string representation
 of the transaction for record-keeping purposes.
 '''
 if self.get_stock(stock) == None:
 stock_to_buy = {}
 stock_to_buy['stock'] = stock
 stock_to_buy['shares'] = shares
 stock_to_buy['value'] = "{0:.2f}".format(float(price))
 self.stocks.append(stock_to_buy)
 else:
 stock_to_buy = self.get_stock(stock)
 stock_to_buy['value'] = "{0:.2f}".format((((float(stock_to_buy['value']) * int(stock_to_buy['shares'])) + (float(price) * int(shares))) / (int(stock_to_buy['shares']) + int(shares))))
 stock_to_buy['shares'] = str(int(stock_to_buy['shares']) + int(shares))
 return "You bought " + shares + " shares of " + stock + " at a price of $" + "{0:.2f}".format(float(price)) + " per share"
 def sell_stock(self, stock, shares, price):
 ''' (Portfolio, str, str, str) -> str
 Sell shares number of shares of stock at price and return a string representation
 of the transaction for record-keeping purposes.
 '''
 stock_to_sell = self.get_stock(stock)
 stock_to_sell['shares'] = str(int(stock_to_sell['shares']) - int(shares))
 transaction_balance = (float(price) - float(stock_to_sell['value'])) * int(shares)
 result = "loss" if (transaction_balance < 0) else "profit"
 return "You sold " + shares + " shares of " + stock + " at a price of $" + "{0:.2f}".format(float(price)) + " per share for a " + result + " of $" + "{0:.2f}".format(float(transaction_balance))
 def pay_dividends(self, stock, dividend):
 ''' (Portfolio, str, str) -> str
 Pay out dividends on stock at a rate of dividend per share and return a string representation of the transaction for record-keeping purposes.
 '''
 stock_paying_dividends = self.get_stock(stock)
 self.dividend_income = "{0:.2f}".format(float(self.dividend_income) + int(stock_paying_dividends['shares']) * float(dividend))
 return stock + " paid out $" + "{0:.2f}".format(float(dividend)) + " dividend per share, and you have " + stock_paying_dividends['shares'] + " shares"
 def split_stock(self, stock, split):
 ''' (Portfolio, str, str) -> str
 Split stock split number of ways and return a string representation of the transaction for record-keeping purposes.
 '''
 stock_being_split = self.get_stock(stock)
 stock_being_split['shares'] = str(int(stock_being_split['shares']) * int(split))
 stock_being_split['value'] = "{0:.2f}".format((float(stock_being_split['value']) / int(split)))
 return stock + " split " + split + " to 1, and you have " + stock_being_split['shares'] + " shares"
 def execute_action(self, a):
 ''' (Portfolio, str) -> str 
 Execute the specified transaction on the Portfolio, and return a string representation of that transaction for record-keeping purposes.
 '''
 if a in actions: # a is a trader action
 if(a['action'] == 'BUY'):
 return self.buy_stock(a['ticker'], a['shares'], a['price'])
 else:
 return self.sell_stock(a['ticker'], a['shares'], a['price'])
 else: # a is a stock action
 if(a['dividend'] != ''):
 return self.pay_dividends(a['stock'], a['dividend'])
 else:
 return self.split_stock(a['stock'], a['split'])
 def print_portfolio(self, date):
 ''' (Portfolio, str) -> NoneType
 Print a representation of the current state of the Portfolio object.
 '''
 print("On " + date.replace('/', '-') + ", you have:")
 for stock in self.stocks:
 if stock['shares'] != 0: # ignore stocks with newly depleted shares
 print(" - " + stock['shares'] + " shares of " + stock['stock'] + " at $" + stock['value'] + " per share")
 print(" - $" + str(self.dividend_income) + " of dividend income")
 def generate_statement(self):
 ''' (Portfolio) -> NoneType
 Generate and print a stock statement based on the actions and stock_actions
 given in the lists at the top of this file.
 '''
 dates = [] # for storing list of all dates on which actions occur
 for a in all_actions:
 if a['date'][0:10] not in dates:
 dates.append(a['date'][0:10])
 dates.sort()
 for d in dates:
 daily_actions = self.get_daily_transactions(d)
 transaction_records = [] # list of strings for recording transactions
 if daily_actions: # Ensure that daily_actions is not empty
 for a in daily_actions:
 if self.is_valid_transaction(a):
 transaction_records.append(self.execute_action(a)) 
 for stock in self.stocks: # Remove stocks whose shares are now depleted
 if stock['shares'] == '0':
 self.stocks.remove(stock) 
 if transaction_records: # Ensure that transaction_records is not empty
 self.print_portfolio(d)
 print(" Transactions:")
 for record in transaction_records:
 print(" - " + record)
 def get_daily_transactions(self, date):
 ''' (Portfolio, str) -> list of dict of {str: str}
 Return a list of the transactions that are scheduled to take
 place on date.
 '''
 daily_actions = []
 for a in all_actions:
 if a['date'].startswith(date):
 daily_actions.append(a)
 return daily_actions;
 def is_valid_transaction(self, action):
 ''' (Portfolio, dict of {str: str}) -> bool
 Return whether or not action is a valid transaction. Invalid transactions
 are pay dividends or split actions occuring on stocks that do not
 currently exist in the Portfolio object, or sell actions in cases where there are not enough shares of the stock in the Portfolio to sell.
 '''
 stock_in_portfolio = False;
 if action in actions:
 if action['action'] == 'SELL':
 for s in self.stocks:
 if s['stock'] == action['ticker']:
 stock_in_portfolio = True; 
 if not stock_in_portfolio:
 return False;
 if int(action['shares']) > int(self.get_stock(action['ticker'])['shares']):
 return False; 
 else:
 for s in self.stocks:
 if s['stock'] == action['stock']:
 stock_in_portfolio = True; 
 if not stock_in_portfolio:
 return False; 
 return True; 
 def get_stock(self, stock_name):
 ''' (Portfolio, str) -> dict of {str: str}
 Return the stock with name stock_name in this Portfolio.
 '''
 for s in self.stocks:
 if s['stock'] == stock_name:
 return s
 return None
if __name__ == '__main__':
 p = Portfolio()
 p.generate_statement()

Answer 1

The code generally looks all right – you can definitively program. However, there are many details that show that you're fairly new to Python. Some potential problems are fairly minor, but could indicate that you are not an experienced software developer.

The most pressing problem is that you are using strings and dictionaries for everything. For many of these cases you should use float or int or a custom object instead. Because you only use strings you convert your values repeatedly, all over the place, which clutters your code. Anything else in your code is fairly forgiveable and can be corrected quickly, but this here really sticks out.

The fix here is to separate input from your domain model from your output. The domain model represents the things that are actually interesting for your program, like stocks and trading actions. These should be represented in a way that are convenient for the problems you are solving. Usually, this means creating a class for each concept you are modelling.

When data is loaded into your program, you convert the data to your domain model at the application boundary. When you output data, you convert the domain model to the required data format – again, only at the application boundary, not in the middle of things.

Your code lacks this architecture. One could even say that your code lacks any architecture. You have a single class that does everything. Except for the instance fields stocks and dividend_income, your methods would work just as well as free functions, no class needed.

---

Classes should inherit from object. Instead of class Portfolio, I'd expect to see class Portfolio(object).

Minor: All your docstrings have leading spaces. This is unnecessary.

Minor: All your function docstrings contain a type signature. According to the PEP-257 docstring conventions, the first line should contain a short summary of the description of the behaviour:

The one-line docstring should NOT be a "signature" reiterating the function/method parameters (which can be obtained by introspection). Don't do:

def function(a, b):
 """function(a, b) -> list"""

This type of docstring is only appropriate for C functions (such as built-ins), where introspection is not possible. However, the nature of the return value cannot be determined by introspection, so it should be mentioned. The preferred form for such a docstring would be something like:

def function(a, b):
 """Do X and return a list."""

(Of course "Do X" should be replaced by a useful description!)

In any case, documenting the type of the self parameter to a method is misleading. Since no such parameter is explicitly passed when calling a method, it's best to ignore self in the docs.

Don't use == to compare with None. Instead of equality, test for identity with is, i.e. if x is None: ....

You are using dictionaries as ad-hoc objects. This is sometimes OK, but generally makes the code more difficult to maintain. It is easy to accidentally introduce a bug by mistyping one of the keys. Instead, define a simple class. Convert the input data to objects of this class first. This also avoids having to convert the strings to ints or floats for each calculation, thus reducing clutter.

This line is unacceptable:

stock_to_buy['value'] = "{0:.2f}".format((((float(stock_to_buy['value']) * int(stock_to_buy['shares'])) + (float(price) * int(shares))) / (int(stock_to_buy['shares']) + int(shares))))

For starters, it's too long. This is not just problematic because long lines are more difficult to read, but because this line does too much. I've already mentioned that the conversions should be done once when loading the input data. Once we do that, this is simplified to

stock_to_buy['value'] = "{0:.2f}".format((((stock_to_buy['value'] * stock_to_buy['shares']) + (price * shares)) / (stock_to_buy['shares'] + shares)))

But the high number of parentheses (((( is still quite difficult to read. Some of those parentheses are entirely unnecessary. If they don't contribute to clarity, they should be omitted. But more importantly, you should extract part of the calculation into separate variables with self-documenting names. Each line should do just one thing. How these variables should be named depends on the problem domain; since I'm not familiar with shares I'll not suggest anything here.

You are using + to concatenate strings. Instead, use the .format() method to insert data into a template. This

"You bought " + shares + " shares of " + stock + " at a price of $" + "{0:.2f}".format(float(price)) + " per share"

should be

"You bought {shares} shares of {stock} at a price of ${price:.2f} per share".format(
 shares=shares,
 stock=stock,
 price=float(price))

Note that with many arguments to format(), using named placeholders makes the code more easy to read.

If you need comments to explain what a variable means, you should try to find a better name. The comment # a is a trader action clearly shows that a should be called trader_action or action instead. Single-letter names are usually a bad idea, except for extremely obvious cases like x, y, i.

Don't use parentheses in a conditional: if(a['action'] == 'BUY'): should be if a['action'] == 'BUY':.

You are mixing business logic with output and string formatting. This is not a good idea: it makes the business logic more difficult to see among all that string handling, and it means you'll have to touch the business logic if you'd have to change the output format in the future. This goes against your requirement to make the code easy to extend.

The generate_statement() function has a fairly high "cyclomatic complexity": there are many nested loops and conditionals. Some of these are processing data, others are generating output. You can simplify this by separating those parts into separate functions or classes.

You occasionally use semicolons ; to end a Python statement. While this is technically allowed, this is never necessary in good Python programs. Therefore: do not use semicolons in Python, at all.

The top-level invocation looks like this:

p = Portfolio()
p.generate_statement()

Why is this a problem?

If a class only has a single public method that should be invoked, you should just use a plain function instead. Your Portfolio object provides no benefit over simply calling a function generate_statement().

More importantly, this function takes no arguments and returns nothing. Instead, it implicitly finds its input in a global variable and prints the output to the console. The input data should be passed as a function argument. In real code, I wouldn't be interested in running the same analysis for the same input again and again. I'd want to create a portfolio for many different data sets. Therefore, using a global variable for input is not a good idea.

---

How can you get better at this?

First, install Pylint. This tool is like an automated code review and is really good at finding all kinds of issues in your code. I use it all the time. It is highly configurable so you can deactivate warnings that are not applicable to your circumstances. Run Pylint on your code and try to improve it until Pylint is happy.

Next, try to get better at design. Your coding is quite good, and I'm sure your design skills will improve rather fast. I've talked about a couple of points above. Maybe you'd like to read more about the idea of an "domain model". You will definitively want to separate the data loading, the action processing, and the output from each other. It might be helpful to think about the data flow in your code, and how it can be structured. Try to use classes instead of dictionaries.

This is a lot to process. Once you've fixed the smaller issues and have run Pylint and have tried to improve the design of the code, please ask another question here with your improved code. I'd love it if you could notify me when you've done that by dropping a comment on my answer here.

• Max B: Thanks for the in-depth answer! I will definitely take a look at Pylint, the PEP-257 conventions and the idea of the 'domain model' you mentioned a few times. Before I was asked to code this I actually hadn't coded in Python in a few years, so things like the unnecessary semicolons and parentheses in conditionals are probably because I've been coding mostly in Java and C since then and forgot the proper Python syntax. In any case, thanks again for your feedback, I'll definitely let you know and repost once I've improved my code.