The Design of Software (CLOSED)

A public forum for discussing the design of software, from the user interface to the code architecture. Now closed.

The "Design of Software" discussion group has been merged with the main Joel on Software discussion group.

The archives will remain online indefinitely.

State Changing Functions and Return Status

My question is what is the best coding practice for state changing functions and status returns when the status could be returned a priori to calling the state changing function. Let me define my terminology.  A state changing function is a function might have some state changing effect when called. A state change check function is a function that returns false if current properties would prevent a state change from successfully succeeding, for example a user permission check function. A state changing status return function is a function that might change state and returns pass/failure status, or status codes.

Now my question is what is the best coding practices.  In particular, should you try and split state changing status returning function in a state change check function and a state change function if at all possible.  Issues with this is partial duplication of code.  Bonus is the ability to a priori know if a state change will fail and simplifies your functions.

My particular problem was the following, and I am curious if my actions were correct, could be improved on, or caused serious havoc to the code base.

I ran into a defect that required me to query the return of a state changing status returning function prior to calling it.  It should be noted that the state change would only fail because of properties that could be checked prior to calling it, such as user permissions or if this thing is blue or I'm in a good mood. The function in question was in the following format

bool Cls::ChangeState()
{
  ... do stuff

  if(!ChangeState1())
      return false;

  ... do stuff

  if(DoSpecialCaseHandling())
      return true;

  ... do stuff

  return true;
}

Now I didn't write this function.

now my defect looked a little like this

Cls* pObj = NULL;
while(!bFound)
{
  ... stuff
  if( ...stuff)
  {
    pObj = pObjTemp;
    break;
  }
}
if(!pObj->ChangeState())
  screwed
else
  fine

So I tried to solve it like this

Cls* pObj = NULL;
while(!bFound)
{
  ... stuff
  if( ...stuff  && pObjTemp->CanChangeState())
  {
    pObj = pObjTemp;
    break;
  }
}
pObj->ChangeState();

This required refactoring a lot of state changing status returning functions into two functions.  One that checks permissions to change state.  The other that changes state.

bool Cls::CanChangeState()
{
  if(!CanChangeState1())
      return false;

  ... do stuff

  if(CanDoSpecialCaseHandling())
      return true;

  return true;
}

void Cls::ChangeState()
{
  if(!CanChangeState()
      return;

  ... do stuff

  if(!ChangeState1())
      return ;

  ... do stuff

  if(DoSpecialCaseHandling())
      return ;

  ... do stuff

  return ;
}

So what do you think.  Is this bad, is this a step in the right direction.  Is there a better way that reduces the need to duplicate some code.
toids Send private email
Wednesday, May 23, 2007
 
 
So, basically, you left the "ChangeState()" method alone, and created a (mostly duplicated) method to check to see if the "ChangeState()" method COULD succeed, based on factors that were already true.

I guess I wouldn't see a problem with doing this, as long as the "CanChangeState()" method doesn't change any of the state variables.

Especially if you're not allowed to change the "ChangeState()" method in the first place.

Still, it seems of limited application.  I mean, how many programs will have a situation where you'll KNOW if the state can change, without actually trying to change the state?  But if that is your situation, this is not a bad way to go about it, even with the duplicated code.
AllanL5
Wednesday, May 23, 2007
 
 
Oh, and I'd STILL check a return status from "ChangeState()".  First of all, though you've checked all the a-priori conditions that would prevent you from changing state, you haven't covered any conditions in the middle of ChangeState that might bite you.

Second of all, this would mean you wouldn't have to refactor all the code that originally called ChangeState -- the 'fail-screwed' part should never happen once you've added the "CanChangeState" part.
AllanL5
Wednesday, May 23, 2007
 
 
This is a terrible, awfull, abominable idea and will almost ensure that your programs will not work as intended. The problem is that, if the state can be changed from outside of the current flow of execution (i.e. in another thread or process) then you have established a "race condition": can the other thread or process screw up your state between the call to the state test and the subsequent attempt to do something based on the result of that test?

The first way to avoid this kind of race condition is simply to try to do the thing that might fail and check the return code (this appears to be what you are trying to avoid). The second way to avoid the race condition is to have the test function also reserve the state that it has just tested, preventing anyone else from modifying it before you do what you want to do. The second method has its own set of problems: you may forget to release the reserved resources, causing a deadlock condition, or you may have the resources automatically released after a short delay, which just reinstates the original race condition.

Now, you may object that the state you are testing for is not affected by anything outside of the current flow of execution. That may be true in your specific case, but it is not true in general. If the state resources are really that localized, then there is probably no need to test them at all, simply keep a flag that tells whether or not the operation you want to perform is allowed and set the flag appropriately elsewhere in your code where the state resources are changed. In the general case, however, such state resources (memory, file system objects, etc.) are shared with other independant processes and you simply can't be sure that they won't change out from under you.

All of this is a roundabout way of saying that you should always check your return codes and there are good reasons why it's done this way.
Jeff Dutky Send private email
Wednesday, May 23, 2007
 
 
This kind of approach is a manifestation of the "Command/Query Separation" principle [1].  It basically says that a function should either return a value or else have side effects, but not both.

As Jeff Dutky noted previously, taking this approach can, in general, cause trouble in the presence of multiple threads or re-entrant code.  There are approaches to multi-threading, though, that are consistent with command/query separation. For example, have a look at the SCOOP programming model [2].

Most of the time, if I know I need to support threads, I dispense with the command/query separation. It's too aggravating to get the concurrency control right.  The rest of the time, I try to apply the principle since I find it makes the code cleaner.


[1] http://en.wikipedia.org/wiki/Command-Query_Separation
[2] http://se.inf.ethz.ch/people/nienaltowski/papers/scoop_easy_draft.pdf
Franklin Send private email
Wednesday, May 23, 2007
 
 
Jeff,

"The second way to avoid the race condition is to have the test function also reserve the state that it has just tested,"

When a specific principle implemented in a specific structure fails, you can easily find other methods by questioning the structure instead of the principle. Instead of your second way:

TestState()
    LockResource

ChangeState()
    UnlockResource

if (TestState())
    ChangeState()

which additionally introduces side effects in TestState, try this third way:

LockResource
if (TestState())
    ChangeState()
UnlockResource
Secure
Thursday, May 24, 2007
 
 
Toid

I found this "check prior to do" practice really anoying, with all the respect to Bertran Mayer :-D

Now, you said that
>I ran into a defect that required me to query the return
>of  state changing status returning function prior to
>calling >it.

Maybe you are solving the problem at the wrong place!

If I understood well, your problem is that the offending function, didn't return the correct boolean value. Otherwise, I don't understand why you need to do this previous check.

if this is the case, maybe you should encapsulate it in another function that actually does the checking "behind scenes", like

ReliableChangeState()
if(!CanChangeState())
 return false
else
 return ChangeState()
Pablo Chacin Send private email
Thursday, May 24, 2007
 
 
I would go with separating the check logic from the operational logic.  It is usually advantageous to push the control logic as high up the chain as possible.  On a slightly different note, unless there is something the user can do about the situation, you might as well throw an exception rather than returning a status code.
Wayne M.
Thursday, May 24, 2007
 
 
Thanks for the comments.  I am going to go ahead with the Command-query separation (CQS).  It has already been a pain as multiple functions and there implementations had to change just to accommodate the CQS of this one functions.  The alternatives were either a larger refactoring of the work-flow or a shoe string, bubble gum and band-aid approach.  I am hoping that the payoff will come from less defects in the code do to unintended side effects caused by partial state changes prior to preventable failures. I'll try and update with any issues this has caused me as time goes on.
toids Send private email
Thursday, May 24, 2007
 
 
From the top of my head, what it seems like your doing is your constantly polling the system, to check if the current state of the object/entity has chanaged. You have I think two solutions avaliable.

You modules could register a callback that once the state has chanaged it informs all the registered callbacks of the chanage in its current state.

Next option you have avaliable, is splitting your interface from changing the object directly from a public function. For example, let say you have some function call foo within an object, that chanages the member variable i. You would do the following.

class Foo
{ int i;
public:
  void modifyState(int newvar)
  {
    i = newvar; // depending on condition 
  }
}

You make a requested variable.

class Foo
{ int i;
  int requesti;
public:
  void modifyState(int newvar)
  {
      requesti = newvar;
  }
  void RunUpdate()
  {
      // Check all dependices relating to i, and if I can  be chanaged. Perform actions depending on i.
  }
}

Although the above is a suggestion. Either way I think have weak coupling between your modules is better than requiring modules to understand the complete state of an object before modifiying it.
entity Send private email
Friday, May 25, 2007
 
 

This topic is archived. No further replies will be accepted.

Other recent topics Other recent topics
 
Powered by FogBugz