4

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 ?

3
  • I'm guessing the code you posted more or less just an illustrative example, but that it has some structural/conceptual resemblance to the real code; now, my question is, at call site, is there a relationship between total and total2? If there is, maybe you can make the code cleaner by introducing an extra concept - something like a resource pool, where your A & B would ask the resource pool to reserve a certain amount, then see if you can commit at the end. Maybe the resources should not be managed by A & B themselves. Jul 31, 2022 at 20:59
  • I mean, the fact that you're passing in total perhaps indicates that A and B already do not have full ownership over the resources (as you're tracking or calculating the total externally) - which may be perfectly fine: the responsibility of A and B could just be to encapsulate the rules (they would represent the rules), but if that's the case, maybe the _quantity field shouldn't be in those classes. Are they pulling from the same pool of resources, or are there separate pools for A and B each? Jul 31, 2022 at 21:04
  • I'm wondering how realistic your example is relative to the complexity of its code. If you're dealing with a "get" operation which removes the retrieved item in some way and some "add" operation in the target destination, you're likely going to already need a transaction model to ensure this stays in sync (and so your taken goods don't disappear due to an exception) and the transaction model would significantly impact any syntax that you use in the example. For example, if you only commit your changes in the end, the original snippet isn't wrong because you haven't truly "taken" an item yet.
    – Flater
    Aug 1, 2022 at 15:56

3 Answers 3

2

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.

1
  • 3
    There is also variant #4: "Let someone else solve the problem". There are system which already have transactions built in. If you can push the computation into such a system, the problem will be solved for you. E.g. databases or Software Transactional Memory, such as Clojure's Refs, Haskell's STM library or the STM library for F# in FSharpX.Extras. Jul 31, 2022 at 12:47
0

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.

2
  • 1
    Well its not a performance drawback thats for sure, it just felt a bit messy. So it made me wonder if I possibly missed a design choice that would serve me better.
    – WDUK
    Jul 31, 2022 at 1:27
  • @WDUK: see my edit. Imagine for a moment the class would be designed by person X, and used by person Y who does not know the internals. Then none of the would see a problem. Your issue only arises because you know both, a potential calling sequence and the implementation, but that is point of view only relevant for optimization, not for good interface design.
    – Doc Brown
    Jul 31, 2022 at 5:53
-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.

0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.