HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Logging/debugging for testing purposes

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
loggingdebuggingtestingforpurposes

Problem

This is basically a logger which I use/wrote. It has convenient features like including one-line stack traces with print statements, making the text easier to find, and piping all output to either standard out or standard error to make it so that no print statements will be out of order.

This is part of my LogAppTester. I am looking for any reviews or suggestions.

```
package Utilities;

import java.io.*
import java.text.*
import java.util.*

/**
* Use this for various testing/debugging purposes including multi-threaded
* print statements with built in stack trace, assertions that stop the entire
* application rather than just the current thread, and application termination
* with a full stack trace for terminal error conditions. Can also check and
* respond to background events.
*
* @author johnmichaelreed2
*/
public class AppTester {

//
//
//

/**
* The system independent line separator, shortened to two letters for
* convenience.
*/
public static final String ls = System.getProperty("line.separator");
static {
AppTester.check(ls != null);
}

/**
* The number of important rows in a stack trace.
*/
private static int numberOfRowsIDisplayInStackTraces_ = 6;

public static int getNumberOfRowsIDisplayInStackTraces_() {
return numberOfRowsIDisplayInStackTraces_;
}

public static void setNumberOfRowsIDisplayInStackTraces_(int aNumberOfRowsIDisplayInStackTraces_) {
AppTester.check(aNumberOfRowsIDisplayInStackTraces_ >= 0, "You can't display a negative number of rows in a stack trace.");
numberOfRowsIDisplayInStackTraces_ = aNumberOfRowsIDisplayInStackTraces_;
}

/**
* When this variable is true, the log file will be printed to. If it is false,
* the log file will not be used even if it can be written to.
*/
private static boolean printToLogFile_ = true;

/**
* When this variable is true, the terminal will be printed to

Solution

It's very very long, so don't be surprised when I stop at some random point.

print statements with built in stack trace, assertions that stop the entire application..., Can also

Nearly every class described as "does this and that and also this" should usually be three classes.

However, I like the idea of print statements with stack trace. Actually, that's something I've been using since long and I find it much more efficient than normal debugging.

Nonetheless, logging is a different story and in case I want logging, there's logback and many other frameworks.

package Utilities;


The name is wrong, unless you own the top level domain "utilities".
Even then, capitalized package names are OK, but not exactly preferred. So maybe johnreedlol.utilities or better, use a reversed name of a domain you own (or "pretend" to own).

import java.io.*

Don't use start imports. Let your IDE take care of it.

static {
    AppTester.check(ls != null);
}


It's usually no good idea to call methods which may use not yet initialized parts of the class.

Moreover, statics and static initializer blocks should be rather sparingly as they make the code hard to test. Even for a class which gets used only statically, I'd create an instance and delegate all calls to it.

/**
 * The number of important rows in a stack trace.
 */


private static int numberOfRowsIDisplayInStackTraces_ = 6;

IIRC there's a language using trailing underscores by convention, but it's not Java. The word "important" in the comment does not correspond with what the variable controls, does it?

public static void setNumberOfRowsIDisplayInStackTraces_(int aNumberOfRowsIDisplayInStackTraces_) {
     AppTester.check(aNumberOfRowsIDisplayInStackTraces_ >= 0, "You can't display a negative number of rows in a stack trace.");
    numberOfRowsIDisplayInStackTraces_ = aNumberOfRowsIDisplayInStackTraces_;
}


Why so cruel? If someone passes an illegal argument, then an IllegalArgumentException should be thrown. No reason to call the four horsemen to terminate the world.

/**
  * All messages that are at this debug level or higher are printed. By
  * default set to {@link #NORMAL}
  */
 private static Rank myRank_ = NORMAL;


So "Level" or "Rank"? Decide which one is better and stick with it.

* {@link #EITHER_STD_OUT_OR_STD_ERROR}


There's nothing like this there.

private static ScheduledExecutorService myScheduler_
        = null; //Executors.newSingleThreadScheduledExecutor();


You're using git, so there's no need to leave dead code lying around.

/**
 * This represents the main thread. If this thread is dead,
 * {@link #myScheduler_} thread must die as well, otherwise, the application
 * won't exit at the end of the main method.
 */
private static final Thread myMainThread_;

static {
    myMainThread_ = Thread.currentThread();
}


This could be written as

private static final Thread myMainThread_ = Thread.currentThread();


It's pretty fragile. Your "main thread" is defined as the thread which caused the loading of AppTester. So when I call AppTester.whatever in my main, open a JFrame, and let main exit normally, then your "main thread" is gone, although the relevant thread (AWT) is still running.

The proper solution would be to use deamon threads in the scheduler.

public static boolean getGenerateLogFiles() {
    return printToLogFile_;
}


Decide what's the better name and stick with it.

public static void setMyDebugLevel(Rank level) {
    myRank_ = level;
}


Again, "level", "debugLevel", or "myRank"?

... skip ... skip ...

public static void killApplication(Throwable t, String messsage) {


This might be a useful method... from time to time. There's surely no need to provide 4 overloads. If someone wants to terminate the application and has nothing to say, then let them provide the empty message themselves. Frequently used operations should be as comfortable as possible, rarely used one don't need to.

public static void killApplicationNoStackTrace(String message) {


Do you need it?

System.exit(-1);


The exit code meaning is system dependent, but -1 usually mean something extraordinary terrible. I guess, +1 would be appropriate.

* Checks to see if an assertion is true. Prints stack trace and crashes the
 * program if not true. USE THIS INSTEAD OF REGULAR "assert" STATEMENTS. For


That's completely backwards. There's no use in replacing assert by your check.

  • The regular assert is for program sanity checks and can be switched off (and is off by default), which is incredibly useful for performance.



  • Methods should check their inputs and (at least for public methods) this should never be switched off. That's what Guava Preconditions are good for.



  • Sometimes the problem is unsolvable and program termination is the proper outcome. Then your check is good, but note that this is a very exceptional event.



```
private

Code Snippets

package Utilities;
static {
    AppTester.check(ls != null);
}
/**
 * The number of important rows in a stack trace.
 */
public static void setNumberOfRowsIDisplayInStackTraces_(int aNumberOfRowsIDisplayInStackTraces_) {
     AppTester.check(aNumberOfRowsIDisplayInStackTraces_ >= 0, "You can't display a negative number of rows in a stack trace.");
    numberOfRowsIDisplayInStackTraces_ = aNumberOfRowsIDisplayInStackTraces_;
}
/**
  * All messages that are at this debug level or higher are printed. By
  * default set to {@link #NORMAL}
  */
 private static Rank myRank_ = NORMAL;

Context

StackExchange Code Review Q#98092, answer score: 5

Revisions (0)

No revisions yet.