Setters

[A Alqtn]

Hello everyone,

I want to know how to inform the user a setter method about the success or failure of setting a value for an instance variable. For example:

public boolean setFace(int face) {
if( face >= 0 ) {
this.face = face;
return true;
}

return false;
}

is it better to use the boolean return type for the above setter method or it is better to use a throws clause?

Thanks

Alqtn

[Stuart Ash]

It depends on what you want the caller of the method to do with the answer, the severity of the non-settability, etc. Imagine yourself as the user of this code, and write code which would call this method. That might make it clearer in your mind as to how best this can be used, and thus designed.

[A Alqtn]

I am talking in general. Many others might use different approaches to this, but what I want to know which is better in general to depend on exceptional hadling or return a value from the method to indicate its success or fail.

If you use a simple return value from a setters then you have to use if statements for the setters. This is anoying at the driver level.

Alqtn.

[Ilja Preuss]

Generally, I'd try to avoid designs that make this necessary - it sounds like a code smell to me. I'd wonder whether the logic that is calling the setter is at the wrong place.

It's hard to say more without having an example...

[A Alqtn]

Here is an example:

public class A {
private int bonus;


public boolean setBonus(int bonus) {
if( bonus > 0 ) {
this.bonus = bonus;
return true;
}

return false;
}
}


public class B {
public static void main(String [ ] args) {
A a = new A();

int bonus = read from the user the bonus value;

if( a.setBonus( bonus ) == false )
System.out.println("invalid bonus value);
}
}

[Michael Duffy]

Originally posted by A Alqtn:
Here is an example:

public class A {
private int bonus;


public boolean setBonus(int bonus) {
if( bonus > 0 ) {
this.bonus = bonus;
return true;
}

return false;
}
}


public class B {
public static void main(String [ ] args) {
A a = new A();

int bonus = read from the user the bonus value;

if( a.setBonus( bonus ) == false )
System.out.println("invalid bonus value);
}
}

That's a C-style of coding - having clients check a return value to see if an operation worked.

Another way to do it might look like this:

  
public class A 
{
   private int bonus;
   
   /**
    * Write access to the bonus value
    * @param bonus new value
    * @throws IllegalArgumentException if the input value is negative
    * (Where I can I get an employement application?  I want to work there.)
    */ 
   public void setBonus(int bonus) 
   {
      if (bonus < 0)
         throw new IllegalArgumentException("bonus values cannot be negative");
 
      this.bonus = bonus;
   }
}

Now clients don't have to check a value to see if the set succeeds, and they'll be told if the operation fails.

Exceptions can be expensive, and there's an argument that says they shouldn't be used for normal program flow. But you've documented the proper use of your class in javadocs, so any user who wants to use your class will be fully-informed as to its proper use. I think it's a better alternative.

[Ilja Preuss]

if( a.setBonus( bonus ) == false )
  System.out.println("invalid bonus value);
}

Mhh, I don't think this reads very well. I think I'd typically prefer something along the lines of

if( !a.isValidBonus( bonus ) ) {
  System.out.println("invalid bonus value");
} else {
  a.setBonus(bonus);
}

setBonus could throw an exception if an invalid bonus is tried to set, but that should indicate a programming error.

[Stuart Goss]

What about something like this?

public void setBonus(int aBonus) {
    if( aBonus > 0 ) {
        bonus = aBonus;
    } 
    else {
        bonus = -1;
    }
}

[Jim Yingst]

Stuart - personally I don't like that very much. If elsewhere in the code someone is calling setBonus(-10) or setBonus(-5000) for some reason, I don't want to just silently set it to -1. Chances are that whoever wrote the setBonus(-10) thought the method did something different than it actually does. So I'd much rather draw attention to the problem by throwing an exception:

public void setBonus(int aBonus) {
    if (aBonus < 0)
        throw new IllegalArgumentException("Bonus may not be negative: " + aBonus);
    bonus = aBonus;
}

Note that if there's no bonus, the default value of 0 seems like a perfectly logical way to represent that. I don't see a need for a special -1 value here.

Heh - I wrote the above without noticing that Michael Duffy had already suggested the exact same thing. So consider this a bid "me too".
[ January 02, 2006: Message edited by: Jim Yingst ]

[A Alqtn]

Michael,

So you suggest throwing an exception for setters. What about if we make it a standard. Meaning all classes developed use exception handling instead of the C-Style as you mentioned. When you say exception handling is expensive? In what factors might this approach affect the application.

Alqtn

Originally posted by Michael Duffy:

  
public class A 
{
   private int bonus;
   
   /**
    * Write access to the bonus value
    * @param bonus new value
    * @throws IllegalArgumentException if the input value is negative
    * (Where I can I get an employement application?  I want to work there.)
    */ 
   public void setBonus(int bonus) 
   {
      if (bonus < 0)
         throw new IllegalArgumentException("bonus values cannot be negative");
 
      this.bonus = bonus;
   }
}

Now clients don't have to check a value to see if the set succeeds, and they'll be told if the operation fails.

Exceptions can be expensive, and there's an argument that says they shouldn't be used for normal program flow. But you've documented the proper use of your class in javadocs, so any user who wants to use your class will be fully-informed as to its proper use. I think it's a better alternative.

[Layne Lund]

IllegalArgumentException is a runtime exception. Generally, runtime exceptions indicate a programmer error and not a system error. This means that such an exception should NOT occur in a production environment. Persumably, the exception is thrown to facilitate testing so that such programming errors can be easily found and fixed.

I think in most cases this would be an acceptable solution as long as the whole system is thoroughly tested to ensure that such exceptions are not thrown after the program has been distributed for production. I also agree with Ilja that the way to handle this type of situation might not be entirely universal and depends on the exact situation. Don't get stuck into one solution so much that you are not flexible enough to modify it when there are reasons to do so.

Layne

[A Alqtn]

Originally posted by Layne Lund:
IllegalArgumentException is a runtime exception. Generally, runtime exceptions indicate a programmer error and not a system error. This means that such an exception should NOT occur in a production environment. Persumably, the exception is thrown to facilitate testing so that such programming errors can be easily found and fixed.

I think in most cases this would be an acceptable solution as long as the whole system is thoroughly tested to ensure that such exceptions are not thrown after the program has been distributed for production. I also agree with Ilja that the way to handle this type of situation might not be entirely universal and depends on the exact situation. Don't get stuck into one solution so much that you are not flexible enough to modify it when there are reasons to do so.

Layne

Layne,

When reading a value from a text field, a runtime error might happen and it is not a programmer error. Such a exception can happen a production environment. What I mean is that not all run time errors represent progrmmaer error.

Alqtn

[Layne Lund]

Originally posted by A Alqtn:



Layne,

When reading a value from a text field, a runtime error might happen and it is not a programmer error. Such a exception can happen a production environment. What I mean is that not all run time errors represent progrmmaer error.

Alqtn

I think there is a difference between what you are calling a "runtime error" and what Java calls a "runtime exception" (or more specifically RuntimeException and its subclasses). In Java, you RuntimeException and its subclasses typically (although not always) represent a programming error.

In your particular example, I would imagine that you will use Integer.valueOf() to get the numeric value of the String typed in the text field. Now that I look at the Javadocs, it looks like this method throws NumericFormatException which is a RuntimeException. That seems a little odd to me from my basic understanding of what RuntimeExceptions are supposed to mean, but some people argue that Java's exception handling mechanism is somewhat flawed anyways.

I think the point I was trying to make here is something like Stuart says. You need to determine the severity of the error. Is this error caused by a "reality-check" that programmers should use to make sure they are using the class correctly? This is basically the case that I have described where the error indicates a programming error and should only occur during testing and debugging. On the other hand, does the error indicate a user or system error that the programmer cannot control? In such a case, the overhead of exception handling may be something you need to consider. You should also consider how often you expect this type of error to occur.

Anyways, that's just my two cents. In the end, I think this will come down to a design decision which only you can make.

Layne

[Garrett Rowe]

I gotta agree with the exception throwers here ;-). Especially if its well documented, the user of the method should check the parameter before it is passed to the method. Progammatically (is that a word?) its just about as easy as testing to see if the method passed or failed. But passing a negative value as a paramter when the method expects value >= 0, is as bad imho as passing a String when the method expects a double. In each case, it is the user's resposibility to check the input before calling the method.

[Mingwei Jiang]

I prefer the exception way, because it's more readable. But you should take care of the exception type, make one for yourself, like InvalidBonusException, it's reasonable.

Exception does cost time and thus expensive, but I consider readability more important.

[Mike Noel]

Having the caller check the validity of the data before sending it to the setter means that the caller has to know something about the data and how it will be used by the class. In this bonus example it seems obvious that bonuses need to be >= 0 but that's taking advantage of knowledge that maybe should only be in the class. Of course if the API for this class is well documented it can state the valid ranges.

A slightly more complex example would be where the bonus has to be >= 0 and <= 10% of another field, (maybe "gross"). Now the client has to know a bit of the business logic that the class is implementing. You can imagine even more complicated cases. Following the "check for validity first" path can mean that the client ends up replicating a lot of the class's own logic. IMHO this defeats the purpose of setters.

I suggest that the class provide a method that can test for valid values or throw an exception. Making a checked exception seems like a reasonable idea for a class that's a bit complex. The client can wrap all the setter calls in a single try block and create appropriate catch code to deal with invalid input.

These are design decisions though. They have to be considered in light of the actual problem you're solving. You don't want to create a huge mechanism of exception handling (custom exceptions, detailed catch blocks, etc.) when you're class is very simple.

_M_

[Ilja Preuss]

Originally posted by Mike Noel:
A slightly more complex example would be where the bonus has to be >= 0 and <= 10% of another field, (maybe "gross"). Now the client has to know a bit of the business logic that the class is implementing. You can imagine even more complicated cases. Following the "check for validity first" path can mean that the client ends up replicating a lot of the class's own logic. IMHO this defeats the purpose of setters.

You don't have to replicate the logic. The client simply could simply call an isValidBonus method before calling the setter.

I suggest that the class provide a method that can test for valid values or throw an exception. Making a checked exception seems like a reasonable idea for a class that's a bit complex. The client can wrap all the setter calls in a single try block and create appropriate catch code to deal with invalid input.

I think there are better solutions to this, such as the Builder pattern, or a Collecting Parameter. Code doesn't *have* to be complex, it's always a choice we make.

[Mike Noel]

You're right. The class can provide a validity testor (that was the gist of my first suggestion). I'm not up on all of the patterns so I don't know about the ones you mentioned but I'm sure they would be great.

_M_

[A Alqtn]

Originally posted by Ilja Preuss:


I think there are better solutions to this, such as the Builder pattern, or a Collecting Parameter. Code doesn't *have* to be complex, it's always a choice we make.

---------------------------

The whole point I am trying to reach is when you design an application that reads values from the user (through a form or whatever means you can design), the validity checking of those values should be made at which level? Should they be checked at the driver class level and then when everything is OK call the setters with proper data? In this case I think you might not need to have any validity checking in the setter methods, since they receive valid data any way. Remeber you have developed everythinh.


However, if you want to rely on the setter method to reject invalid data, then I believe exception mechanism is a good choice.

My question again, which approach is used most and why?

[Ilja Preuss]

I'm not sure whether that's what you were saying, but the best thing to do is for the GUI not to allow the input of invalid data - for example by providing a widget that you simply can't use that way, or by disabling the OK button if the data is invalid, etc.

Regarding setters not accepting invalid input, I often have them throw an Exception in that case. But that Exception is *not* supposed to be used by programming logic to decide whether the input was valid - it denotes a failure of the programming logic to reject the invalid data earlier.