2

I am learning how to write neat and organized code in Java. Can a set() method return a value or is there a more efficient/readable way of doing this?

public class Car {

     private boolean mHasAxles;
     private boolean mHasTires;
     private Tires mTires;

     public setAxels(boolean hasAxles) {
          mHasAxels = hasAxles;
     }

     public boolean hasAxles() {
          return mHasAxles;
     }

     public boolean setTires(Tires tires) {
          if(hasAxles()){
               mTires = tires;
               mHasTires = true;
               return true;      // Returns true if it was able to set tires
          }
      return false;  // Returns false because car did not have axels
                     // Therefore, tires could not be set
     }
}

In this example, my question is specifically about the setTires() method. Should the class check whether the car has axles when setting the tires or should the logic be left to the class that uses Car? Should the setTires() method be called something else since it returns a value?

  • 1
    "Conventionally", no. – user2864740 Apr 22 '15 at 19:04
  • This doesn't pertain to your main question but does have to do with writing organized code: the field mHasAxels doesn't make a lot of sense for the Car class since practically all cars have axels...I think. If you want to work with cars with no axels, I would subclass Car and make something like AxellessCar. – 11th Hour Worker Apr 22 '15 at 19:09
  • 2
    Another way to do this would be to throw an Exception, like the IllegalStateException – ControlAltDel Apr 22 '15 at 19:09
  • It may not be conventional, but there's really nothing wrong with that example. I think a more conventional way would be to check if it has axles before calling the set method, or know that if you're calling the `setTires` method then the car has axles, i.e. there's no way the car couldn't have axles. – DigitalNinja Apr 22 '15 at 19:10
  • is this good coding practice? NO! ever heard of "separation of concerns"? if you have a need for this type of code your fundamental design is wrong. you need to read "Clean Code" by Uncle Bob – Hector Apr 22 '15 at 19:12
  • Possibly a duplicate of: http://stackoverflow.com/questions/1345001/is-it-bad-practice-to-make-a-setter-return-this – cybersam Apr 22 '15 at 19:17
  • An `addTires` sounds like a better approach and it would make more sense for such a method to return `true/false` to indicate the outcome. Otherwise setters sometimes return `this` in order to chain setters. – maba Apr 22 '15 at 19:41
  • @Hector Thank you! That's what I was looking for. I'll definitely read that book. So to follow the "separation of concerns" design principle in this example, would the class that uses Car have to check to see if the car has axles before setting the tires, since the Car cannot think on its own? Would this be best practice? –  Apr 23 '15 at 16:13
  • you also need to consider encapsulation. Of course programmatic Cars can think for themselves :). From the information you have given in your question it is impossible to say how you can complete the overall design of your application. If i were you i would take a step back and think of the BIG picture. Are you trying to represent all vehicles types "Known To Man"? or just types of cars? – Hector Apr 23 '15 at 17:23

3 Answers3

2

Strictly conventionally - no, a setter usually returns void.

Having said that, you are free to return boolean if you wish - conventions are often broken (even in the internal java api's) and the method signature, including it's return type, should inspire an interested developer to navigate into the code to see exactly why a boolean is returned.

To make it clearer, you might want to use a different method name, e.g. setTiresIfAble(Tires tires), or you could alternatively return void and throw an Exception as per below:

 public void setTires(Tires tires){
     if(!hasAxels()) {
         throw new IllegalStateException("no axels!");
     }
     mTires = tires;
     mHasTires = true;
 }

Incidentally, mHasTires is redudant, you can always check if tires == null.

Lastly, you can avoid the m or Hungarian notation in java (as per convention), like this:

 public setAxels(boolean hasAxels){
      this.hasAxels = hasAxels;
 }
vikingsteve
  • 38,481
  • 23
  • 112
  • 156
1

A setSomething method shouldn't return anything

A trySetSomethingmust return a boolean which say if the set has been successful or not.

Why ? When you are writting some code, in Java, in C++, in any langage, you want that any reader of your code, which is probably a human being, can access to the knownledge of most of what a method do, just reading his name.

To complete this assertion, we can study the case that a set can fail. There are two possibilities to deal with failures, depending on the scope of the method :

  1. if the scope of setSomething is protected, private or package, it means that you have control, as API developper, on the way it will be called. Possible failures can be managed with assertion, or with RuntimeException (as it's not necessay to declare a throws clause in the method signature).

  2. if the scope of setSomething is public, it means that you don't have control, as API developper, on the way it will be call. You have to warn the users of your API that the setSomething isn't error-safe. You have to manage the possible failures with an Exception which has to be declared in a throw clause.

0

You "can" return a value, but by convention setters don't return a value. As setters generally are used in a manner where one doesn't even reference a return value it is quite easy for any caller of your class to ignore that you are returning a value. Nothing stops the called from doing:

myCar.setTires(aTire);

...and ignoring you even return a Boolean, because that is the normal way setters are called and used. This defeats the purpose of your returning a value if the caller can just ignore it and the code appears correct.

My first inclination is to say throw an java.lang.IllegalArgumentException rather than return false.