0

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";
    }

}

}
  • The jdk optimizes the creation of strings, so don't concern yourself too much with the creation of new strings. The ugly thing here is that you're using "failure"/"success" in many different places. If you find yourself doing this, you should probably be asking yourself if you shouldn't be creating a constant (static final field). – Neil Mar 1 '18 at 15:32
  • 3
    Or learning what exceptions are. – candied_orange Mar 1 '18 at 15:35
  • 1
    Setting result = "foobar"; does not create a new String object every time it gets executed. There will be only one "foobar" string, which is created when the class is loaded. If, for some reason you actually wanted a new string every time, then you would write result=new String("foobar"); – Solomon Slow Mar 1 '18 at 16:03
  • Slightly off-topic, but if you used enums for result values you wouldn't even have this problem. – Mael Mar 1 '18 at 16:16
  • Your resultis a reference variable, which is effectively a pointer to a string. It is not a string object. "hello" is a string object. In your sample code, no string objects are being changed or copied, only the reference variable is being updated to point to different string objects. – Erik Eidt Mar 1 '18 at 17:17
2

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.

  • 1
    Re, "...replace the string values...with constants." A string literal is a constant. "success" is a constant. What you are talking about is replacing them with named constants. (e.g., final string SUCCESS="success";) – Solomon Slow Mar 1 '18 at 16:07
  • 1
    Re, "If you only ever need two cases then a boolean would do just dandy." Except that, the names "true" and "false" don't offer much help to somebody who's trying to read and understand the code. Does true mean that the function did what it was supposed to do? or does true mean that an error happened? I've seen plenty of examples of both styles. – Solomon Slow Mar 1 '18 at 16:11
  • @jameslarge: Perhaps I was not clear in my explanation, I was referring here to a variable declared as final and have the code return this constant. Strings in java are immutable, true, but that does not make a variable constant because it contains a string. For the true/false, you are right, it's not really much better for readability. It does prevent typos and does enroll the help of the compiler to this purpose. That said, a function declared as isValid() and having it return a true or false would not need a ton of documentation to figure out which value means what now would it ? – Newtopian Mar 1 '18 at 16:49
0

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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