Side effect in if clause

[roadelou]

In the trash_videos functions of src/video_logging.py, you use an uncommon pattern. The function called move_to_trash is implemented as such :

def move_to_trash(file, duration):
  # Some doc here
  if duration < time_limit:
    if not os.path.isdir('Trash'):
      os.mkdir('./Trash')
    os.rename(file, os.path.join('Trash', file))
    return True
  return False

In short, the function will either move a file to trash and return True or do nothing and return False. The thing that is important here is that move_to_trash can have a side effect. In this case, running move_to_trash can move files on the OS.

So far so good. The uncommon pattern is the way move_to_trash is used below :

if move_to_trash(file, duration):
  nb_trashed += 1

As you can see, move_to_trash is used directly in the boolean equation of the if statement. Just to be 100% clear, this is not at all a fault here, and one could even argue that this is a pretty clever shortcut in this context :+1:.

However, I feel like it is worth mentioning why you wouldn't want to overuse this pattern. Plus I find the explanation behind it interesting :smile: (you probably already know it though). Take the following example :

if move_to_trash(file1, duration1) and move_to_trash(file2, duration2):
  both_moved = True

The expected behavior would be that this code tries to move both files, and if both files were moved both_moved is set to True. But this is not what python will actually do, and this can lead to unexpected bugs.

Lets assume that duration1 is higher than the time_limit threshold but duration2 is smaller. That means file2 should be moved but file1 should not.

When the python interpreter resolves the boolean equation within the if statement, what it will actually do is :

1. Execute move_to_trash(file1, duration1). This returns False.
2. False and whatever will always return False, hence there is no point in calling move_to_trash(file2, duration2).
3. The interpreter jumps directly to the end of the if statement.

The problem would be that file2 will in fact not be moved to trash when it should have. The side effect of move_to_trash did not take place because the expected call was optimized out.

Those boolean optimizations don't have anything to do with the if statement in itself, but an if statement is where the mistake would usually be made. As an example, you could try running :

False and print("rouge")
True or print("vert")

That is one reason why one would usually avoid putting a function with a side effect within an if clause. There are noticeable exceptions to this rule (for instance when consuming an iterator), but putting a function with a side effect where it could be optimized out is risky.

Once again, in your code this will not happen (because the function is alone in the boolean equation) :smile:

[theodumont]

Yes you're right, although it is functional in this case, one should avoid doing these kind of tricks! I modified the code a little bit so that the move_to_trash side effect is more visible in trash_videos.