13
\$\begingroup\$

A couple of weeks ago I had to do a coding challenge for a job interview. I did the challenge using Python, the code was working and I got the job. But I would like to know how (if possible) I can improve/refactor the code I've written. The reviewer told my future employer there were too many if statements and that I could have written better code. I guess this is an example of too many if statements:

def get_row_tax_percentage(product_name: str):
    exempt = is_exempt(product_name)
    imported = is_imported(product_name)
    if exempt and imported:
        return Decimal(".05")
    elif exempt and not imported:
        return Decimal("0")
    elif imported:
        return Decimal(".15")
    else:
        return Decimal(".1")

Link to the task description: https://github.com/xpeppers/sales-taxes-problem

Link to my solution: https://github.com/soulpaul/sales-taxes-solution

I think this could be a chance to learn something new. I don't need you guys to write code for me, but please point me to the right direction. Thanks to anyone who is going to spend some time on this.

\$\endgroup\$
5
  • 2
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$ – BCdotWEB Apr 22 at 7:59
  • 2
    \$\begingroup\$ Welcome to the Code Review Community and congrats on the new job. In the future please copy the programming challenge into the question so that we understand the requirements. The full code that you want reviewed needs to be embedded in the question, we can use repositories for references, but we can't review code in them. \$\endgroup\$ – pacmaninbw Apr 22 at 11:34
  • \$\begingroup\$ Just curious: Why f"{word.lower()}" instead of word.lower() here? \$\endgroup\$ – Manuel Apr 23 at 11:55
  • 1
    \$\begingroup\$ There's 2 things I don't like about the task, that would probably require it to be different in production code. First I wouldn't want to hard code the values into the code, they should be external to the code. Secondly the production code should probably be able to handle additional taxes (which is what I assume they are). That is their mistake and not your mistake. But if they expected you to ask about it, or they thought it was common sense, it could explain why they complained about the amount of ifs. \$\endgroup\$ – Polygorial Apr 23 at 19:10
  • \$\begingroup\$ @Manuel honestly... it doesn't make sense to me as well. It's probably something due to some kind of refactoring. It's just bad code. \$\endgroup\$ – soulpaul Apr 24 at 15:32
20
\$\begingroup\$

That really depends on how the task is stated. If it's stated like your code, just in English, with four different (and possibly independent) values, then I'd say your way is actually ok. Because then if one of them changes, you only need to change the value, not rewrite logic.

But the task description says:

Basic sales tax is applicable at a rate of 10% on all goods, except books, food, and medical products that are exempt. Import duty is an additional sales tax applicable on all imported goods at a rate of 5%, with no exemptions.

So it is in fact stated as two separate taxes that should be determined separately and just added. Make the code's logic reflect the real logic.

For extra clarity, use the proper names as well. For example:

def get_row_tax_percentage(product_name: str):
    basic_sales_tax = 10 if not is_exempt(product_name) else 0
    import_duty = 5 if is_imported(product_name) else 0
    return Decimal(basic_sales_tax + import_duty) / 100

Or maybe with 0 if is_exempt(product_name) else 10, I can't decide which I find clearer and/or closer to how the task says it.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I omitted to write the task description because it was way too long to include. I should have added a reference, you are right, especially since I was looking for more feedback on the other part of the code as well. Thanks for your time and your precious answer! \$\endgroup\$ – soulpaul Apr 22 at 7:33
14
\$\begingroup\$

I'm confused why all the answers are so fond of the ternary operator (Y if X else Z) rather than simple if statements:

def get_row_tax_percentage_calc(product_name: str):
    basic_sales_tax = Decimal('0.1')
    import_duty = Decimal('0.05')

    total = Decimal('0')
    if not is_exempt(product_name):
        total += basic_sales_tax
    if is_imported(product_name):
        total += import_duty
    return total

Coming from C++, I find python's ternary syntax particularly hard to parse, as the natural language order does not follow the flowchart logic.

EDIT: thanks to @GregoryCurrie for the suggestion to make the magic numbers into named constants.

\$\endgroup\$
7
  • \$\begingroup\$ Ternary statements are less code and arguably easier to read. That's not to say that IF statements are usually hard to read to begin with, but ternary is even easier to parse. Even in C#, I use ternary wherever it is applicable. It's an IF-THEN-ELSE all on one line if you please rather than what is usually going to be a minimum of four lines. width = width == 0 ? 5 + 2 : width + 2 versus if (width == 0) <New Line> width = 5 + 2 <New Line> else <New Line> width = width + 2 \$\endgroup\$ – Hashgrammer Apr 22 at 16:27
  • 3
    \$\begingroup\$ @Hashgrammer How is result += 5 if is_imported(product_name) else 0 less code or easier to read than if is_imported(product_name): result += 5? \$\endgroup\$ – Manuel Apr 22 at 16:59
  • 3
    \$\begingroup\$ If I may suggest an improvement, pull out Decimal("0.1") and Decimal("0.05") into two constants with the name of the tax involved. In any case, this is certainly the clearest answer. \$\endgroup\$ – Gregory Currie Apr 22 at 17:17
  • 3
    \$\begingroup\$ @Hashgrammer I totally agree ternary operators are valuable and have their place, but in this case the else condition is a no-op and can be omitted, making the plain if statement both less verbose and in my opinion more readable \$\endgroup\$ – thegreatemu Apr 22 at 22:20
  • 1
    \$\begingroup\$ Useful to cite the discussion of pros and cons of using Python conditional expressions (aka 'the ternary operator') from Does Python have a ternary conditional operator? \$\endgroup\$ – smci Apr 23 at 19:50
13
\$\begingroup\$

Calcuating the result

The approach they might be looking for is the following:

def get_row_tax_percentage_calc(product_name: str):
    result = 0 if is_exempt(product_name) else 10
    result += 5 if is_imported(product_name) else 0

    return Decimal(result) / Decimal(100)

As you can see we're able to calculate the result without hardcoding all possible combinations of product characteristics. This approach is more maintainable: If we need to consider another characteristic of the product for our tax percentage, it's only a matter of adding a single line:

def get_row_tax_percentage_calc(product_name: str):
    result = 0 if is_exempt(product_name) else 10
    result += 5 if is_imported(product_name) else 0
    result += 7 if fits_new_characteristic(product_name) else 0

    return Decimal(result) / Decimal(100)

I would generally not recommend the following suggestions in an interview or at all, but they are some neat tricks to know:


We can also multiply the bool values (False -> 0 - True -> 1) with our surcharges to get this really simple implementation:

def get_row_tax_percentage_calc(product_name: str):
    result = 10 * (not is_exempt(product_name))
    result += 5 * is_imported(product_name)

    return Decimal(result) / Decimal(100)

We can also use the bool values from is_exempt and is_imported as indices (again, False -> 0 - True -> 1).

def get_row_tax_percentage_calc(product_name: str):
    result = [10, 0][is_exempt(product_name)]
    result += [0, 5][is_imported(product_name)]

    return Decimal(result) / Decimal(100)

or

def get_row_tax_percentage_calc(product_name: str):
    result = [0, 10][not is_exempt(product_name)]
    result += [0, 5][is_imported(product_name)]

    return Decimal(result) / Decimal(100)
\$\endgroup\$
15
  • 6
    \$\begingroup\$ i would strongly recommend against 10 * bool in an interview or production either, it might be .1% faster but is a very unexpected and unusual use of * \$\endgroup\$ – cat Apr 22 at 2:33
  • 8
    \$\begingroup\$ @cat: I guess it depends who reads it. If everyone else on the team has C experience, go for it! It feels natural, and exposes the logic well. I personally find [0, 10][not is_exempt(product_name)] harder to parse. \$\endgroup\$ – Eric Duminil Apr 22 at 6:38
  • 3
    \$\begingroup\$ +1 for suggesting the 10*bool approach. \$\endgroup\$ – Jack Aidley Apr 22 at 7:32
  • 4
    \$\begingroup\$ There is some clever code here but if readability is a key focus I would avoid your last few examples. The first one is good, though. \$\endgroup\$ – rshepp Apr 22 at 7:48
  • 3
    \$\begingroup\$ @Dakkaron The problem statement shows that the underlying logic is not a matrix of characteristics, but the summation of independent surcharges. \$\endgroup\$ – riskypenguin Apr 22 at 16:55
3
\$\begingroup\$

The reviewer was probably thinking that you can get the same result by adding together two independently-calculated surcharges:

def get_row_tax_percentage(product_name: str) -> Decimal:
    nonexempt = Decimal("0") if is_exempt(product_name) else Decimal(".05")
    imported = Decimal("0.10") if is_imported(product_name) else Decimal("0")
    return nonexempt + imported

There are other ways to write this, of course, but this seems clear to me: there is a surcharge for non-exempt items and another surcharge for imported items and the total percentage is the sum of both.

Note that in your original code, and not imported is redundant.

You annotated the type of product_name but not the return type of the function; -> Decimal should work.

\$\endgroup\$
4
  • 4
    \$\begingroup\$ Tangentially: I encourage you not to take negative interview feedback too seriously. The standard interview process, especially for low-experience positions, is a process of elimination, and therefore strongly relies on finding and magnifying flaws rather than identifying excellence. Small mistakes become big ones, and an interviewer may ding you for purely stylistic choices or for interpreting an ambiguous spec in an unexpected way. I've had two interviewers give me exactly opposite advice. Do accept constructive criticism, but don't put undue weight on vaguely negative feedback. \$\endgroup\$ – trentcl Apr 21 at 14:48
  • \$\begingroup\$ As a grizzled veteran from the interviewing trenches, I endorse your comment about the software engineering hiring process -- although I would not restrict it to low-experience positions. It's an imperfect process, to say the least. \$\endgroup\$ – FMc Apr 21 at 17:12
  • \$\begingroup\$ I don't take interviews feedback too seriously but I want to understand If it could help me become a better coder. That's why, despite having had success in getting the job, I'm willing to work on this. I still have a long road of learning ahead :) \$\endgroup\$ – soulpaul Apr 22 at 7:45
  • \$\begingroup\$ For real-world code, this would be pretty bad. Imagine, one of the values change, and then they cannot be added up that neatly. \$\endgroup\$ – Dakkaron Apr 22 at 16:46
0
\$\begingroup\$

Your code is simple, straightforward, easy to understand, and easy to change. It is absolutely fine and acceptable. Simple is good for code. It is not clever. I don’t want clever code.

Apparently your reviewer wants more clever code. That is bad news as far as getting that job goes, but your code is just fine. Some examples of more clever code have been posted. But I always see for example the assumption that the taxes for “non-exempt” and “imported” are just added; that is not obvious and could quite obvious change at any time; your government might decide that 15 percent for non-exempt AND imported goods is too low or too high, and instead of changing one line you have to change your code completely.

For example, the dictionary code is very clever. You could use it to impress that reviewer. I’d hate to see it in my codebase.

Summary: Your code is fine as it is. I like it. And most people would not complain about it.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Maybe for some others it was an assumption. For me it wasn't. It's in the task description. And I don't think that mindlessly converting English to Python is "clever". It's rather the opposite. It's trying not to be clever. Takes less brainpower and is less error-prone than calculating the four values and fumbling them into four ifs. \$\endgroup\$ – Manuel Apr 22 at 10:11
  • \$\begingroup\$ Actually... since the OP also just added the two taxes, you should complain about that just as much. Probably even more, since they hide it by having done that in the background and acting like those four values were given. \$\endgroup\$ – Manuel Apr 22 at 10:57
  • 1
    \$\begingroup\$ I also don't find the OP's "simple". For example to figure out what the 10% is for, you need to read, negate, and combine three different if-conditions. The second of which is even confusing, because the and not imported seems superfluous, so makes one wonder why that was included, and why the third if-condition doesn't include and not exempt . \$\endgroup\$ – Manuel Apr 22 at 11:04
0
\$\begingroup\$

Disclaimer: this answer was posted before the problem statement was posted. Only the code was there in the beginning. So the point of this answer was, if you see a possible pattern where there might not be a real pattern (e.g. the corners of the matrix add up, so use addition instead of four fields) is a bad idea, because what if the pattern breaks once requirements change

The code is good as it is. It is simple, straight-forward and decidedly not clever. That's exactly how production code should be.

The other answers here do indeed make the code shorter, but they make it a lot less maintainable and understandable. The only thing that could be reasonably improved is nested ifs, so that it's easier to see the matrix of results:

def get_row_tax_percentage(product_name: str):
    exempt = is_exempt(product_name)
    imported = is_imported(product_name)
    if exempt:
        if imported:
            return Decimal(".05")
        else:
            return Decimal("0")
    else:
        if imported:
            return Decimal(".15")
        else:
            return Decimal(".1")

That said, if the goal is to make a code golf like as-short-as-possible solution, I totally agree with the other answers. Code golf is fun, but has no reason to exist in production code.

If you want that, you can go with this fun solution:

def get_row_tax_percentage(product_name: str):
    return [ Decimal(x) for x in ["0.1", "0.15", "0", "0.05"] ][is_imported(product_name) + is_exempt(product_name)*2]

This is a nifty one-liner that anyone that needs to maintain the code after you will hate you for.

\$\endgroup\$
6
  • 1
    \$\begingroup\$ How is a direct translation from the task description to code "a lot less maintainable and understandable" than inventing new logic and hiding the calculation of those four numbers? \$\endgroup\$ – Manuel Apr 22 at 17:10
  • \$\begingroup\$ Your one-liner wants a whopping 50% tax for some products when it should be 5%. \$\endgroup\$ – Manuel Apr 22 at 17:22
  • \$\begingroup\$ Since you started the golfing, here's mine, 49 characters shorter :-) return Decimal(2-is_exempt(p:=product_name)*2+is_imported(p))/20 \$\endgroup\$ – Manuel Apr 22 at 17:30
  • 1
    \$\begingroup\$ Btw if one of the two tax numbers does get changed then the solutions just following the specification simply need to update that number. You on the other hand will need to update two numbers and you'll have to calculate them. And if yet another tax is added, your style would add four more branches? In another indentation level? And all that is supposed to be more maintainable? \$\endgroup\$ – Manuel Apr 22 at 20:54
  • \$\begingroup\$ A less evil version of your golf is to work with a 2D array so we don't need to double one of the booleans. This is more maintainable, especially with many extra conditions. One could even make an argument for such an approach if the effects aren't simply additive. (It's not an argument I'd make, though.) \$\endgroup\$ – J.G. Apr 23 at 20:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.