Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite
1

I have some code that looks like this:

if (condition1) {
    doSomeObjectSetup();
    try {
        saveObjectToDatabase()
        return theObject;
    } catch (ValidationException) {
        setupError(thing);
        setupError2(thing2);
        displayErrorInUI();
        return 400 Bad Request;
    }

} else {    // condition1 is false
    setupError(thing);
    setupError2(thing2);
    displayErrorInUI();
    return 400 Bad Request;
}

The duplicated code in setting up the error condition worries me. Is there a good way to refactor this so the error code doesn't repeat?

share|improve this question
don't know what language is this (I assume is java, given the formatting), but I think you don't need return; after you throw an exception. – w0lf Apr 3 '12 at 18:27
Sorry it's not quite throwing an exception, it's a webapp and setting an error message in the UI. – Kevin Burke Apr 3 '12 at 18:30
Then I think throwError() is a terrible name for that method – w0lf Apr 3 '12 at 18:33
Thanks, it's just an example and not the code i'm actually using for the site. – Kevin Burke Apr 3 '12 at 20:00
yes, sorry for being so harsh in my previous comment. Thanks for updating the question; I've updated my answer accordingly – w0lf Apr 3 '12 at 20:20

4 Answers

up vote 6 down vote accepted

You could use throw to implement the branching logic instead of if - else:

try {   
    if (!condition1)
        throw new ValidationException("condition1 not met");

    doSomeObjectSetup();    
    saveObjectToDatabase()
    return theObject;
} catch (ValidationException) {
    setupError(thing);
    setupError2(thing2);
    displayErrorInUI();
    return 400 Bad Request;
}
share|improve this answer
I've removed the first option, as I think it's less efficient now, given the code update in the question – w0lf Apr 3 '12 at 18:36
Interesting approach but it's not ideal because if ValidationException is abstract you'd have to instantiate and throw some subclass instead, which would make the code harder to follow. – seand Apr 3 '12 at 22:02
@seand Good observation. As you can tell, I'm a C# developer and didn't know that. :) However, even throwing a subclass of ValidationException does not affect the readability of this code IMO. – w0lf Apr 4 '12 at 6:27
It does if the subclass doesn't look like it's related to ValidationException. You could, of course, create a custom subclass to give it a name like InvalidArgumentValidationException, which makes the relationship obvious, but that results in extra code. – seand Apr 5 '12 at 14:54
up vote 5 down vote

Another alternative is to create a method to handle the error. I'm not sure if I've written it in the correct language but you should get my drift.

Perhaps something like:

MyObject getMyObject(bool condition1) {

   if (condition1) {

       doSomeObjectSetup();

       try {

           saveObjectToDatabase()
           return theObject;

       } catch (ValidationException) {
          // error reporting specific to exception here   
       }    
   }

   // I would prefer to return an object here over throwing an exception however
   // it depends on if the object is the same type as theObject?  For now we will
   // just throw an exception
   throwBadRequestException();
}

   void throwBadRequestException() {
      setupError(thing);
      setupError2(thing2);
      // throw a custom exception
      throw new BadRequestException();
   }

I would probably move the displayErrorInUI() outside of this routine so that you can handle the error's it returns in different ways. Something like:

try {

   bool myObject = myMethod(condition1);

   // do something with myObject
}
catch(BadRequestException ex) {
   displayErrorInUI();
}

If you find yourself doing this a lot then you could wrap the the call to getMyObject etc in an getMyObjectUI() or some such and passing a call back method for the do something part of it.

share|improve this answer
up vote 3 down vote

You can just use a fall-through technique:

if (condition1) {
    doSomeObjectSetup();
    try {
        saveObjectToDatabase();
        return theObject;
    } catch (ValidationException) { /* fall through */ }
}

// condition1 is false OR saveObjectToDatabase() threw an exception
setupError(thing);
setupError2(thing2);
displayErrorInUI();
return 400 Bad Request;
share|improve this answer
I don't like this because it makes the error handling look like part of the normal flow through the method. Yes, it is normal to handle errors, but this style looks to me like you expect errors to happen every time. Leonid's answer is more my style. – jimreed Apr 5 '12 at 13:11
"looks to me like you expect errors to happen every time" - @jimreed: It doesn't look that way to me at all. Can you elaborate? – seand Apr 5 '12 at 14:52
fall-through sounds a lot like case statement short circuiting i.e. omitting the break between case statements because you want the same block of code to run. it's not clean code. – booyaa Apr 25 at 8:33
up vote 4 down vote

Yet another take on it. I like it because I think you can follow what is going on without too much head scratching.

// These always occur together
object setupAndDisplayError(thing, thing2) {
    setupError(thing);
    setupError2(thing2);
    displayErrorInUI();
    return 400 Bad Request;
}

object DoSomething() {
    if (!condition1) {
        return setupAndDisplayError(thing, thing2);
    }

    doSomeObjectSetup();
    try {
        saveObjectToDatabase()
        return theObject;
    } catch (ValidationException) {
        return setupAndDisplayError(thing, thing2);
    }
}
share|improve this answer
1  
Yep, nice and simple – dreza Apr 3 '12 at 23:36

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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