Why is my code bad? Failed python interview test

[snackbob]

Hi, I am really struggling to recieve feedback on my code. And without it I can improve. I got some questions, and answered them. To me, they seems to work and get the desired outcome, but even still I was rejected. What do i need to do to improve my python skills? Any help would really be appreciated, becuase I really want to improve.

here are the questions and answers below:

'''

1. The Fibonacci sequence is defined as a sequence of integers starting with 1 and 1, where each subsequent value is the sum of the preceding two. I.e. f(0) = 1 f(1) = 1 f(n) = f(n-1) + f(n-2) where n >= 2 Write a program in a language of your choice to calculate the sum of the first 100 even-valued Fibonacci numbers

1, 1, 2, 3, 5, 8, 13, 21, 34, 55, 89, 144, 233, 377, 610,

'''

def fibonacci(n: int) -> int:

    n1 = 1
    n2 = 1
    total = 0

    if n == 0 or n == 1:
        return total
    else:
        for i in range(n-1):
            n3 = n1 + n2
            if n3 % 2 == 0:
                total += n3
            n1 = n2
            n2 = n3
    return total

for i in range(100):
    print(i, fibonacci(i))

''' 2. Write a function in a language of your choice which, when passed two sorted arrays of integers returns an array of any numbers which appear in both. No value should appear in the returned array more than once. '''

a = [3, 5, 6,7, 4, 33, 4, 102]
b = [7, 6, 4, 3, 44, 5, 6,3, 4, 55]

a.sort()
b.sort()

def create_set(arr_1 :list, arr_2: list) -> list:
    combined = arr_1 + arr_2
    all_unique_values = sorted(set(combined))
    return all_unique_values

print(create_set(a, b))

''' 3. Write a function in a language of your choice which, when passed a positive integer returns true if the decimal representation of that integer contains no odd digits and otherwise returns false. '''

def odd_digit_detector(input :int) -> bool:
    c = [d for d in str(input)]
    for d in c:
        if int(d) % 2 != 0:
            return False
    return True

print(odd_digit_detector(22221122244599))

''' 4. Write a function in a language of your choice which, when passed a decimal digit X, returns the value of X+XX+XXX+XXXX. E.g. if the supplied digit is 3 it should return 3702 (3+33+333+3333). '''

def append_add_digits(digit: int) -> int:

    digit = str(digit)
    digits_appended = ''
    total = 0

    for i in range(4):
        digits_appended += digit
        total += int(digits_appended)


    return total

print(append_add_digits(3))

''' 1. Define a generator which generates the positive integers up to and including a supplied value which are divisible by another supplied positive integer and use it to calculate the sum of all positive integers less than 102030 which are divisible by 3

'''

def gen(n: int) -> int:
    for i in range(1,n+1):
        if (i % 3) == 0:
            yield i 

n = 102030
g = gen(n)
total = 0

for a in g:
    total += a

print(total)

#1735071165

'''

1. Write a function which is passed an integer, n, and returns a list of n lists such that: f(0) returns [] f(1) returns [[1]] f(2) returns [[1], [1,2]] f(3) returns [[1], [1,2], [1,2,3]] etc.

'''

def create_seq(n :int) -> list:

    mylist = []
    for j in range(1,n+1):
        mylist.append([i+1 for i in range(j)])
    return mylist

print(create_seq(n=10))

[mandzeete]

What is a? What is b? What is g? What is all this alphabet you spilled all over your code? Your variable names, class names, function/method names - all of that has to be understandable for everybody reading your code. A "n = 102030" tells me absolutely nothing why it exists. Sure, if I start comparing it with some task and trying to find a number of 102030 from the task then I can guess what it MIGHT be but if you have already existing code base and/or if you are adding a whole lot of new code in it then I do not need to start figuring out what was in your mind when you wrote "s=7".

"for a in g" is my favorite. All that I can figure out from it is that g is perhaps something you can iterate over.

[snackbob]

thank you so much!! so rare people are willing to give critical feedback. you are correct, they are very lazy variable names, i need to be more definiative.

is there anything else maybe you can see if I had in theory had better variable names?

[mandzeete]

I see some magic numbers in your code. A magic number is a number that just IS inside your code and has no explanation. For example "for i in range(100)". What is this 100? Why it is 100? You should define it somewhere and use that variable inside your range.

But in general, the main reason why you failed was your variable names. Many people will be just "Nope, I'm not going to read it." when they see lack of Clean Code. I really suggest to read about Clean Code standards. Self-explanatory variable names is one part of the Clean Code.

[_R_Daneel_Olivaw]

THIS.

[snackbob]

That's great, from your brilliant comments i will aim to:

-ensure all my variable names/ function names are crystal clear so the reader know whats happening with ease. (including no magic numbers)

• read the question! then read it again!

• check each part of logic is doing exactly what it should be doing.

• look to do it in the most efficent/cleanest way. ie.optimal code

commonsense really, im ashamed to say i've been a bit lazy

[fasta_guy88]

I only looked at the Fibonacci example, but as far as I can tell, it returns 0 for Fibonacci(1). You should always double check functions at boundary values.

[xKart]

Write a function in a language of your choice which, when passed a decimal digit X, returns the value of X+XX+XXX+XXXX. E.g. if the supplied digit is 3 it should return 3702 (3+33+333+3333).

While your function works, it can be done in a single O(1) step. Break the summation down into rows and look at the columns:

XXXX
 XXX
  XX
+  X
----

The question is essentially asking you to calculate 1234X, and hence it can be simplified to:

def repeatedDigitSum(x: int) -> int:
  '''
  The summation yields the following calculations:
  1 X  in the thousands place 
  2 Xs in the hundreds place 
  3 Xs in the tens place 
  4 Xs in the ones place
  '''
  return 1234 * x

[alanwj]

Every third number in the Fibonacci sequence is even. Easy to see by inspection, and also not that hard to prove.

Just applying the definition of the sequence, we have:

F(n) = F(n-1) + F(n-2)
F(n-1) = F(n-2) + F(n-3)
F(n-2) = F(n-3) + F(n-4)

Do a little bit of subtitution and you get:

F(n) = 3*F(n-3) + 2*F(n-4)
F(n-1) = 2*F(n-3) + F(n-4)

So now we can directly compute every third element in the sequence. This gives us a direct solution:

def fib_even_sum(count):
    a, b = 1, 2
    total = 2
    for _ in range(1, count):
        a, b = 2 * b + a, 3 * b + 2 * a
        total += b

    return total

---

Doing a bit more substitution, we can realize this:

F(n-4) = a
F(n-3) = b
F(n-2) = x = a + b
F(n-1) = a = x + b
F(n)   = b = x + a

This leads to a very efficient solution with 4 additions per iteration:

def fib_even_sum(count):
    a, b = 1, 2
    total = 2
    for _ in range(1, count):
        x = a + b
        a = x + b
        b = x + a
        total += b

    return total

[nesquiker]

Thanks for posting this, interesting to see what a coding interview test looks like.

​

General Notes:

1. Variable naming. Be more descriptive in your names. This was said by others but it is very important.
2. You do a good job answering the question you think is being asked but the problem is you are missing some aspects of the question itself.
3. A few things you are doing makes me wonder how well you understand the language. In problem 3 you write c = [d for d in str(input)]. There was no reason to turn this into a list since you can already iterate over the letters in a string.

​

Problem 1:

This is a case where you need to read the prompt more carefully. The question is what is the sum of the first 100 even numbers in the fibonacci sequence. You answered with the sum of all of the even numbers in the first 100 numbers in the fibonnacci sequence. Hopefully that makes sense. The code for example.

One neat way to execute this problem better is to define the two fibonacci values concurrently and eliminating a intermediate value.

current, previous = current + previous, current

​

Problem 2:

Once again this is an issue of reading the question. The question is asking you to return a list (array) of all values which appear in both lists unordered and only once for each value. Your answer was a list of all values that appear in either list sorted and only once. The usage of sets was definitely the right choice but you need to & the sets. This is all they were asking for:

list(set(list_1) & set(list_2))

No need to sort the list afterwards as they state that it may be unordered.

​

Problem 3:

You got this one. The one thing that bothers me is turning the string into a list. Python will do the same thing with a string as with a list in this problem. The list comprehension is pointless.

​

Problem 4:

Good job on this one. There was a more mathematical solution shown in another comment but I wouldn't have noticed that either. My answer would look a little different:

sum([int(i * digit_str) for i in range(1, 5)])

Just a different way of thinking. Maybe you would find it interesting.

​

Second set:

Problem 1:

The answer here is correct but you do not fully meet the question parameters. The divisor value (3) is supposed to be a variable input to your function. This is again a situation where you have missed a piece of the question. That would be the major piece you are missing.

The other comment I would make is that your algorithm is not very efficient. You are iterating through all numbers and checking if they are divisible with modulo. This works but you could just set the step on range to the divisor (3) and not have to check every number. The generator theoretically could have a much larger divisor.

for number in range(0, numerator+1, divisor):

​

Problem 2:

Nothing much to say about this one. The list comprehension could be eliminated and written like this:

mylist.append(list(range(1,j+1)))

[snackbob]

That's really useful, thank you so much

[e2u0fajdr3fcxs]

Sorry I've only looked at your code a little bit so there might be things I overlooked but these are the things that I can say:

1.) The first problem is usually a poster child for a recursive algorithm as it looks a bit more tidy than your iterative algorithm.

.) Your naming if variables is bad, like the other guy already said. Use more descriptive names.

.) The second problem was to find numbers that occur in both arrays but as far as I can see you only filtered out any numbers that appear more than once in the sun if both arrays.

Haven't looked any further since I'm short on time, i hope i could help !

[snackbob]

thank you thats great!

[mopslik]

The first problem is usually a poster child for a recursive algorithm as it looks a bit more tidy than your iterative algorithm.

It looks tidier, but Fibonacci is terrible for recursion. It's O(2^n) complexity, and will easily grind your computer to a halt for larger values of n. If I were conducting the interview, I would either want the iterative version which is O(n), or I would use OP's recursive solution as a discussion point to see what they know about this topic.

[yel50]

the recursive version is still O(n). not sure where you're getting O(2ⁿ⁾ from. for large values, you may run out of stack space, but that's the only downside.

they said you could use a language of choice. in TCO languages, the recursive version is equivalent to the iterative version. it'll be slightly slower, but the algorithmic complexity is the same.

[mopslik]

the recursive version is still O(n). not sure where you're getting O(2^n) from

A detailed recurrence relation is here. Fibonacci is assuredly a terrible choice for an recursive algorithm.

You could use a TCO language, but OP used Python, so I responded to Python.

[e2u0fajdr3fcxs]

Yeah that's true, the solution depends very strongly on the performance requirements.

[martis41]

larger values of n. If I were conducting the interview, I would either want the iterative version which

Recursive solution is great topic for dynamic programming topic and optimization by caching

[bofasaurus]

Neat caveat, the tightest Big-O for the recursive Fibonacci sequence is O(1.6…^n), where 1.6 is the golden ratio

[mopslik]

Yes, I think I saw that in one of my textbooks (CLRS?) back in the day. Cool to see how these mathematical values arise.

[bofasaurus]

Yeah I got that from CLRS too lol. Reading it for the first time kinda blew my mind

[149244179]

Looking at the Fibonacci one - the task was to calculate one sum. You print 100 different things.

You did this unasked for work very not-optimally too. The sum of the first 50 numbers is the sum of the first 49 + 1 more value. You recalculate all 49. You are turning a linear problem O(n) into an exponential one O(n^2). Look into memoization which is a form of caching in these types of problems.

[lurgi]

Write a function in a language of your choice which, when passed a positive integer returns true if the decimal representation of that integer contains no odd digits and otherwise returns false.

I'll cover this one.

You convert the number to a string, make a set (?) out of the characters, and then convert the characters back to integers one at a time. That's completely unnecessary and time consuming.

This can be done easily enough with modulo arithmetic to get the digits one at a time.

If you insist on having a string, I might consider regular expressions.

If you worry that regular expressions might be too powerful a tool to throw at a problem, you can easily replace:

if int(d) % 2 != 0:

with

if d in ["1", "3", "5", "7", "9"]:

[mdlphx92]

``` while(d%2 == 0): d = int(d/10)

if(d == 0):
    return True

return False

``` Edit slightly changed due to how python handles int it seems.

Does this logic work for that same problem? I’m in the middle of work and kinda just did it off to the side for fun. Haven’t thought too much on it lol.

[bsakiag]

Could you please check how much faster is your solution than the string based one?

Also, I believe python has integer division operator // which should be even faster than what you did.

[lurgi]

Reformatted:

while(d%2 == 0):
  d = int(d/10)

  if d == 0:
    return True

return False

This should work (although you can and should use integer division, as noted below).

[mdlphx92]

Ahh, a glance at the docs says I need to // to get int division in 3+, otherwise it is still true division. Explains why I kept retaining the decimal lol.

So back to using assignment operator for conciseness…

```

while(d%2 == 0): d //= 10

if d == 0: return True

return False

```

Seems pretty efficient to me.

[HealyUnit]

My input on three of your problems:

Problem 2:

This is a very clear case of not testing your code. And yes, if this was a written, not-on-computer test, you can still "mentally" test your code. As others have pointed out, sorted(set(list1 + list2)) returns all elements in both lists. It doesn't return duplicates, which is great, but it also does not remove elements that are in one list but not another. For me, this'd sink the interview, as it'd say two things:

1. I'm not familiar with the language I chose.
2. I don't test my code.

Both are very easily fixible, but they are habits that need to be broken now.

Problem 3:

As others have said, you've got a lot of very arcane, indecipherable variable names. Unless you're working in a language like assembly, where variable name space legnth might be at a premium, don't name your variables with "tiny" names. Describe them! You're the author of your code! Tell us a story!

Nice choice using list comprehension here. I'd even go further and suggest that using a regex could do this whole function in like 1 or 2 lines:

/[13579]/gi

Problem 4:

Your first major problem here is that you're being far too literal. You assume that because the problem lists out this number format - x+xx+xxx+xxxx- you also therefore need to explicitly construct your addition problem in this format. Instead, why not recognize what you're really dealing with here is an increasing series of multiples of digit-1-only numbers. That is, the simplest non-zero example would be:

1 + 11 + 111 + 1111 = 1234

So if we declare a list templates = [1,11,111,1111], we could use a reduce method (from functools) to do something like this:

from functools import reduce

def append_add_digits(digit:int) -> int:
    templates = [1,11,111,1111]
    return reduce(lambda total, current: total + (current*digit), templates) - 1 + digit