views:

1123

answers:

8

We have a huge (old legacy java) code-base, where many files (around 5k) have System.out.println's. We are planning to remove them for cleanup/performance reasons. How can we write a script that will replace them without introducing any issues in the code? The script cannot blindly delete them as following case can be an issue:

if ()
  some.code...
else
  System.out.println(...);
DB.close();

I'm thinking of replacing them with ';'. That will take care of above case. Do you see any other issues? Any other suggestions?

+1  A: 

Personally I would use {} instead, but I think it works just the same.

Martinho Fernandes
+13  A: 

Have you consider the silly case:

System.out.println(" Print " +  object.changeState() );

I don't think it happen but chances are the println executes a method that is actually performing some action on which the system depends on and may introduce subtle bugs ( believe me or not, but I have witnessed this )

Probably replacing with a logger and disabling the loggers may do.

Or creating a null object using the NullObject pattern:

public final DevNull { 
    public final static PrintStream out = new PrintStream(new OutputStream() {
        public void close() {}
        public void flush() {}
        public void write(byte[] b) {}
        public void write(byte[] b, int off, int len) {}
        public void write(int b) {}

    } );
}

And replacing

 System.out.println();

With

 DevNull.out.println();
OscarRyz
I quite like it, Oscar. Well thought, I must say.
Adeel Ansari
Stunning, I had to break out NetBeans and try it.
Paxic
Nice idea - it would be even easier to see if you changed the "silly case" from object.stateChanged() to object.changeState(), otherwise it sounds like a query to the object that I always view as having no business changing the objects state. ...personal habit...
Olaf
+2  A: 

You could use a conditional compilation to have a debug build with the print statements and a release build without them.

Basically, the idea is to create a final static class with a final static boolean that you use as a switch at compile time.

public final class Debug {
   //set to false to allow compiler to identify and eliminate
   //unreachable code
   public static final boolean ON = true;
}

Then you can just replace all of your System.out.println statements with

if(Debug.ON)
{
    System.out.println...
}

Since the compiler will ignore any unreachable branches of code, you can just set ON = false when you do a release build and the print statements will be excluded from your bytecode.

Note: This doesn't deal with the case that Oscar pointed out, where a print statement may change the state of some object. You could use conditional compilation to print to a null object when in release mode, as he suggested, instead of removing the prints altogether.

Bill the Lizard
+6  A: 

Log4E is an eclipse plugin that has a "Replace System.out.println()" feature. It will happily convert all those pesky println calls to log4j calls. It will even wrap them with a log level check.

Chris Nava
link: http://log4e.jayefem.de/
Nathan Feger
+6  A: 

You could start by calling Systems.setOut and passing in your own OutputStream that does nothing. That will help you see if there is a perfomance gain. This is safer than removing them (for the reason that Oscar pointed out - coding by side effect). If the performance gain is negligable then you might want to focus your efforts elsewhere.

Two issues with my above method:

  1. any System.out.printlns you want to keep will disapper too
  2. the String concatination will still take place (and that can be expensive depending on how much there is)

However it is a good quick test to see if you get the performance gains you are looking for.

TofuBeer
A: 

I wrote a regex in perl that replaces the string "System.out.println" with ";//System.out.println". I believe that there are very few cases where this would break the build. It would just become an "else ;", which is compiled to zero bytecode instructions.

It looks like this is what you've proposed. It worked for me -- except if you have additional statements on the same line. However, that is bad style to begin with (and I knew I didn't do that).

Overflown
+3  A: 

Extending Oscar's concept you can do even better IMHO:

if(!DEBUG) {
    System.setOut(
        new PrintStream(new OutputStream() {
            public  void    close() {}
            public  void    flush() {}
            public  void    write(byte[] b) {}
            public  void    write(byte[] b, int off, int len) {}
            public  void    write(int b) {}

        } );
    }
}

In this case, if you are not in debug mode or any other the default system out is replaced internally with devNull implementation, else it works as expected. This way you do not have to find and replace anything in your code.

Paulo Lopes
Interesting enough. The new print statements could be handled with loggers and disabling completely the System.out with this. :)
OscarRyz
So, can we call this "Oscar Reyes + Bill the Lizard" approach ? :P :P +1 for that
OscarRyz
This is assuming no other code is already doing setOut ;) Just kidding. Its a good suggestion.
Probably the easiest and fastest way to do this. Definitely the way to go if no legal console output is required.
cdecker
A: 

Have you considered editing those source files to remove the lines?

You might find that it will only a developer a couple of days to go through and get rid of most of them. We had a similar issue and I just got up really early and went through all our files getting rid of rubbish.

I used Eclipse and the clean up on save feature to cleanup imports and stuff at the same time.

Its quite a therapeutic thing to do!

Fortyrunner