Should I try it straight or should I first check if it can be done?

Question

I have a set of items, and each item in the set must be unique. Item are composed from multiple properties and each property of each item can be changed. But after each change every item in the set must still be unique.

Should I simply try to change the property:

int err = setOfItems.SetPropertyOfItem( item, property, newValue )
if ( err != 0 ) {
    MessageBox( "You cannot change property to this value" );      
    return;
}
// ( ...or similar exception based code )

Or should I first check if the property can be changed and only then change it:

if ( !setOfItems.CanBePropertyChanged( item, property, newValue ) ) {
    MessageBox( "You cannot change property to this value" );      
    return;
}
setOfItems.SetPropertyOfItem( item, property, newValue );

Which approach is better and why?

Answer 1

As long as the SetPropertyOfItem method reliably doesn't change the property when it detects a conflict, I would always choose the first way of doing things. Separating checks from actions can work, but it's prone to misuse by callers who perform the action without performing the check. And folding the check into the action method can lead to wasted effort if you also have a separate check method that a confused caller might call without need.

One problem with returning error codes is that the caller might forget to check it, and falsely assume that a change has occurred when it hasn't. If this is likely to be a problem, raising exceptions instead of error codes is an option.

(There are also people who advocate that you should never return anything from a method with side effects. I consider that short-sighted extremism, like claiming that the 100% pure functional style is the right choice for everything. I think design decisions always have to be made considering the pressures and context in the current situation.)

Answer 2

It depends on domain and UI requirements. Should a site/programs warn a user about possible problems? If so, you need a method to check.

bool CanBeChanged(item, property, newValue);

An usual setter must be an usual. It's void, i.e. it raises an exeption:

void Change(item, property, newValue);

What if a user doesn't want an exception, he wants the return code:

bool TryChange(item, property, newValue);

The "Try" word tells to a user of class, that the method doesn't raise any exceptions.

Since a check for uniqueness can be a long operation, TryChange can lead to error in multithreading app. A user may be in doubt: is your method thread-safe?

bool TryChangeConcurrently(item, property, newValue);

In general it's a bad practice to disclose details of the implementation in names of domain's classes and methods, so TryChange is still preferable. Consider, domain classes must be thread-safe as possible.

Of course, all of this are general considerations.

In C++ can_be_changed should be declared as const. Declaration of change in Java should contain throws clause, while canBeChanged and tryChange shouldn't. In C# any setter method has a form set[item, property] = value. Each language offers its own idioms and we can best follow them.

Summary: I would leave all three methods, and I'd try to make the class thread-safe.

Answer 3

(disclaimer: use of Java knowledge below)

In your second solution, I presume one of the two approaches will take place for CanBePropertyChanged:

1. Loop through all the other entries of your Set to make sure none contain oldProperty1, oldProperty2... newPropertyN.
2. Construct a temporary object temp with oldProperty1, oldProperty2... newPropertyN and using a standard method, e.g. Set.contains(temp), check that the method returns false.

If there are no matches, then your SetPropertyOfItem will apply the new property without further checks.

In your first solution, I presume it is doing something similar to the approaches for CanBePropertyChanged, just that this is done as the first step within SetPropertyOfItem. If the Item cannot be modified, then you return a non-zero 'error code'.

The problem I see with (1) above is that you will be duplicating your class's equivalence checking, which may not be future-proof if your Items gain new properties or constraints that affects how this equivalence is done. For both solutions, concurrency, should you choose to implement that, introduces the small possibility that the actual modification of your Item object is not in sync with other concurrent read/write operations on either the Set or Item object.

Alternatives?

Therefore, I'll approach this by asking this question: How costly is it to re-construct a new updatedItem with oldProperty1, oldProperty2... newPropertyN? If that is a relatively cheap operation (e.g. you don't need to send these properties to a database to retrieve data that needs further parsing), why not consider making Item an immutable class? This will be similar to using approach (2), just that you will be adding the non-temporary (permanent till the next modification) new updatedItem into your Set, and removing the old originalItem from it. If it helps, you can even introduce a bit of syntactic sugar:

Item updatedItem = oldItem.createWithUpdate( property, newValue ); // creates a new object
if ( set.add( updatedItem ) ) {
    set.remove(oldItem);
} else {
    log.warn( "You cannot change property to this value" );
}

Your code can be simpler as a result. Also, a Set by definition does not contain duplicate values, so the statement

But after each change every item in the set must still be unique

Is practically a given after an add() operation (which, in Java, returns true if the Set did not already contain the specified element). Also, when your class is immutable, there is less cause for worry introducing concurrency, since objects themselves wouldn't be mutated out-of-sync. You will only need to code defensively against concurrent operations on your Set object.

---

To answer the question directly

Ok, if Item is in fact already immutable, or you prefer not to make the proposed changes above, and I have to take my pick from either two approaches, I will go with the first solution. However, instead of an int-based return code, I will either return a boolean or use exception-handling. I will favor the first solution because it opens up the possibility in the future to change the implementation of SetPropertyOfItem, broadly speaking. @Kilian Foth's answer says it better in this regard.