views:

615

answers:

11

I have been assigned a project to develop a set of classes that act as an interface to a storage system. A requirement is that the class support a get method with the following signature:

public CustomObject get(String key, Date ifModifiedSince)

Basically the method is supposed to return the CustomObject associated with the key if and only if the object has been modified after ifModifiedSince. If the storage system does not contain the key then the method should return null.

My problem is this:

How do I handle the scenario where the key exists but the object has not been modified?

This is important because some applications that use this class will be web services and web applications. Those applications will need to know whether to return a 404 (not found), 304 (not modified), or 200 (OK, here's the data).

The solutions I'm weighing are:

  1. Throw a custom exception when the storage system does not contain the key
  2. Throw a custom exception when the ifModifiedSince fails.
  3. Add a status property to the CustomObject. Require caller to check property.

I'm not happy with any of these three options. I don't like options 1 and 2 because I don't like using exceptions for flow control. Neither do I like returning a value when my intent is to indicate that there was no value.

Nonetheless, I am leaning towards option 3.

Is there an option I'm not considering? Does anyone have strong feelings about any of these three options?


Answers to this Question, Paraphrased:

  1. Provide a contains method and require caller to call it before calling get(key, ifModifiedSince), throw exception if key does not exist, return null if object has not been modified.
  2. Wrap the response and data (if any) in a composite object.
  3. Use a predefined constant to denote some state (UNMODIFIED, KEY_DOES_NOT_EXIST).
  4. Caller implements interface to be used as callbacks.
  5. The design sucks.


Why I Cannot Choose Answer #1

I agree that this is the ideal solution, but it was one I have already (reluctantly) dismissed. The problem with this approach is that in a majority of the cases in which these classes will be used, the backend storage system will be a third party remote system, like Amazon S3. This means that a contains method would require a round trip to the storage system, which would in most cases be followed by another round trip. Because this would cost both time and money, it is not an option.

If not for that limitation, this would be the best approach.

(I realize I didn't mention this important element in the question, but I was trying to keep it brief. Obviously it was relevant.)


Conclusion:

After reading all of the answers I have come to the conclusion that a wrapper is the best approach in this case. Essentially I'll mimic HTTP, with meta data (headers) including a response code, and content body (message).

+4  A: 

You can create a special final CustomObject as a "marker" to indicate unchanged:

static public final CustomObject UNCHANGED=new CustomObject();

and test for a match with "==" instead of .equals().

It might also work to return null on unchanged and throw an exception on does not exist? If I had to choose one of your 3, I would choose 1 because that seems the most exceptional case.

Software Monkey
This does introduce a series of if, then, else checks that every call to this method has to muck around with. You're almost better off then with Exceptions, cause at least they're enforced, and more structured.
Spencer K
Yes, but exceptions introduce a series of try/catch/finally, so the net gain is only enforcement, at the cost of performance? And the exception handling is more verbose, and more distant from the causative code. So... it depends... The OP asked how to avoid exceptions.
Software Monkey
+2  A: 

Looking for an object that does not exist seems like an exceptional case to me. Coupled with a method that allows a caller to determine if an object exists, I think it would be ok to throw the exception when it doesn't.

public bool exists( String key ) { ... }

Caller could do:

if (exists(key)) {
   CustomObject modified = get(key,DateTime.Today.AddDays(-1));
   if (modified != null) { ... }
}

or

try {
    CustomObject modified = get(key,DateTime.Today.AddDays(-1));
}
catch (NotFoundException) { ... }
tvanfosson
A: 

If it is acceptable, you may return an amplified CustomObject (a wrapper), which contained values which represented the object and its modification state, if any, etc.

eulerfx
+2  A: 

The problem with exceptions is they are meant to signal a "fail fast" scenario (i.e. if not processed, an exception will stop an application) due to an exceptional and abnormal behavior.

I do not think that "the scenario where the key exists but the object has not been modified" is an exceptional one, certainly not an abnormal one.

Hence I would not use exception, but rather I would document the action the caller need to do in order to correctly interpret the result (property or special object).

VonC
Yeap, in this case an exception a bad decision.
OscarRyz
Where did the key come from? If you have the key, then the object ought to exist and if it doesn't then it's an exceptional case.
tvanfosson
@Buzzer: may be so, but does the application has to *stop* if your exception goes unchecked ? I do not think so.
VonC
Then make the exception checked. Unchecked exceptions are either unrecoverable or unhandle-able at the current level. They can be bubbled up to a layer that notifies someone of the event, then waits till action is been taken. Exceptions are not the best solution, but are viable in this instance.
Spencer K
+1  A: 

How strict is the requirement for that method signature?

It seems as though you are working on a project that is still in progress. If the consumers of your class are other developers, can you convince them that the method signature they have asked for is insufficient? Perhaps they haven't yet realized that there should be two unique failure modes (key does not exist and object has not been modified).

I would discuss it with your supervisor if that is an option.

e.James
+5  A: 

With the given requirement you cannot do this.

If you designed the contract, then add a condition and make the caller invoke

exists( key ): bool

The service implementation look like this:

if ( exists( key ) ) {
    CustomObject o = get( key , Date modifSince );
    if( 0 == null ) { 
      setResponseCode( 302 );
    } else {
      setResponceCode( 200 );
      push( o );
   }

} else {
      setResponseCode( 400 );
}

The client remains unchanged and never notice you've validated upfront.

If you didn't design the contract Probably there's a good reason for that or probably it's only the designer ( or architect ) fault. But since you cannot change it, then you don't have to worry either.

Then you should adhere to the specifications and proceed like this:

 CustomObject o = get( key , Date modif );

 if( o != null ) {
      setResponseCode( 200 );
      pus( o );
  } else {
     setResponseCode( 404 ); // ether not found or not modified.
  }

Ok, you're not sending 302 in this case, but probably that is the way it was designed.

I mean, for security reasons, the server should not return more information than that [ the probe is get( key, date ) only return either null or object ]

So don't worry about it. Talk with your manager and let him know this decision. Comment the code with this decision too. And if you have the architect in hand confirm the rationale behind this strange restriction.

Chances are they you didn't see this coming and they can modify the contract after your suggestion.

Sometimes while wanting to proceed right we may proceed wrong and compromise the security of our app.

Communicate with your team.

OscarRyz
404 == not found, 304 == not modified. If one result means two different things, that's going to bite someone someday.
Dustin
Yes, but maybe that's the intention. For instance. Log in into Yahoo! with a fake account. The server says: "Invalid ID or password." They don't say "the account does not exists" There's a good reason for this. Yahoo won't tell you if an account exist and the password is wrong. Even when they could.
OscarRyz
This may be a security constraint imposed by the security consultant in the enterprise ( we all have one right? ) Oooor maybe, the designer just forgot this scenario ( that never happens right? The design is ALWAYS correct ) We don't know. Problably this service was used internally and now is pub
OscarRyz
+1  A: 

I'd still return null.

The intention of the property is to return the object that was modified after the specified date. If returning null for no object is ok, then surely returning null for an unmodified object is ok too.

I personally would return null for a non-modified object, and throw an exception for a non-existing object. That seems more natural.

You're quite right to not use exceptions for flow control BTW, so if you only have those 3 options, your gut instinct is right.

gbjbaanb
This is the only reasonable solution. If the qualifier for the method is not met, then it should return null, I don't see the point of anything else. The null on invalid key is questionable and could go either way. A Map would return null, other classes would throw IllegalArgumentException.
Robin
Of course the invalid key case is already determined, so null for both.
Robin
+1  A: 

You could follow the .Net library pattern of and have a public static readonly field in the custom object called CustomObject.Empty that is of type CustomObject (like string.Empty and Guid.Empty). You could return this if the object is not modified (the function consumer will need to compare against it).
Edit: I only just spotted that you're working in Java, but the principle still applies

This gives you the option of the following

  • Return null if the key doesn't exist.

  • Return CustomObject.Empty if the key exists but the object has not been modified.

The drawback is that the consumer would need to know the difference between a null return value and a CustomObject.Empty return value.

Perhaps the property would be more aptly called CustomObject.NotModified as Empty is really intended for Value types as they cannot be null. Also NotModified would convey the meaning of the field more easily to the consumer.

DoctaJonez
+7  A: 

It sounds like you actually want to return two items: the response code and the object found. You might consider creating a lightweight wrapper that holds both and return them together.

public class Pair<K,V>{
  public K first;
  public V second;
}

Then you can create a new Pair that holds your response code and the data. As a side effect to using generics, you can then reuse this wrapper for whatever pair you actually need.

Also, if the data hasn't expired, you could still return it, but give it a 303 code to let them know that it is unchanged. 4xx series would be paired with null.

James
This isn't bad, but this should be more strongly typed, and doesn't need to be made generic.
Spencer K
That Pair type couldn't be any more strongly typed, and why would you not want to make things as generic as possible? Polymorphism is a good thing.
Apocalisp
Instead of returning both a found object and a response code, you could return EITHER the found object or a failure code. This can be made type-safe with a disjoint union type like Either<A, B>.
Apocalisp
Poor choice of wording. Yes, it is strongly typed, but "Pair" is vague. Pair of what? Yes you specify "Pair<CustomObject, Integer>", but what does Integer mean? There's generic, and then there's confusing. Hopefully at least then, the code is typed in a wrapper or something.
Spencer K
+1  A: 

The (intended) interface regarding the requirements is seriously broken. You try to do unrelated things within one method. This is the road to software hell.

Amén! And there's no way back from there.
OscarRyz
I second that! Its not a good situation to be in.
DoctaJonez
+1  A: 

Provide a Callback as the argument where the Callback class could either be event driven, or setter driven.

You have your class's interface define the various errors that can occur, passing in the CustomObject as the parameter for the event, if needed.

public interface Callback {
  public void keyDoesNotExist();
  public void notModified(CustomObject c);
  public void isNewlyModified(CustomObject c);
  .
  .
  .
}

In this way, you allow the implementor of the Callback interface to define what to do when the event occurs, and you can choose through the interface, whether that conditions requires the passing of the retrieved object. Lastly, it reduces the complexity of the logic on a return. Your method does that once. Implementors of the API don't require doing it at all, as it's done for them.

Spencer K