8
\$\begingroup\$

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()
\$\endgroup\$

1 Answer 1

Reset to default
15
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Feb 19, 2017 at 15:43

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.