From ddbf9acd073c927542d78df339ae2cac1371244f Mon Sep 17 00:00:00 2001 From: delvh Date: Sun, 23 Aug 2020 13:39:31 +0200 Subject: [PATCH] Apply suggestions from code review Co-authored-by: CyB3RC0nN0R --- .../java/envoy/client/data/ClientConfig.java | 11 +-- .../main/java/envoy/client/net/Client.java | 4 +- .../main/java/envoy/client/net/Receiver.java | 2 +- .../envoy/client/ui/controller/ChatScene.java | 1 - common/src/main/java/envoy/data/Config.java | 98 +++++++++---------- .../src/main/java/envoy/data/ConfigItem.java | 36 ++----- .../main/java/envoy/event/NoAttachments.java | 10 +- .../java/envoy/server/data/ServerConfig.java | 16 +-- server/src/main/resources/server.properties | 2 +- 9 files changed, 73 insertions(+), 107 deletions(-) diff --git a/client/src/main/java/envoy/client/data/ClientConfig.java b/client/src/main/java/envoy/client/data/ClientConfig.java index fa5181d..60196de 100644 --- a/client/src/main/java/envoy/client/data/ClientConfig.java +++ b/client/src/main/java/envoy/client/data/ClientConfig.java @@ -32,9 +32,9 @@ public final class ClientConfig extends Config { private ClientConfig() { super(".envoy"); - put("server", "s", identity(), true); - put("port", "p", Integer::parseInt, true); - put("localDB", "db", File::new, true); + put("server", "s", identity()); + put("port", "p", Integer::parseInt); + put("localDB", "db", File::new); put("ignoreLocalDB", "nodb", Boolean::parseBoolean); put("user", "u", identity()); put("password", "pw", identity()); @@ -62,10 +62,7 @@ public final class ClientConfig extends Config { * @return {@code true} if the local database is to be ignored * @since Envoy Client v0.3-alpha */ - public Boolean isIgnoreLocalDB() { - final var ignoreLocalDB = items.get("ignoreLocalDB").get(); - return ignoreLocalDB != null && (Boolean) ignoreLocalDB; - } + public Boolean isIgnoreLocalDB() { return (Boolean) items.get("ignoreLocalDB").get(); } /** * @return the user name diff --git a/client/src/main/java/envoy/client/net/Client.java b/client/src/main/java/envoy/client/net/Client.java index 923f721..2cecde8 100644 --- a/client/src/main/java/envoy/client/net/Client.java +++ b/client/src/main/java/envoy/client/net/Client.java @@ -164,11 +164,11 @@ public final class Client implements Closeable { // Process ProfilePicChanges receiver.registerProcessor(ProfilePicChange.class, eventBus::dispatch); - // Process requests to not send anymore attachments as they will not be shown to + // Process requests to not send any more attachments as they will not be shown to // other users receiver.registerProcessor(NoAttachments.class, eventBus::dispatch); - // Process group creation rejections - they have been disabled on the server + // Process group creation results - they might have been disabled on the server receiver.registerProcessor(GroupCreationResult.class, eventBus::dispatch); // Send event diff --git a/client/src/main/java/envoy/client/net/Receiver.java b/client/src/main/java/envoy/client/net/Receiver.java index ce510cf..e9ac48f 100644 --- a/client/src/main/java/envoy/client/net/Receiver.java +++ b/client/src/main/java/envoy/client/net/Receiver.java @@ -67,7 +67,7 @@ public final class Receiver extends Thread { // Catch LV encoding errors if (len != bytesRead) { // Server has stopped sending, i.e. because he went offline - if (len == 0 && bytesRead == -1) { + if (bytesRead == -1) { isAlive = false; logger.log(Level.INFO, "Lost connection to the server. Exiting receiver..."); continue; diff --git a/client/src/main/java/envoy/client/ui/controller/ChatScene.java b/client/src/main/java/envoy/client/ui/controller/ChatScene.java index fa788e4..83bb085 100644 --- a/client/src/main/java/envoy/client/ui/controller/ChatScene.java +++ b/client/src/main/java/envoy/client/ui/controller/ChatScene.java @@ -132,7 +132,6 @@ public final class ChatScene implements Restorable { private AudioRecorder recorder; private boolean recording; private Attachment pendingAttachment; - private boolean postingPermanentlyDisabled; private final SystemCommandsMap messageTextAreaCommands = new SystemCommandsMap(); diff --git a/common/src/main/java/envoy/data/Config.java b/common/src/main/java/envoy/data/Config.java index b51acea..7b7c097 100644 --- a/common/src/main/java/envoy/data/Config.java +++ b/common/src/main/java/envoy/data/Config.java @@ -11,10 +11,15 @@ import envoy.util.EnvoyLog; /** * Manages all application settings that are set during application startup by - * either loading them from the {@link Properties} file + * either loading them from the {@link Properties} file (default values) * {@code client.properties} or parsing them from the command line arguments of - * the application.
- *
+ * the application. + *

+ * All items inside the {@code Config} are supposed to either be supplied over + * default value or over command line argument. Developers that fail to provide + * default values will be greeted with an error message the next time they try + * to start Envoy... + *

* Project: envoy-client
* File: Config.java
* Created: 12 Oct 2019
@@ -30,10 +35,10 @@ public class Config { protected Config(String folderName) { final var rootDirectory = new File(System.getProperty("user.home"), folderName); - put("homeDirectory", "home", File::new, true); + put("homeDirectory", "home", File::new); ((ConfigItem) get("homeDirectory")).setValue(rootDirectory); - put("fileLevelBarrier", "fb", Level::parse, true); - put("consoleLevelBarrier", "cb", Level::parse, true); + put("fileLevelBarrier", "fb", Level::parse); + put("consoleLevelBarrier", "cb", Level::parse); } /** @@ -43,10 +48,8 @@ public class Config { * @since Envoy Common v0.1-beta */ private void load(Properties properties) { - items.entrySet() - .stream() - .filter(e -> properties.containsKey(e.getKey())) - .forEach(e -> e.getValue().parse(properties.getProperty(e.getKey()))); + items.entrySet().stream().filter(e -> properties.containsKey(e.getKey())) + .forEach(e -> e.getValue().parse(properties.getProperty(e.getKey()))); } /** @@ -61,25 +64,31 @@ public class Config { for (int i = 0; i < args.length; i++) for (final ConfigItem item : items.values()) if (args[i].startsWith("--")) { - if (args[i].length() == 2) throw new IllegalStateException("Malformed command line argument at position " + i + ": " + args[i]); + if (args[i].length() == 2) + throw new IllegalStateException( + "Malformed command line argument at position " + i + ": " + args[i]); final String commandLong = args[i].substring(2); if (item.getCommandLong().equals(commandLong)) { item.parse(args[++i]); break; } } else if (args[i].startsWith("-")) { - if (args[i].length() == 1) throw new IllegalStateException("Malformed command line argument at position " + i + ": " + args[i]); + if (args[i].length() == 1) + throw new IllegalStateException( + "Malformed command line argument at position " + i + ": " + args[i]); final String commandShort = args[i].substring(1); if (item.getCommandShort().equals(commandShort)) { item.parse(args[++i]); break; } - } else throw new IllegalStateException("Malformed command line argument at position " + i + ": " + args[i]); + } else + throw new IllegalStateException( + "Malformed command line argument at position " + i + ": " + args[i]); } /** - * Supplies default values from the given .properties file and - * parses the configuration from an array of command line arguments. + * Supplies default values from the given .properties file and parses the + * configuration from an array of command line arguments. * * @param declaringClass the class calling this method * @param propertiesFilePath the path to where the .properties file can be found @@ -93,37 +102,38 @@ public class Config { * @since Envoy Common v0.1-beta */ public void loadAll(Class declaringClass, String propertiesFilePath, String[] args) { - if (modificationDisabled) throw new IllegalStateException("Cannot change config after isInitialized has been called"); + if (modificationDisabled) + throw new IllegalStateException("Cannot change config after isInitialized has been called"); // Load the defaults from the given .properties file first final var properties = new Properties(); try { properties.load(declaringClass.getClassLoader().getResourceAsStream(propertiesFilePath)); } catch (final IOException e) { - EnvoyLog.getLogger(Config.class).log(Level.SEVERE, "An error occurred when reading in the configuration: ", e); + EnvoyLog.getLogger(Config.class).log(Level.SEVERE, "An error occurred when reading in the configuration: ", + e); } load(properties); // Override configuration values with command line arguments - if (args.length > 0) load(args); + if (args.length > 0) + load(args); - // Check if all mandatory configuration values have been initialized + // Check if all configuration values have been initialized isInitialized(); // Disable further editing of the config modificationDisabled = true; } /** - * @throws IllegalStateException if a mandatory {@link ConfigItem} has not been + * @throws IllegalStateException if a {@link ConfigItem} has not been * initialized * @since Envoy Common v0.1-beta */ private void isInitialized() { - if (items.values().stream().filter(ConfigItem::isMandatory).map(ConfigItem::get).anyMatch(Objects::isNull)) - throw new IllegalStateException("config item(s) has/ have not been initialized:" + items.values() - .stream() - .filter(configItem -> configItem.isMandatory() && configItem.get() == null) - .map(ConfigItem::getCommandLong) - .collect(Collectors.toSet())); + if (items.values().stream().map(ConfigItem::get).anyMatch(Objects::isNull)) + throw new IllegalStateException("config item(s) has/ have not been initialized:" + + items.values().stream().filter(configItem -> configItem.get() == null) + .map(ConfigItem::getCommandLong).collect(Collectors.toSet())); } /** @@ -131,28 +141,13 @@ public class Config { * @return the config item with the specified name * @since Envoy Common v0.1-beta */ - public ConfigItem get(String name) { return items.get(name); } - - /** - * Shorthand for
- * {@code items.put(commandName, new ConfigItem<>(commandName, commandShort, parseFunction, defaultValue, mandatory))}. - * - * @param the type of the {@link ConfigItem} - * @param commandName the key for this config item as well as its long name - * @param commandShort the abbreviation of this config item - * @param parseFunction the {@code Function} that parses the value - * from a string - * @param mandatory indicated that this config item must be initialized with - * a non-null value - * @since Envoy Common v0.2-beta - */ - protected void put(String commandName, String commandShort, Function parseFunction, boolean mandatory) { - items.put(commandName, new ConfigItem<>(commandName, commandShort, parseFunction, mandatory)); + public ConfigItem get(String name) { + return items.get(name); } /** * Shorthand for
- * {@code put(commandName, commandShort, parseFunction, false)}. + * {@code items.put(commandName, new ConfigItem<>(commandName, commandShort, parseFunction, defaultValue))}. * * @param the type of the {@link ConfigItem} * @param commandName the key for this config item as well as its long name @@ -162,25 +157,30 @@ public class Config { * @since Envoy Common v0.2-beta */ protected void put(String commandName, String commandShort, Function parseFunction) { - put(commandName, commandShort, parseFunction, false); + items.put(commandName, new ConfigItem<>(commandName, commandShort, parseFunction)); } /** * @return the directory in which all local files are saves * @since Envoy Client v0.2-beta */ - public File getHomeDirectory() { return (File) items.get("homeDirectory").get(); } + public File getHomeDirectory() { + return (File) items.get("homeDirectory").get(); + } /** * @return the minimal {@link Level} to log inside the log file * @since Envoy Client v0.2-beta */ - public Level getFileLevelBarrier() { return (Level) items.get("fileLevelBarrier").get(); } + public Level getFileLevelBarrier() { + return (Level) items.get("fileLevelBarrier").get(); + } /** * @return the minimal {@link Level} to log inside the console * @since Envoy Client v0.2-beta */ - public Level getConsoleLevelBarrier() { return (Level) items.get("consoleLevelBarrier").get(); } - + public Level getConsoleLevelBarrier() { + return (Level) items.get("consoleLevelBarrier").get(); + } } diff --git a/common/src/main/java/envoy/data/ConfigItem.java b/common/src/main/java/envoy/data/ConfigItem.java index 91113ca..5f61750 100644 --- a/common/src/main/java/envoy/data/ConfigItem.java +++ b/common/src/main/java/envoy/data/ConfigItem.java @@ -4,8 +4,10 @@ import java.util.function.Function; /** * Contains a single {@link Config} value as well as the corresponding command - * line arguments and its default value.
- *
+ * line arguments and its default value. + *

+ * All {@code ConfigItem}s are automatically mandatory. + *

* Project: envoy-clientChess
* File: ConfigItem.javaEvent.java
* Created: 21.12.2019
@@ -18,7 +20,6 @@ public final class ConfigItem { private final String commandLong, commandShort; private final Function parseFunction; - private final boolean mandatory; private T value; @@ -29,28 +30,12 @@ public final class ConfigItem { * @param commandShort the short command line argument to set this value * @param parseFunction the {@code Function} that parses the value * from a string - * @param mandatory indicated that this config item must be initialized with - * a non-null value - * @since Envoy Common v0.1-beta - */ - public ConfigItem(String commandLong, String commandShort, Function parseFunction, boolean mandatory) { - this.commandLong = commandLong; - this.commandShort = commandShort; - this.parseFunction = parseFunction; - this.mandatory = mandatory; - } - - /** - * Initializes an optional {@link ConfigItem} without a default value. - * - * @param commandLong the long command line argument to set this value - * @param commandShort the short command line argument to set this value - * @param parseFunction the {@code Function} that parses the value - * from a string * @since Envoy Common v0.1-beta */ public ConfigItem(String commandLong, String commandShort, Function parseFunction) { - this(commandLong, commandShort, parseFunction, false); + this.commandLong = commandLong; + this.commandShort = commandShort; + this.parseFunction = parseFunction; } /** @@ -86,11 +71,4 @@ public final class ConfigItem { * @since Envoy Common v0.2-beta */ protected void setValue(T value) { this.value = value; } - - /** - * @return {@code true} if this {@link ConfigItem} is mandatory for successful - * application initialization - * @since Envoy Common v0.1-beta - */ - public boolean isMandatory() { return mandatory; } } diff --git a/common/src/main/java/envoy/event/NoAttachments.java b/common/src/main/java/envoy/event/NoAttachments.java index 126b93f..aace460 100644 --- a/common/src/main/java/envoy/event/NoAttachments.java +++ b/common/src/main/java/envoy/event/NoAttachments.java @@ -11,15 +11,7 @@ package envoy.event; * @author Leon Hofmeister * @since Envoy Common v0.2-beta */ -public class NoAttachments extends Event { +public class NoAttachments extends Event.Valueless { private static final long serialVersionUID = 1L; - - /** - * Creates a new {@code NoAttachments}. - * - * @since Envoy Common v0.2-beta - */ - public NoAttachments() { super(null); } - } diff --git a/server/src/main/java/envoy/server/data/ServerConfig.java b/server/src/main/java/envoy/server/data/ServerConfig.java index 89e7c35..150e626 100644 --- a/server/src/main/java/envoy/server/data/ServerConfig.java +++ b/server/src/main/java/envoy/server/data/ServerConfig.java @@ -24,17 +24,17 @@ public final class ServerConfig extends Config { private ServerConfig() { super(".envoy-server"); - put("enter-to-stop", "dev-stop", Boolean::parseBoolean, true); + put("enter-to-stop", "dev-stop", Boolean::parseBoolean); // parameters for issue reporting - put("issueCreationURL", "i-url", identity(), true); + put("issueCreationURL", "i-url", identity()); put("issueAuthToken", "i-token", identity()); - put("userMadeLabel", "l-um", identity(), true); - put("bugLabel", "l-b", identity(), true); - put("featureLabel", "l-f", identity(), true); + put("userMadeLabel", "l-um", identity()); + put("bugLabel", "l-b", identity()); + put("featureLabel", "l-f", identity()); // enabling/ disabling several processors - put("enableIssueReporting", "e-ir", Boolean::parseBoolean, true); - put("enableGroups", "e-g", Boolean::parseBoolean, true); - put("enableAttachments", "e-a", Boolean::parseBoolean, true); + put("enableIssueReporting", "e-ir", Boolean::parseBoolean); + put("enableGroups", "e-g", Boolean::parseBoolean); + put("enableAttachments", "e-a", Boolean::parseBoolean); } /** diff --git a/server/src/main/resources/server.properties b/server/src/main/resources/server.properties index 96f128f..2d50513 100644 --- a/server/src/main/resources/server.properties +++ b/server/src/main/resources/server.properties @@ -13,4 +13,4 @@ featureLabel=119 #bugLabel="bug" #featureLabel="feature" consoleLevelBarrier=FINEST -fileLevelBarrier=WARNING \ No newline at end of file +fileLevelBarrier=WARNING