Skip to main content
Spelling and grammar
Source Link
Reinderien
  • 65.3k
  • 5
  • 69
  • 187

If your algorithm can notcannot find a date, it is easier to raise an Exception thenthan to return ''. returningReturning sentinel values instead of exceptions can lead to unexpected behaviour if the user of this function does not test for this sentinel value.

CommenstComments should explain why you did something, not how. # Take the last four digits tells you nothing more than the code itself. I would rather comment at field[-4 - i:n - i] why you did n - i instead of just -i.

Instead of nesting a number of if-clauses, it can be better to test the negative of the condition, and continue, so the rest of the code is less nested.

Don't test condition is True. Just do condition. In Python a lot of values can act as True or False in tests.

Your match is never used anyway,anyway; the moment you set it to True, you also return the result, so a while True: would have sufficed here.

returnReturn type

While Truewhile True

You use a while Truewhile True-loop, with an incrementing counter. A better way to do this would be to either use for i in range(...) or using itertools.count: for i in itertools.count(). In this case you know there will be no more thenthan len(field) - 7 iterations, so you might as well use that.

revertRevert the algorithm

You explicitly test whether the substring is 8 characters long, and then if it is in the right format. By changing the while True to the for-loop, you know the substring will be 8 characters long. thenThen it makes sense to first try to convert it to a datetime, and then check whether the year is correct:

If your algorithm can not find a date, it is easier to raise an Exception then to return ''. returning sentinel values instead of exceptions can lead to unexpected behaviour if the user of this function does not test for this sentinel value

Commenst should explain why you did something, not how. # Take the last four digits tells you nothing more than the code itself. I would rather comment at field[-4 - i:n - i] why you did n - i instead of just -i

Instead of nesting a number of if-clauses, it can be better to test the negative of the condition, and continue, so the rest of the code is less nested

Don't test condition is True. Just do condition. In Python a lot of values can act as True or False in tests

Your match is never used anyway, the moment you set it to True, you also return the result, so a while True: would have sufficed here

return type

While True

You use a while True-loop, with an incrementing counter. A better way to do this would be to either use for i in range(...) or using itertools.count: for i in itertools.count(). In this case you know there will be no more then len(field) - 7 iterations, so you might as well use that

revert the algorithm

You explicitly test whether the substring is 8 characters long, and then if it is in the right format. By changing the while True to the for-loop, you know the substring will be 8 characters long. then it makes sense to first try to convert it to a datetime, and then check whether the year is correct

If your algorithm cannot find a date, it is easier to raise an Exception than to return ''. Returning sentinel values instead of exceptions can lead to unexpected behaviour if the user of this function does not test for this sentinel value.

Comments should explain why you did something, not how. # Take the last four digits tells you nothing more than the code itself. I would rather comment at field[-4 - i:n - i] why you did n - i instead of just -i.

Instead of nesting a number of if-clauses, it can be better to test the negative of the condition, and continue, so the rest of the code is less nested.

Don't test condition is True. Just do condition. In Python a lot of values can act as True or False in tests.

Your match is never used anyway; the moment you set it to True, you also return the result, so a while True: would have sufficed here.

Return type

while True

You use a while True-loop, with an incrementing counter. A better way to do this would be to either use for i in range(...) or using itertools.count: for i in itertools.count(). In this case you know there will be no more than len(field) - 7 iterations, so you might as well use that.

Revert the algorithm

You explicitly test whether the substring is 8 characters long, and then if it is in the right format. By changing the while True to the for-loop, you know the substring will be 8 characters long. Then it makes sense to first try to convert it to a datetime, and then check whether the year is correct:

Source Link
Maarten Fabré
  • 9.1k
  • 1
  • 15
  • 27

Exception

If your algorithm can not find a date, it is easier to raise an Exception then to return ''. returning sentinel values instead of exceptions can lead to unexpected behaviour if the user of this function does not test for this sentinel value

comments

Commenst should explain why you did something, not how. # Take the last four digits tells you nothing more than the code itself. I would rather comment at field[-4 - i:n - i] why you did n - i instead of just -i

nesting

Instead of nesting a number of if-clauses, it can be better to test the negative of the condition, and continue, so the rest of the code is less nested

match

Don't test condition is True. Just do condition. In Python a lot of values can act as True or False in tests

Your match is never used anyway, the moment you set it to True, you also return the result, so a while True: would have sufficed here

field

This is a very unclear variable name. This method excepts a date in string format, so why not call the argument like that?

return type

Your code does 2 things now. It looks for a date in the string, and converts that date to another format. It would be better to separate those 2 things, and return a datetime.datetime instead, and let the caller of this method worry about formatting that correctly.

While True

You use a while True-loop, with an incrementing counter. A better way to do this would be to either use for i in range(...) or using itertools.count: for i in itertools.count(). In this case you know there will be no more then len(field) - 7 iterations, so you might as well use that

revert the algorithm

You explicitly test whether the substring is 8 characters long, and then if it is in the right format. By changing the while True to the for-loop, you know the substring will be 8 characters long. then it makes sense to first try to convert it to a datetime, and then check whether the year is correct

def format_dates2(date_string):
    n = len(date_string)
    for i in range(n - 7):
        sub_string = date_string[-(8 + i) : n - i]
        # not just -i because that fails at i==0
        try:
            date = dt.strptime(sub_string, "%d%m%Y")
        except ValueError:
            continue
        if not (1919 <= date.year <= 2019):
            continue
        return date
    raise ValueError("Date not in the correct format")