Simplifying TryGet methods to execute logic

Question

Sorry my title is terrible i couldn't think of a concise title to use.

I have a bit of code that needs to consume two quantities from two objects, this is done using a TryGet() type of method and the code looks like this:

if(A.TryGet(resource, total) && B.TryGet(resource2, total2)){
    //do what ever
}

The problem is, if A.TryGet() is true but B.TryGet() is false, then I have taken resources from A but not B so now I have to return the resources back to A. This feels messy to me.

How then do you make this kind've problem cleaner?

My current idea is to check if I can take the resources from A and B then take the resources:

if(A.CanGet(resource, total) && B.CanGet(resource2, total2)){
    A.Take(resource, total);
    B.Take(resource2, total2);
}

This works but the Take method would be doing the same thing because it too would need to check if I can take beforehand even if it doesn't respond with a true/false:

public void Take(Resource resource, int total){
    if(CanGet(resource,total))
       _quantity -= total;
}

So here I have now ran the same CanGet method twice, one in the initial if statement and then a second time when calling the Take method. So we got some duplicate code running one line after another and kind've smells off.

Alternatively the Take method takes the resource without any checking. So it does not call CanGet. But this seems like a bad decision to make it public since you could easily get negative values. It is making the assumption that the calling script has done the logic checks beforehand - I don't think that is a wise choice.

What then is the best way to set this up ?

Answer 1

You are trying to update two resources together, as a single transaction that either fully completes or does nothing.

This is a difficult problem, no matter how you turn it.

I can think of three general approaches you can take to make sure the transaction completes fully or not at all:

• check before making the change
• revert if the change fails
• work on a copy of the resource

Check before making the change

Your CanGet() approach is the “check before making the change” variant. While this approach is annoying to use, this annoyance stems entirely from the inherent complexity of juggling two resources together. This approach is fairly simple, so it is often a great design. However, it is also easy to use incorrectly – the programmer must make sure to pay attention to do everything correctly. I'd probably want to write that transaction as:

bool TryGetTwoResources() {
  // check that the transaction can complete
  if (!A.CanGet(resource, total)) return false;
  if (!B.CanGet(resource2, total2)) return false;

  // perform the changes
  bool ok = true;
  ok &= A.TryGet(resource, total);
  ok &= B.TryGet(resource2, total2);

  if (!ok) {
    // ERROR: transaction failed despite prior check
  }

  ... // do whatever
  return true;
}

This strategy does have problems if the resources are used concurrently, because the state of the resource can change between the check and the change. Such time-of-check to time-of-use race conditions are sometimes also exploitable security vulnerabilities. For example, if the resources in questions are bank accounts, such an attack might make it possible to spend the same money multiple times.

To prevent such issues, an exclusive lock on the resource would be needed during the transaction including during the check.

Revert if the change fails

Instead of doing the check first, we can just check afterwards. If the transaction failed, we can restore a backup. In many cases, this is simpler than performing the check first, because it involves less logical duplication. Such an optimistic approach can also be more performant, if you can assume that rollbacks will be rare and if backups are very cheap.

bool TryGetTwoResources() {
  // make a backup
  var backup = resource.Backup();
  var backup2 = resource2.Backup();

  // optimistically apply the change
  book ok = true;
  ok &= A.TryGet(resource, total);
  ok &= B.TryGet(resource2, total2);

  // revert change if necessary
  if (!ok) {
    resource.Restore(backup);
    resource2.Restore(backup2);
    return false;
  }

  ... // do whatever
  return true;
}

Again, this critical section should be protected via mutexes so that no other changes are possible between the backup and the potential restoration of the backup.

Such a reversal-based approach is also easy to write in an exception-safe manner. Instead of tracking success/failure of each individual operation in an ok boolean or in a conditional, we could use a similar variable as a checkpoint:

bool checkpoint = false;
try {
  // assume that Take() throws an exception if it fails
  A.Take(resource, total);
  B.Take(resource2, total);
  checkpoint = true;
}
finally {
  if (!checkpoint) {
    ... // restore backups
  }
}

Work on a copy of the resource

Mutable state is complicated, and requires locks/mutexes for synchronization. A more functional design in which we can replace the old resource state with a new resource state can also be an attractive approach (see also the talk Functional Core, Imperative Shell). Instead of a TryGet() function that modifies a resource, I'll assume that Take() creates a new representation of the resource that indicates the new state, without modifying its inputs. There would also be an immutable Resources object containing all resources in the system. Then:

bool TryGetTwoResources() {
  // get a snapshot of the current resource state
  var oldState = resources;

  // create a tentative new state
  var newState = new Resources {
    resource  = A.Take(oldState.resource,  total),
    resource2 = B.Take(oldState.resource2, total2),
  };

  // Atomically apply the new state.
  // This will fail if someone else replaced the resources in the meanwhile.
  return Object.ReferenceEquals(
    oldState,
    Interlocked.CompareExchange(ref resources, newState, oldState));
}

This can have an advantage in highly concurrent systems since the critical section is smaller – the calculation of the tentative new state does not have to be protected by mutexes, but we don't know if our new state will be applied until the test-and-set succeeds. Of course, a new state can be re-calculated in a loop until the update succeeds.

Using this approach requires that your software is designed in a suitable manner, so this approach is not appropriate in many scenarios. But when it is possible to use, then this is typically a fairly good design, in particular also because it is safer to use: you don't have to remember to make the correct checks first or to store appropriate backups. Many filesystems and databases use a variation of this approach to safely modify on-disk data structures.

Conclusion

There are many different patterns and approaches for updating two resources together. You have found one of the common approaches, which is a very good and simple approach save for some concerns regarding concurrency. Depending on your overall system design and on your concurrency requirements, other designs with different tradeoffs are also available.

Answer 2

You are overthinking this.

If you just look at this from the point of view of the class itself, your new interface (Take / CanGet) is fine, there is no "duplicate" call inside the class.

From the point of view of a caller, this is also true: when one does not know how Take works internally, one cannot know if some calls happen twice.

Hence, in case you don't get a serious performance drawback (which I guess is unlikely), I would not invest too much thoughts into the repeated call and simply get used to it. Note I used the term "repeated", not "duplicate", because the logic of your code is still in one place, which means even with the same check occuring two times, the code is still DRY.

Answer 3

You should really try to make your TryGet method callable repeatedly without side effects. How you do that will depend on the language; in languages with garbage collection or reference counting you should need to do nothing in your code.