changing the value of String object in java

Question

I have to set the value of a string object "result" depending on the results of different methods and different if/else conditions. In the end, there would be one (last value set) in the string that i need to use. The first way i did it is:

public void MyClass(){

        public void validateApples() {
            String result = "failure";

            if (condition 1) {
                //do something
            }

            if (condition 2) {
                //do something
            }

            if(condition 3) {
                //do something
            }

            if (condition 4) {

               if (condition a)
                    result = "success";                    
                } else{
                    //One of the first three conditions resulted in failure so don't change string value
                }
            }

            if (condition 5) {
                //Do validation only for xml files
                result = validateMachintoshApples ();
            }
       }

      private String validateMachintoshApples() {
         String result;            

            result = oneStepValidation();             

       return result;
     }


     private String oneStepValidation(){
         String result;           

         if (some condition) {    
            // some code
            result = "success";   
        }else{
            // some code
            result = "failure";
        }
        return result;
    }


}

I feel that i am recreating a new String object many times in different methods of the same class. I don't think its efficient in terms of memory, object creation time and coding standards.

I than moved on to creating one string object "result" for the whole class and assign it "failure" or "success" as i think that would be more clean code.(see code below)

1) But since Stings are immutable in JAVA, will I achieve something this way?

2) Am i still creating new String each time with result = "failure" or result = "success" or is it only a value change? I doubt it is a value change as strings are immutable.

3) Is the second way less readable since the functions are setting the value for result but not returning anything?

4) What should be the best practice in this case?

My second way of coding looks like:

public void MyClass(){

    private String result;

    public String getResult() {
        return result;
    }

    public void setResult(String result) {
        this.result = result;
    }

    public void validateApples() {


        if (condition 1) {
            //do something
            result = "failure";
        }

        if (condition 2) {
            //do something
            result = "failure";
        }

        if(condition 3) {
            //do something
            result = "failure";
        }

        if (condition 4) {

           if (condition a)
                result = "success";                    
            } else{
                //One of the first three conditions resulted in failure 
            }
        }

        if (condition 5) {
            //Do validation only for xml files
            validateMachintoshApples ();
        }
   }

  private void validateMachintoshApples() {

        result = oneStepValidation();             

 }


 private void oneStepValidation(){

     if (some condition) {    
        // some code
        result = "success";   
    }else{
        // some code
        result = "failure";
    }

}

}

Answer 1

Let's look at this for a minute or two...

So you are running some code and expect a string return to tell you how it went. In doing this you noticed (and rightly so) that this evokes a feeling somewhat unpleasant, this just dont feel right you say. The issue however is not one of performance, or rather the performance is but a twig that hides the creepy forest behind.

First change you can do here is to replace the string values you just hard code everywhere with constants. This will cover cases where you make typoes assigning the value leading to the cases not being detected by the caller.

But it still feels wrong no ?

Well, you can help yourself a bit more by helping the compiler. Put all the possible values in an enum and assign that instead. Here we are no longer working with strings directly but actual object-like with a type and a strictly controlled set of values. No typos possible and makes it easier to ensure you cover all the possible return values (in cases where you get more than just "success" or "failure")

If you only ever need two cases then a boolean would do just dandy, true = ok, false = fail...

Yet you can still do better here...

enter Exceptions

All this code is basically to check that something is what it's supposed to be right ? So you basically have a success case and one or more failure types.

so instead of returning a value that needs to be checked don't return anything. IF all goes well the code executes normally. If the validation failed then the failure will throw an exception you will have created (ValidationException or something similar) that will be caught by a caller code, could be the direct caller, could be higher up the stack too. It basically will be caught at a level where something can be done about it.

Need more validation types ? (just like the enum above) create sub-classes of this ValidationException for each type of problems that needs detection. This will unclutter your code that deals with the normal case while giving you the means to aggregate the handling code where it matters.

Long story short, the language offers powerful tools to handle errors like this, embrace them. You will find your code much easier to read and maintain.

Answer 2

There's nothing wrong in the sense that result will get updated each time. Performance is not a concern here. This is a very common way of writing methods / functions:

private int MethodName()
{
   int result = 0;
   if (condition1)
   {
      result = 1;
   } 
   else if (condition2)
   {
      result = 2;
   } 
   else if (condition3)
   {
      result = 3;
   }
}

Another common alternative:

private int MethodName()
{
   if (condition1)
   {
      return 1;
   } 
   else if (condition2)
   {
      return 2;
   } 
   else if (condition3)
   {
      return 3;
   }

   return 0;
}

Note that in your first example, result is a local variable, scoped to the validateApples() function, and never returned. So, it is not used outside of that function. You do all the validation, but don't do anything with the answer. In the second example, result is a private member of the class, so its value is updated as part of the validateApples() function, and can then be used by the getResult() function of the class.

However, consider the case where condition 1, condition 2, or condition 3 are true. In this case, result = "failure". Now, if condition 4a is true, result = "success", obscuring the previous failure. All of your if statements are just that - if statements, not if else. So each one will be executed, potentially overwriting the result of the previous statement. If that's what you want, that's fine, but that's usually not the case - you don't want a failure to be covered up and called a success.

As mentioned in the comments, you're using "success" and "failure" many times in the code, which would make them prime candidates for constants or even an enum. You may also wish to consider exceptions instead for the failures. Both of those would make the code much more readable and much more robust.

For a more in-depth code review, you may wish to post on codereview.stackexchange.com; they will be able to critique your actual code much better than we can here.