How to not write a condition for code readability

Sometimes you find a perl in some code and it might take you a while to understand. In the sessions management code of Chamilo, I found this the other day, then decided to leave it for later because I was in a hurry. Then today, I found it again, and again... it took me about 10 minutes to make sure of how the code was wrong and how to make it right.

 if ( $session->has('starttime') && $session->is_valid()) {
   $session->destroy();
 } else {
   $session->write('starttime', time());
 }

To give you the context, we want to know whether we should destroy the session because its time is expired, or whether to extend the period of time for which it is valid (because it is still active now, and we just refreshed a page, which lead us to this piece of code). OK, so as you can see, we check if the session has a value called "starttime". If it hasn't we pass to the "else" statement and define one, starting from now. If, however, the session has a start time, we check whether it... "is valid" (that's what the function's called by, right?) and, if it is... we destroy the session. Uh... what? Yeah... if it is valid, we destroy it (DIE SESSION, DIE!) OK, so something's wrong here. Let's check that "is_valid()" method (there was documentation, but it was wrong as well, so let's avoid more confusion)

  public function is_valid()
  {
    return !$this->is_stalled();
  }
  function is_stalled()
  {
    return $this->end_time() >= time();
  }

OK, so as you can see, is_valid() is the boolean opposite of is_stalled(). "Is stalled", in my understanding of English (and I checked a dictionnary, just to make sure) means "blocked" (in some way or another). Now it doesn't really apply to something that is either "valid" or "expired", either. And this is part of the problem. The is_stalled() method, here, checks if the end_time for the session is greater than the current time. Obviously, we'll agree (between you and me) that if the expiration time is greater (further in the future) than the current time, this means that we still have time before it expires, right? Which means that, if the condition in is_stalled() is matched, we are actually *not* stalled at all. We are good to go. So I believe the issue here was the developer either confusing the word "stalled" for something else or, as it doesn't really apply to "expired" or "not expired", that he simply badly picked the word describing the state. In any case, now that we've reviewed these few lines of code, I'm sure you'll agree it would be much clearer this way:

 /* caller code */
 if ( $session->has('starttime') && $session->is_expired()) {
   $session->destroy();
 } else {
   $session->write('starttime', time());
 }
 /* called method */
 public function is_expired()
 {
   return $this->end_time() < time();
 }

Conclusion: always choose your functions naming carefully. Others *need* to be able to understand your code in order to extend it.