[slf4j-dev] Comments on slf4j-converter

[Ceki Gulcu]

Hi Jean-Noel,

I just had a look a your recent additions to slf4j-converter. It looks like we 
are on the verge of having a functional application, which is great news.

Here are a number of comments listed in random order.

- Logging not necessary

Paradoxically, slf4j-converter should probably not use a logging library, not 
even SLF4J. I can think of two reasons. First, slf4j-converter is not a long 
living application. It runs for a few minutes and then stops. We don't need a 
sophisticated logging machinery to send the output to a database, to a remote 
server or a file. Printing a few messages on the console using 
System.out.println should be sufficient. Second, by removing logging output, 
slf4j-converter will be easier to package and more importantly easier to launch 
by the end user. Remember, all the user wants is to convert his or her projects 
to SLF4J API with the least amount of fuss possible.


- AbstractMatcher does more than matching

AbstractMatcher is not only responsible for matching lines of text but also 
outputting it to a stream using its writer object. I think it the code would be 
easier to understand and to test if the matcher did only text substitution, and 
some other component was responsible for reading and writing files.

The matcher would be much easier to test. For example you could write:

public class MatcherTest extends TestCase {


   public void testBasic() {
     JCLMatcher m = new JCLMatcher();
     {
       String r = m.replace("import org.apache.commons.logging.Log;");
       assertEquals("import org.slf4j.Logger", r);
     }

     {
       String r = m.replace("import org.apache.commons.logging.LogFactory;");
       assertEquals("import org.slf4j.LoggerFactory", r);
     }

     {
       String r = m.replace("Log l = LogFactory.getLog(MyClass.class);");
       assertEquals("Logger l = LoggerFactory.getLogger(MyClass.class);", r);
     }

     ...
   }

}

- Hard coded paths in Converter

The getPath() method in the Converter class uses hard coded paths.

- The convert method in the Converter takes a list of files. It has the 
following signature.

   private void convert(List<File> lstFiles) {

How about a convert method taking a single file and another method looping on a 
list of files?

That's all I have time for the moment.

Cheers,

-- 
Ceki Gülcü
Logback: The reliable, generic, fast and flexible logging framework for Java.
http://logback.qos.ch