Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

Therefore, I would move the definition of numberList to the main function. If you don't want a main function, it should go in the if __name__ == '__main__': if __name__ == '__main__': block. If you don't want that, it should at least go after the function definitions. If you don't want that either, ... you're a hopeless case.

Therefore, I would move the definition of numberList to the main function. If you don't want a main function, it should go in the if __name__ == '__main__': block. If you don't want that, it should at least go after the function definitions. If you don't want that either, ... you're a hopeless case.

Therefore, I would move the definition of numberList to the main function. If you don't want a main function, it should go in the if __name__ == '__main__': block. If you don't want that, it should at least go after the function definitions. If you don't want that either, ... you're a hopeless case.

Source Link
zondo
  • 3.9k
  • 14
  • 27

Your stringToList function creates a variable, and does nothing but return it. Why create the variable? Why not just return string.split() in the first place? I actually don't see any use in creating the function. To me string.split() is more clear than stringToList(string). One could reasonably make a list of each character in a string and call the function stringToList. The method .split(), however, is built-in and well documented. Why complicate things?


I'm not sure that it is a good idea to put the grid directly into your code. The more magic values you have, the less reusable your code is. I would suggest that your program should take a filename as an argument and read the grid from there. Of course, you would make sure that the user actually gives a filename, the filename is valid, etc.


You use a list comprehension below, but you don't use one when you convert the list of strings to a list of lists of strings. In fact, I would merge the two:

numberList = [[int(x) for x in line.strip().split()] for line in numberSequence]

If you were reading from a file, you would switch numberSequence to the name chosen for your file object (numbersFile if you did something like numbersFile = open(filename))

On top of all that, I suggest that you move the definition of numberList lower down. Usually, code should go like this:

import module
#...
CONSTANT = value
#...
def function():
# ...
def function2():
# ...
#...
def main():
# ...
if __name__ == '__main__':
 main()

Therefore, I would move the definition of numberList to the main function. If you don't want a main function, it should go in the if __name__ == '__main__': block. If you don't want that, it should at least go after the function definitions. If you don't want that either, ... you're a hopeless case.


Your scanning functions are very similar. They are so similar, in fact, that I believe they can be merged:

def scan(numberList, x, y, nums=4):
 products = [1] * nums
 # Changes we will make to x and y to get the adjacent numbers.
 # (0, -1) means x will stay the same, but y will go down by one for
 # each adjacent number (down by index, but up visually).
 changes = [(0, -1), (0, 1), (1, 0), (-1, 0)]
 for i in range(nums):
 for p, (xChange, yChange) in enumerate(changes):
 try:
 products[p] *= numberList[x+(i*xChange)][y+(i*yChange)]
 except IndexError:
 products[p] = 0
 return max(products)

The above has not been tested, but the concept should be fairly clear.


Now that it is all in one function, your largestProduct definition is fairly simple:

largestProduct = max(
 scan(numberList, x, y)
 for x, item in enumerate(numberList)
 for y in range(len(item))
)

(Again, untested. If you could test it yourself and confirm that it works, I can remove this clutter.)

lang-py

AltStyle によって変換されたページ (->オリジナル) /