Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I have some methods that perform some data changing in a database (insert, update, and delete). The ORM I'm using return row-affected int values for those type of method. What should I return for "my method", in order to indicate the success/failure state of the operation?

Consider the code that is returning an int:

A.1

public int myLowerLevelMethod(int id) {
    ...
    int affectedRows = myOrm.deleteById(id)
    ...

    return affectedRows;
}

Then usage:

A.2

public void myOtherMethod() {
    ...
    int affectedRows = myLowerLevelMethod(id)

    if(affectedRows > 0) {
        // Success
    } else {
        // Fail
    }
}

Compare to using boolean:

B.1

public boolean myLowerLevelMethod(int id) {
    ...
    int affectedRows = myOrm.deleteById(id)
    ...

    return affectedRows > 0;
}

Then usage:

B.2

public void myOtherMethod() {
    ...
    boolean isSuccess = myLowerLevelMethod(id)

    if(isSuccess) {
        // Success
    } else {
        // Fail
    }
}

Which one (A or B) is better? Or pros/cons of each?

share|improve this question
    
In your "A.2". If zero rows are affected, why is that a failure if zero rows need to be affected? In other words if there is not a database error, why is it failure? –  Jaydee Jul 9 at 9:16
5  
Is there a semantic difference between "unsuccessful" and "zero rows affected"? Eg, when deleting all orders of a customer, there is a difference between "customer does not exist" and "customer has no orders". –  Cephalopod Jul 9 at 10:42
    
Do you consider a delete with zero rows unexpected? In that case throw. –  usr Jul 9 at 20:29
    
@Arian I think that is the real question for me. I think I'd chose B, because with A, my code now contains checking for 0 in some places and for -1 in others –  Hoang Tran Jul 10 at 10:18

6 Answers 6

up vote 18 down vote accepted

Another option is to return a result object instead of basic types. For example:

OperationResult deleteResult = myOrm.deleteById(id);

if (deleteResult.isSuccess()) {
    // ....
}

With this, if for some reason you need to return the numbers of rows affected, you simply can add a method in OperationResult:

if (deleteResult.isSuccess()) {
    System.out.println("rows deleted: " + deleteResult.rowsAffected() );
}

This design allows your system to grow and include new functionality (knowing about affected rows) without modifying existing code.

share|improve this answer
2  
+1 "This design allows your system to grow and include new functionality (knowing about affected rows) without modifying existing code" This is the right way of thinking about this category of questions. –  InstructedA Jul 9 at 16:03
    
I did similar thing in my old project. I have wrapper Request and Result object. Both use Compositions to include more details. and both have basic data in it as well. In this case, the Result object has a status code and a msg field. –  InstructedA Jul 9 at 17:42
    
Especially for a system using an ORM, I would call this best practice. The ORM in question may already include such result object type(s), too! –  Brian S Jul 9 at 19:18
    
Just looking at the OPs example, all I can think of is that deleteById will return 2 at some point :) so definitely a custom type, yes. –  deadsven Jul 9 at 20:43
    
I like this approach. I think it is non-blocking, cover both of my approaches, and modifiable/expandable (in future, if needed). The only downside I can think of is it's a bit more code, especially when it come in the middle of my project. I'll mark this as the answer. Thank you. –  Hoang Tran Jul 10 at 10:26

Returning the number of affected rows is better because it gives additional information about how the operation proceeded.

No programmer will blame you because he/she has to write this to check if they got some changes during the operation:

if(affectedRows > 0) {
    // success
} else {
    // fail
}

but they will blame you when they'll have to know the number of affected rows and they realize there are no method for getting that number.

BTW: if by "failure" you mean a syntactical query error (in which case the number of affected rows is obviously 0) then throwing the exception would be more appropriate.

share|improve this answer
4  
-1 because of the reason you gave. Giving additional information is not always a good idea. Often, better design would led to communicating to the caller just what it needs, and nothing more. –  MainMa Jul 9 at 10:42
1  
@MainMa nothing stands in way of creating overloaded methods. Additionally, in native languages (C family for example), number of rows can be used directly in logic (0 is false, anything else is true). –  PTwr Jul 9 at 11:39
    
@PTwr: so to cope with the fact that the method is returning too much information, you suggest creating an overload? This doesn't seem right. As for "zero is false, non-zero is true", this is not the point of my comment, and is a bad practice in some languages. –  MainMa Jul 9 at 11:50
1  
Probably I am one of the spoiled programmers for I don't think that using any tricks could be considered good practice. In fact, whenever I have to deal with someone else's code I am thankful to whoever wrote it and did not employ any tricks. –  proskor Jul 9 at 12:50
4  
You should always return the number of affected rows here!!! Since you can't possibly know if number of rows = 0 is a failure, or if number of rows = 3 is a success? If someone wants to insert 3 rows and only 2 get inserted you would return true, but it is not right! And if someone wants to update t set category = 'default' where category IS NULL even 0 affected rows would be a success, because now there is no item without category, even if no rows were affected! –  Falco Jul 9 at 15:28

I would not recommend any of them. Instead, return nothing (void) on success and throw an exception on failure.

This is for exactly the same reason I choose to declare certain memebers of class private. It also makes function easier to use. More operational context does not always mean better, but it certainly means more complex. The less you promise, the more you abstract away, the easier it is for the client to understand and the more freedom you have in choosing how to implement it.

The question is how to indicate success/error. In this case it's enough to signal failure by throwing an exception and return nothing on success. Why do I have to provide more than the user needs?

Failures/exceptional situations can happen and then you'll have to deal with them. Whether you use try/catch to do it or examine the return codes, is a matter of style/personal preference. The ideas behind try/catch are: separate normal flow from exceptional flow and let exceptions bubble up to the layer where they can be handled most appropiately. So, as many have pointed out already, it depends on whether failure is really exceptional or not.

share|improve this answer
4  
-1 Why would you return nothing where you could return something, with no negative side effects, that provides extra operational context? –  FreeAsInBeer Jul 9 at 13:13
7  
For exactly the same reason I choose to declare certain memebers of class private. It also makes function easier to use. More operational context does not always mean better, but it certainly means more complex. The less you promise, the more you abstract away, the easier it is for the client to understand and the more freedom you have in choosing how to implement it. –  proskor Jul 9 at 13:27
1  
Well, what data does the user actually needs to get? The question is how to indicate success/error. In this case it's enough to signal failure by throwing an exception and return nothing on success. Why do I have to provide more than the user needs? –  proskor Jul 9 at 13:34
4  
@proskor: Exceptions are for exceptional cases. "Failure" in this scenario may be an expected outcome. By all means, put this forward as a potential alternative, but there isn't enough information here to make a recommendation. –  Nick Barnes Jul 9 at 13:35
1  
-1 Exceptions should not be part of normal program flow. It is unclear what "failure" means in the context of your error, but an exception in the context of a database call should be due to an exception that occurs in the database. Affecting zero rows should not be an exception. A mangled query that cannot be parsed, references a nonexistent table, etc. would be an exception because the database engine would choke on it and throw. –  Snowman Aug 9 at 22:09

"Is this better than that?" is not a useful question when the two alternatives don't do the same thing.

If you need to know the affected row count, then you must use version A. If you don't have to, then you can use version B - but any advantage you might gain in terms of less code-writing effort is already gone since you took the trouble to post both versions to an online forum!

My point is: which solution is better depends entirely on what your requirements specifically for this application are, and you know those circumstances much better than we do. There is no industry-wide, safe-to-use, best-practice, won't-get-fired-over-it choice that is better in general; you have to think about it yourself. And for a decision as easily revised as this one, you needn't spend all that much time thinking either.

share|improve this answer
    
I agree that this depends much on the app requirements. However it seems that this kind of situation is not very unique, and I'm not looking for a silver bullet but just other's experience dealing with the same/similar thing (maybe the question is a bit misleading, I do like suggestions other than A/B) –  Hoang Tran Jul 10 at 10:44

Two of the most important principles in maintainable software design are KISS and YAGNI.

  • KISS: Keep it Simple, Stupid
  • YAGNI: You Aren't Gonna Need It

It is almost never a good idea to put in logic you don't immediately need right now. Among many other people, Jeff Atwood (a co-founder of StackExchange) wrote about this, and in my experience he and other proponents of these concepts are completely right.

Any complexity you add to a program comes at a cost, paid over a long period of time. The program becomes more difficult to read, more complex to change, and easier for bugs to creep in. Don't fall for the trap of adding things "just in case". It's a false sense of security.

You are rarely ever going to get any code right the first time. Changes are inevitable; adding in speculative logic to defensively prepare for unknown future contingencies will not actually protect you from having to refactor your code when the future turns out to be different than you expected. Upkeep on unnecessary/contingent logic is more of a maintainability problem than refactoring later to add in missing functionality.

Thus, since it appears that all your program needs for now is to know whether the operation succeeded or failed, your proposed solution B (return a single boolean) is the correct approach. You can always refactor it later if the requirement changes. This solution is the simplest and has lowest complexity (KISS) and does just what you need and nothing more (YAGNI).

share|improve this answer

The whole rows or an error status

Consider returning the whole rows, at least as a run-time option. In DB inserts you may need to inspect the data that was inserted - since it often will be different from what you sent to the DB; common examples include autogenerated row IDs (that the app will likely immediately need), default values determined by DB, and results of triggers if you use them.

On the other hand, if you don't need the returned data, then you also don't need the affected row count, since it's not helpful for the result of 0. If there are mistakes, then you need to return what kind of mistake happened, in a way consistent to your project error handling principles (exceptions, numeric error codes, whatever); but there are valid queries that will correctly affect 0 rows (i.e. "delete all expired orders" if there actually aren't any).

share|improve this answer

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.