Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

##Summary

Summary

##Summary

Summary

added 379 characters in body
Source Link
maaartinus
  • 13.7k
  • 1
  • 35
  • 74

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

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.

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

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

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

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.

Source Link
maaartinus
  • 13.7k
  • 1
  • 35
  • 74

It 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.


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 static void printerr(String message, Rank severityLevel) {

This sounds like printer with a typo. I don't thing saving two letters is worth it. It should be called printError (with capital "E").

print, iPrint, uPrinterr, iPrinterr, ....

This sounds like logging, but I'm afraid that it's hard to find a system there. I'd prefer the standard names

trace, debug, info, warn, error


printStackTraceNoLeadingLineNumberWithLeadingMessageAndNewline

New line and what? As I read the end I've already forgot how it started. Sure, naming is hard, but what about something like printStackTraceInternal? It's private and nobody cares much what it exactly does. Describing less important details in Javadoc only is good enough.

##Summary

As you can see, there are many things I dislike, but the problem is not the code, but rather that what you're trying to achieve. Some long time ago I wrote a similar class and later found myself using only a tiny part of it. I've dropped the logging part and improved the debugging part, so I can write

 Debug.out("x =", x, ", y =", y);

and get

maaartinus.some.package.SomeClass:123
x = 12 , y = 34

I added some formatting support and removed everything what I didn't use really often. In fact, I threw away vast majority of it and reworked everything several times. My lesson learned was that unlike a building, software can and should be build top-down. I'd suggest you check your code usage and rethink what you really need.

lang-java

AltStyle によって変換されたページ (->オリジナル) /