# Stock statement generator in Python

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()  ## 1 Answer 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.