“Better code” is asking for an opinion, as there is no objective measure of “better”. My initial, flippant, answer is that the one with 44 lines is better than the one with 67, since it gets the same job done in fewer lines, making it easier to read. (This is what I said to this question on Quora. Since this site is explicitly a code-review site, the philosophy of "better code" may be out of place. I'm sure the regular denizens of this stack exchange site will let me know if this is out of place).
This is not completely trivial (shorter = better). What is important is that computer languages are tools not just for instructing a computer, but also with communicating with other programmers. Computer programs are not static, and you will, in your career as a programmer, find yourself reading code written by others a lot more often than writing new code from scratch. As such, one of the most important needs of “good” code is readability.
Kent Beck highlighted Four rules of Software Design which might be useful to keep in mind. Martin Fowler's description of those four rules are:
- Passes the tests (i.e., it works as expected)
- Reveals intention
- No duplication
- Fewest elements
The last three rules are really rules for readability: You can read it and see what it does, it doesn’t do things more than once, and it’s simple and clear.
You have 1.5 weeks of experience. It isn’t expected that your code is readable or “good”, but it is something you should strive for for your entire career. Professional programmers struggle on this their entire careers, and it is not uncommon for a professional to have to fix a bug in code that hasn’t been touched in 6 months to a year, complain about how crappy it is, and then find that they wrote it. Improving is a continuous process.
With that in mind, let’s look at the code. I will try at first to compare the two versions, then give you ideas on how to improve it.
First, you have defined a recursive function to determine if a string is in alphabetical order,
This is identical in the long version, so it doesn’t make either version better or worse.
Next, you have a section which (presumably) differs between versions, because of length.
Now I would definitely say I would rather edit the shorter version that the longer version. Both versions have a loop which (I assume, without close analysis) finds all substrings of s.
The short version checks to see if the substring is alphabetical, and if it is, if it’s longer than any seen before. Since after processing all the substrings, the longest, alphabetical substring has already been found and stored, all that’s needed is to print it.
The longer version generates a list of substrings (
list1), removes duplicates to get
list2, collects just the ones which are alphabetic to get
list3, then finds the longest substrings
list4, then returns the first one.
The short version expresses its intent better (it took me a few read-throughs to understand the long version). Both versions have opaque names (what is
i? Why are they named that?), but the longer version has more, especially
One place where intent-revealing could be helped in the longer version is the expression
len(list3[len(list3) -1]). It is a bit complex, and it’s uncertain if it ever changes. I also think it’s a potential location of a bug (it looks like you are removing strings from
list3 that are shorter than the last string, but how do we know that the last string is the longest?
There is little obvious duplication in either. I’m sure there is some, probably in the form of repeated calculations of string lengths of the same string, but nothing jumps out.
When evaluating “fewest elements”, this is mainly in the form of looking for things which can be improved or simplified. Obviously, the longer version has more elements than the shorter version, but they are also doing different things. It is hard (using the language and style that you are using) to write the longer version using fewer “elements” than the shorter version, so it is unfair to say the shorter version has 5 variables while the longer version has 12 and say simply from that that the shorter version is better.
But we can look at the longer version, and point out that you have three loops using the variables
e, and the structure is such that clearly you can use
e for all three loops. Similarly, you have
numMatches which even in your comments you acknowledge is superfluous, just there because you want the
else branch of an
if, and need something in the other branch. That’s an element that can be removed (if you knew how to invert the if (check out
element not in array)).
I don’t see that sort of redundant elements in the shorter version. The
itersLeft variable looks dubious, or at least poorly named, but I can’t obviously see how it can be removed.
So based on those measures, I’d say the strict answer to your question remains: Version 1 is “better” code.
Since you’ve essentially asked for a code review, I’ll continue with the other important part of a code review: how can it be made better? This is not intended to be an ego-busting exercise: I’m not saying “you write crappy code, here’s how a pro would do it”, I’m trying to give you advice on how you can take the code you’ve written and improve it. There are some programming disciplines where programmers will write dirty code, verify it works, then clean it immediately before anyone else sees it, all within minutes, all throughout the day.
So, taking your recursive
alphacheck. Here are some small changes you can do:
- My preference is to push the declaration of variables to as close as
their use as possible. Here, you immediately declare i = len(x) -1,
but then don’t use it at all except in the else block of your main
test. So I would tend to put the initialization of to within that
else block, as close to where you use it as possible.
- i is a really crappy variable name in most cases. What does i
represent here? It’s the index of the last character in the string.
So I’d probably rename it as last_index or similar.
- Looking at the line x = x[:last_index], I’d be reminded of Python’s
array slicing s[i:j], and the note that if either i or j is missing,
gets a reasonable default (either 0 or len(s)), and if either i or j
is negative, it is considered to be from the end of the list, not the
beginning. So x = x[:last_index] can be replaced by x = x[:-1].
- Similarly, the if condition x[last_index-1] <= x[last_index] can be
simplified to x[-2] <= x[-1].
- Which means that last_index is no longer being used, and can be
- This is sort of a style thing, but when I think of a recursive
function, I also tend to think in a “functional” programming style,
which eschews the reassignment of variables. That makes me somewhat
leary of x = x[:-1], as it is reassigning x. It isn’t in a loop,
where you have to change the condition of the loop, so I’d consider
renaming the latter x, so it is shortened_string = x[:-1], and
similarly change the next line to refer to the new variable.
- Better yet, since it is a simple expression, and it’s only used in
one place, I’d definitely consider “inlining” it, to remove the
variable entirely. That makes the return statement return
- Another style thing is eliminating deep nesting. In your first “if”
statement, the last thing that happens before the “else” is a return.
Code will not continue after the return, which makes the else sort of
redundant. I would eliminate the else, and remove a level of nesting
in the process.
- I prefer to have the recursive call last. Right now, it isn’t, it’s
embedded in an if block. So I would invert that if condition (and
switch the two code blocks) to be x[-2] > x[-1], or x[-1] < x[-2],
then I would remove the else as described above.
This gives a final code of
if len(x) <= 1: # Empty or single character strings are alphabetic
if x[-1] < x[-2]: # if last two characters are out of order, not alphabetic
return alphacheck(x[:-1]) # recursively check truncated string
I probably wouldn’t include the comments as redundant. Personally, I would probably be annoyed by the negative indices, and note that I can do the check and truncation from the beginning of the string instead, but that wouldn’t be improving your code; that would be changing your algorithm. I’m not trying to do that here.
The nine steps I described come with practice. To do them, you have to look for possible ways to improve it, and you have to see how to make those changes. If you notice, several of the steps were dependent on earlier steps, and the results of some of those steps vanished (I changed a variable name, then eliminated the variable, for instance). Improving your code this way is an iterative process of looking for small changes that can make things better.
I’ll leave improving your other two versions as an exercise for you.
One tip you might want to consider when you do: In the above example, I used the language feature that negative indices on arrays come from the end, not the beginning. This is a language feature you might not have known about. For the long-version, I suggest you look up Python array comprehensions as a language feature you might not have known about which may greatly help your code.