Code Review

- Renamed CrashReportGenerator.makeCrashReport to .crash for convenience
- Split .crash into subroutines
- Documented Analyzer and ContextProvider
  - Both are now supposed to have names In Title Case
- CRG.export now assumes PrintStream does not fail and assumes Log can
fail

src/main/java/ru/windcorp/progressia/client/ProgressiaClientMain.java

@@ -38,7 +38,7 @@ public class ProgressiaClientMain {
 			long[] ssdss = new long[1 << 30];
 		} catch (Throwable t)
 		{
-			CrashReportGenerator.makeCrashReport(t, "u %s stupid", "vry");
+			CrashReportGenerator.crash(t, "u %s stupid", "vry");
 		}
 
 		ProgressiaLauncher.launch(args, new ClientProxy());

src/main/java/ru/windcorp/progressia/common/util/crash/Analyzer.java

@@ -1,7 +1,32 @@
 package ru.windcorp.progressia.common.util.crash;
 
+/**
+ * A crash report utility that performs analysis of a problem during crash
+ * report generation and presents its conclusion to the user via the crash report.
+ * Unlike {@link ContextProvider}s, Analyzers are provided with the reported problem
+ * details.
+ * @see ContextProvider
+ * @author serega404
+ */
 public interface Analyzer {
+	
+	/**
+	 * Provides a human-readable string describing this analyzer's conclusion
+	 * on the presented problem, or returns {@code null} if no conclusion
+	 * could be made.
+	 * @param throwable The reported throwable (may be {@code null})
+	 * @param messageFormat A {@linkplain java.util.Formatter#syntax format string} of a
+	 * human-readable description of the problem 
+	 * @param args The arguments for the format string
+	 * @return a conclusion or {@code null}
+	 */
 	String analyze(Throwable throwable, String messageFormat, Object... args);
 
+	/**
+	 * Returns this analyzer's human-readable name.
+	 * It should be A String In Title Case With Spaces.
+	 * @return this analyzer's name
+	 */
 	String getName();
+	
 }

src/main/java/ru/windcorp/progressia/common/util/crash/ContextProvider.java

@@ -2,8 +2,30 @@ package ru.windcorp.progressia.common.util.crash;
 
 import java.util.Map;
 
+/**
+ * A crash report utility that gathers information about game and system state
+ * when a crash occurs and presents it to the user via the crash report.
+ * ContextProviders are not aware of the nature of the problem, unlike {@link Analyzer}s.
+ * @see Analyzer
+ * @author serega404
+ */
 public interface ContextProvider {
+	
+	/**
+	 * Provides human-readable description of the state of the game and the system.
+	 * This information is {@link Map#put(Object, Object) put} into the provided map
+	 * as key-value pairs. Keys are the characteristic being described, such as "OS Name",
+	 * and should be Strings In Title Case With Spaces.
+	 * If this provider cannot provide any information at this moment, the map is not
+	 * modified.
+	 * @param output the map to append output to
+	 */
 	void provideContext(Map<String, String> output);
 	
+	/**
+	 * Returns this provider's human-readable name.
+	 * It should be A String In Title Case With Spaces.
+	 * @return this provider's name
+	 */
 	String getName();
 }

src/main/java/ru/windcorp/progressia/common/util/crash/CrashReportGenerator.java

@@ -2,11 +2,12 @@ package ru.windcorp.progressia.common.util.crash;
 
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+import org.apache.logging.log4j.core.util.StringBuilderWriter;
 
 import java.io.BufferedWriter;
 import java.io.IOException;
 import java.io.PrintWriter;
-import java.io.StringWriter;
+import java.io.Writer;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -17,21 +18,43 @@ import java.util.*;
 
 public class CrashReportGenerator {
 
-	private CrashReportGenerator() {
-	}
+	private CrashReportGenerator() {}
 
 	private static final Path CRASH_REPORTS_PATH = Paths.get("crash-reports");
 
-	private static final Collection<ContextProvider> PROVIDERS = new ArrayList<>();
+	private static final Collection<ContextProvider> PROVIDERS =
+			Collections.synchronizedCollection(new ArrayList<>());
 
-	private static final Collection<Analyzer> ANALYZERS = new ArrayList<>();
+	private static final Collection<Analyzer> ANALYZERS =
+			Collections.synchronizedCollection(new ArrayList<>());
 
 	private static final Logger LOGGER = LogManager.getLogger("crash");
 
-	public static void makeCrashReport(Throwable throwable, String messageFormat, Object... args) {
-
+	/**
+	 * <em>This method never returns.</em>
+	 * <p>
+	 * TODO document
+	 * @param throwable
+	 * @param messageFormat
+	 * @param args
+	 */
+	public static void crash(Throwable throwable, String messageFormat, Object... args) {
 		StringBuilder output = new StringBuilder();
 
+		appendContextProviders(output);
+		addSeparator(output);
+		if (appendAnalyzers(output, throwable, messageFormat, args)) {
+			addSeparator(output);
+		}
+		
+		appendStackTrace(output, throwable);
+		
+		export(output.toString());
+
+		System.exit(0);
+	}
+
+	private static void appendContextProviders(StringBuilder output) {
 		for (ContextProvider provider : PROVIDERS) {
 			if (provider != null) {
 				Map<String, String> buf = new HashMap<>();
@@ -55,10 +78,13 @@ public class CrashReportGenerator {
 				}
 			}
 		}
+	}
 	
-		addSeparator(output);
-
-		boolean analyzerResponseExist = false;
+	private static boolean appendAnalyzers(
+			StringBuilder output,
+			Throwable throwable, String messageFormat, Object[] args
+	) {
+		boolean analyzerResponsesExist = false;
 		for (Analyzer analyzer : ANALYZERS) {
 			if (analyzer != null) {
 
@@ -67,12 +93,12 @@ public class CrashReportGenerator {
 					answer = analyzer.analyze(throwable, messageFormat, args);
 
 					if (answer != null && !answer.isEmpty()) {
-						analyzerResponseExist = true;
+						analyzerResponsesExist = true;
 						output.append(analyzer.getName()).append(": ").append(answer).append("\n");
 					}
 				} catch (Throwable t) {
 					try {
-						analyzerResponseExist = true;
+						analyzerResponsesExist = true;
 						output.append(analyzer.getName()).append(" is broken").append("\n");
 					} catch (Throwable th) {
 						// You stupid
@@ -82,37 +108,39 @@ public class CrashReportGenerator {
 			}
 		}
 		
-		if (analyzerResponseExist) addSeparator(output);
+		return analyzerResponsesExist;
+	}
+
+	private static void appendStackTrace(StringBuilder output, Throwable throwable) {
+		output.append("Stacktrace: \n");
+		
+		if (throwable == null) {
+			output.append("no Throwable provided").append("\n");
+		}
 		
 		// Formatting to a human-readable string
-		StringWriter sink = new StringWriter();
-		if (throwable != null) {
+		Writer sink = new StringBuilderWriter(output);
 		try {
 			throwable.printStackTrace(new PrintWriter(sink));
 		} catch (Exception e) {
 			// PLAK
 		}
-		} else {
-			sink.append("Null");
+		output.append("\n");
 	}
 	
-		output.append("Stacktrace: \n");
-		output.append(sink.toString()).append("\n");
-
-		LOGGER.fatal("/n" + output.toString());
-
+	private static void export(String report) {
 		try {
-			System.err.println(output.toString());
+			LOGGER.fatal("/n" + report);
 		} catch (Exception e) {
 			// PLAK
 		}
 		
-		generateCrashReportFiles(output.toString());
+		System.err.println(report);
 		
-		System.exit(0);
+		generateCrashReportFiles(report);
 	}
 
-	public static void generateCrashReportFiles(String output) {
+	private static void generateCrashReportFiles(String output) {
 		Date date = new Date();
 		DateFormat dateFormat = new SimpleDateFormat("yyyy-MM-dd-HH.mm.ss");
 
@@ -121,7 +149,6 @@ public class CrashReportGenerator {
 
 			try {
 				Files.createDirectory(CRASH_REPORTS_PATH);
-				;
 				pathExist = true;
 			} catch (IOException e) {
 				// Crash Report not created
@@ -135,8 +162,13 @@ public class CrashReportGenerator {
 		}
 	}
 
-	public static void createFileForCrashReport(String buffer, String filename) {
-		try (BufferedWriter writer = Files.newBufferedWriter(Paths.get(filename), StandardCharsets.UTF_8)) {
+	private static void createFileForCrashReport(String buffer, String filename) {
+		try (
+				BufferedWriter writer = Files.newBufferedWriter(
+						Paths.get(filename),
+						StandardCharsets.UTF_8
+				)
+		) {
 			writer.write(buffer);
 		} catch (IOException ex) {
 			// Crash Report not created
@@ -152,6 +184,9 @@ public class CrashReportGenerator {
 	}
 
 	private static void addSeparator(StringBuilder sb) {
-		sb.append("-------------------------------------------------").append("\n");
+		sb.append(
+				// 80 chars
+				"--------------------------------------------------------------------------------"
+		).append("\n");
 	}
 }

src/main/java/ru/windcorp/progressia/common/util/crash/analyzers/OutOfMemoryAnalyzer.java

@@ -6,12 +6,12 @@ public class OutOfMemoryAnalyzer implements Analyzer {
 	@Override
 	public String analyze(Throwable throwable, String messageFormat, Object... args) {
 		if (throwable instanceof OutOfMemoryError)
-			return "Try add memory for the JVM";
+			return "Try to add memory to the JVM";
 		return null;
 	}
 
 	@Override
 	public String getName() {
-		return this.getClass().getSimpleName();
+		return "Out Of Memory Analyzer";
 	}
 }

src/main/java/ru/windcorp/progressia/common/util/crash/providers/OSContextProvider.java

@@ -8,13 +8,13 @@ public class OSContextProvider implements ContextProvider {
 
 	@Override
 	public void provideContext(Map<String, String> output) {
-		output.put("Name OS", System.getProperty("os.name"));
-		output.put("Version OS", System.getProperty("os.version"));
-		output.put("Architecture OS", System.getProperty("os.arch"));
+		output.put("OS Name", System.getProperty("os.name"));
+		output.put("OS Version", System.getProperty("os.version"));
+		output.put("OS Architecture", System.getProperty("os.arch"));
 	}
 
 	@Override
 	public String getName() {
-		return this.getClass().getSimpleName();
+		return "OS Context Provider";
 	}
 }