Detect changing value of object passed as parameter

Question

I'm now working with code that looks like this

public String getName(User user) {
     user.setSth(...);
     return user.getName();
}

I think it's bad practice to change objects passed as parameters. Is there a tool that detects that kind of code? I looked at findbugs, pmd and checkstyle, but could not find any check for this.

P.S. sorry for bad example.

Answer 1

You won't find anything because, from a tool's point of view, "getName" and "setSth" are just method calls. Humans say "this is a getter" and "this is a setter" but tools don't. In fact, getName() is not a getter because getters don't accept arguments.

So the tool can't see anything unusual because methods change objects all the time.

If you want to enforce this rule, have a look at extending findbugs and PMD. Both allow you to define additional constraints. What you're looking for is probably:

• If method name starts with "get"
• AND method body calls method of any object passes as parameter

then print a warning. That shouldn't take too long. Run this and you will see how many "false positives" you get (warnings about methods which are actually OK). This will help you determine whether it's worth to pursue this further. Plus you'll have a new item to add to your CV :)

Answer 2

There are tools that can "reason" about code on a higher level than compilers typically do. Declarative Metaprogramming for example is a discipline that allows writing programs to check if another program conforms to a certain design, or, conversely, to mine for code smells and anti-patterns.

Some links:

http://prog.vub.ac.be/DMP/

http://www.cs.bris.ac.uk/Publications/pub_master.jsp?id=1000273

and for the rest

http://www.google.com/search?num=100&hl=en&q=Declarative+Metaprogramming

Answer 3

You can create an interface called UserView containing only "getters", make User implement it and use the new UserView interface as the type of parameter.

interface UserView{
 public String getName();
...

class User implements UserView...

public String getName(UserView user) {
     user.setSth(...); // Will not compile
     return user.getName();
}

Answer 4

You're looking for something like "const" in C++ that will enforce making the parameter value as immutable as the reference that's passed in. Immutable objects guarantee that, if you can live with them.

You're arguing that this is "bad" because side effects like this can surprise a user. That's valid, but it's only harmful if it's an unwanted surprise.

Answer 5

Actually this is something that in C++ was very easy to do via the const qualifier. You would define a parameter as const and for that parameter you could only call methods defined as const - usually, getters.

In Java this is absent and frankly, I don't really mind. As mentioned there are source code analyzers which can check this behavior, as well as meta-programming methods to do this as well.

Personally, I believe that if the method is named properly, there is no problem of passing an object to it so that is it modified.

Answer 6

You could make User immutable (declare it final, declare all properties final and remote the setters. I know that isn't practible everywhere but in many places that is good and you will have no problems in passing that to other functions).

If you have to "change" something, you can implement functions like newId in that sample:

public final class User {
    private final String name;
    private final int id;

    User(String name, int id) {
        this.name = name;
        this.id = id;
    }

    public User newId(int newId) {
        return new User(this.name, newId);
    }

    //getters here;
}

The built in String, Integer, ... classes do that, too.

Answer 7

I think you are already on the right track: your best tool to detect this sort of code is almost certainly Findbugs. However, you will probably need to write your own detector for this pattern. Here is an example of how you would write a detector, though it isn't exactly the detector that you're looking for.

Caveat: I don't really agree that a side-effecting getter is always bad style. However, if you really want to find that sort of thing, I would recommend Findbugs.