CODEWITHSUNDEEP
java
Will
Will

Reputation: 8631

string = string.trim(); bad practice?

Ok I am using Sonar to check code quality. It's telling me this simple method is causing two Major warnings.

public static String formatString(String string) {
    if(string==null) {
        return null;
    }
    string = string.trim();
    string = string.toUpperCase();
    return string;
}

Where I can understand the warning due to direct access to the parameter. As you can see this method pretty much does very little. Removes whitespace and puts it in caps. However what is the problem since it in the end returns a string. The creation of a value holder string seems excess to requirements given the overhead in creating a string.

So my question is what I'm doing bad coding and if so why?

Upvotes: 1

Views: 1429

Answers (6)

krystan honour
krystan honour

Reputation: 6793

Given that your example works if coded like this

public static void main(String[] args) {
    String greeting = "Hello greetings ";
    String greeting2 = formatString(greeting);

    System.out.println("Hi ["+greeting +"]");
    System.out.println("Hi ["+greeting2 +"]");
}

private static String formatString(String string) {
    if(string==null) {
        return null;
    }
    string = string.trim();
    string = string.toUpperCase();
    return string;
}

It will be complaining that you are modifying the argument directly, in java this isn't really an issue seeing as the resulting code ends up with the output

Hi [Hello greetings ]
Hi [HELLO GREETINGS]

however its a bad habit to get into given other languages do not behave this way and you could have modified the original variable so that the output would be

Hi [HELLO GREETINGS]
Hi [HELLO GREETINGS]

It is also less readable, most coding standards I've seen say do not directly modify incomming arguments unless you are changing the underlying reference entirely AND it is obvious from the signature that this will happen.

I tend to avoid like the plague this type of code (out of habit) so I would say yes its bad practise but its not technically incorrect.

for completeness I would have written formatString like this

private static String formatString(String string) {
    String output = null;

    if (string != null) {
        output = string.trim().toUpperCase();
    }

    return output;
}

I accept I could use a ternary test here (the ? operator) and do it all on one line but I tend to think thats less readable for more junior guys so again I avoid that and I think this is more readable, but that is of course.... my opinion.

Upvotes: 2

Stephen C
Stephen C

Reputation: 718936

The style checker is really complaining that you are modifying the value of a method argument. A lot of people would stay that this is bad style. It makes it harder to read the code because people don't expect argument values to be changed. (That's kind of a circular argument, but it does reflect the way that many people actually do read/misread code.)

A more stylish solution is this:

public static String formatString(String string) {
    if (string == null) {
        return null;
    }
    String tmp = string.trim();
    tmp = tmp.toUpperCase();
    return tmp;
}

or ....

public static String formatString(String string) {
    return (string == null) ? null :
           string.trim().toUpperCase();
}

Upvotes: 0

amit
amit

Reputation: 178471

The warning is because you change the input variable. In some conventions [such as spartan programming] it is considered a bad practice to change the value of the input.

Some conventions like using the variable $, if it also contains the return value:

just put String $ = string; at the first line of your code - and use $ instead of string on the rest of the method.

The real "spartan way" will be to use the trinary operator (cond ? val1 : val2) to check for null, and return string.trim().toUpperCase() alltogether.

EDIT:
Though stated some conventions supports using the variable $ when it comes to return value, as @StephenC stated, it is a bad practice, since it [the $] is reserved for generated source code,

Upvotes: 1

Sergey Kalinichenko
Sergey Kalinichenko

Reputation: 726659

I do not think that you are doing anything wrong: although the whole code can be folded to a single line like this

return string == null ? null : string.trim().toUpperCase();

I know people who prefer to see it on multiple lines. The compiler should figure out all necessary optimizations related two writing back to string "unnecessarily", so I think you're good.

Upvotes: 5

Jon Skeet
Jon Skeet

Reputation: 1500953

Well, some coders might assume that the parameter value won't be changed. I usually wouldn't change the parameter, but in a method this short I'd be content to do so. On the other hand, I'd probably write it as:

public static String formatString(String text) {
    return text == null ? null : text.trim().toUpperCase();
}

I'd also probably change the name of the method, and give a locale to toUpperCase.

Upvotes: 4

David Heffernan
David Heffernan

Reputation: 613053

Personally I see nothing really wrong with your code. I'd write it like this (after the null check):

return string.trim().toUpperCase();

This avoids modifying a parameter which often makes code easier to analyse. However, in a routine as trivial as this I don't see that as a valid concern.

Upvotes: 2

Related Questions