From 5dc3f4beb708309a281c70b8216a7a8ae11954fe Mon Sep 17 00:00:00 2001 From: Yufeng Wang Date: Thu, 5 Oct 2023 06:21:10 -0700 Subject: [PATCH] [java-matter-controller]Improve the readability and maintainability from code review (#29571) --- .../controller/commands/common/Command.kt | 64 +++++++++---------- .../commands/common/CommandManager.kt | 22 +++---- .../commands/common/CredentialsIssuer.kt | 5 +- .../commands/common/FutureResult.kt | 13 ++-- .../commands/common/MatterCommand.kt | 5 -- .../controller/commands/common/RealResult.kt | 10 +-- 6 files changed, 49 insertions(+), 70 deletions(-) diff --git a/examples/java-matter-controller/java/src/com/matter/controller/commands/common/Command.kt b/examples/java-matter-controller/java/src/com/matter/controller/commands/common/Command.kt index fde197900dcc9e..1b05e0205f9a0b 100644 --- a/examples/java-matter-controller/java/src/com/matter/controller/commands/common/Command.kt +++ b/examples/java-matter-controller/java/src/com/matter/controller/commands/common/Command.kt @@ -28,17 +28,9 @@ import java.util.concurrent.atomic.AtomicLong * that may be performed. Commands are verb-like, such as pair a Matter device or discover Matter * devices from the environment. */ -abstract class Command(private val name: String, private val helpText: String? = null) { +abstract class Command(val name: String, val helpText: String? = null) { private val arguments = ArrayList() - fun getName(): String { - return name - } - - fun getHelpText(): String? { - return helpText - } - fun getArgumentName(index: Int): String { return arguments[index].name } @@ -92,8 +84,8 @@ abstract class Command(private val name: String, private val helpText: String? = * @return The number of arguments currently added to the command * @brief Add a bool command argument */ - fun addArgument(name: String?, out: AtomicBoolean?, desc: String?, optional: Boolean) { - val arg = Argument(name!!, out!!, desc, optional) + fun addArgument(name: String, out: AtomicBoolean, desc: String?, optional: Boolean) { + val arg = Argument(name, out, desc, optional) addArgumentToList(arg) } @@ -108,14 +100,14 @@ abstract class Command(private val name: String, private val helpText: String? = * @brief Add a short command argument */ fun addArgument( - name: String?, + name: String, min: Short, max: Short, - out: AtomicInteger?, + out: AtomicInteger, desc: String?, optional: Boolean ) { - val arg = Argument(name!!, min, max, out!!, desc, optional) + val arg = Argument(name, min, max, out, desc, optional) addArgumentToList(arg) } @@ -130,14 +122,14 @@ abstract class Command(private val name: String, private val helpText: String? = * @brief Add an int command argument */ fun addArgument( - name: String?, + name: String, min: Int, max: Int, - out: AtomicInteger?, + out: AtomicInteger, desc: String?, optional: Boolean ) { - val arg = Argument(name!!, min, max, out!!, desc, optional) + val arg = Argument(name, min, max, out, desc, optional) addArgumentToList(arg) } @@ -152,14 +144,14 @@ abstract class Command(private val name: String, private val helpText: String? = * @brief Add a long Integer command argument */ fun addArgument( - name: String?, + name: String, min: Short, max: Short, - out: AtomicLong?, + out: AtomicLong, desc: String?, optional: Boolean ) { - val arg = Argument(name!!, min, max, out!!, desc, optional) + val arg = Argument(name, min, max, out, desc, optional) addArgumentToList(arg) } @@ -174,14 +166,14 @@ abstract class Command(private val name: String, private val helpText: String? = * @brief Add a long Integer command argument */ fun addArgument( - name: String?, + name: String, min: Long, max: Long, - out: AtomicLong?, + out: AtomicLong, desc: String?, optional: Boolean ) { - val arg = Argument(name!!, min, max, out!!, desc, optional) + val arg = Argument(name, min, max, out, desc, optional) addArgumentToList(arg) } @@ -192,8 +184,8 @@ abstract class Command(private val name: String, private val helpText: String? = * @return The number of arguments currently added to the command * @brief Add an IP address command argument */ - fun addArgument(name: String?, out: IPAddress?, optional: Boolean) { - val arg = Argument(name!!, out!!, optional) + fun addArgument(name: String, out: IPAddress, optional: Boolean) { + val arg = Argument(name, out, optional) addArgumentToList(arg) } @@ -205,8 +197,8 @@ abstract class Command(private val name: String, private val helpText: String? = * @return The number of arguments currently added to the command * @brief Add a String command argument */ - fun addArgument(name: String?, out: StringBuffer?, desc: String?, optional: Boolean) { - val arg = Argument(name!!, out!!, desc, optional) + fun addArgument(name: String, out: StringBuffer, desc: String?, optional: Boolean) { + val arg = Argument(name, out, desc, optional) addArgumentToList(arg) } @@ -214,7 +206,7 @@ abstract class Command(private val name: String, private val helpText: String? = * @param args Supplied command-line arguments as an array of String objects. * @brief Initialize command arguments */ - fun initArguments(args: Array) { + fun setArgumentValues(args: Array) { val argc = args.size var mandatoryArgsCount = 0 var currentIndex = 0 @@ -224,12 +216,12 @@ abstract class Command(private val name: String, private val helpText: String? = } } require(argc >= mandatoryArgsCount) { - "initArguments: Wrong arguments number: $argc instead of $mandatoryArgsCount" + "setArgumentValues: Wrong arguments number: $argc instead of $mandatoryArgsCount" } // Initialize mandatory arguments for (i in 0 until mandatoryArgsCount) { - initArgument(currentIndex++, args[i]) + setArgumentValue(currentIndex++, args[i]) } // Initialize optional arguments @@ -242,18 +234,20 @@ abstract class Command(private val name: String, private val helpText: String? = !(args[i].length <= OPTIONAL_ARGUMENT_PREFIX_LENGTH && !args[i].startsWith(OPTIONAL_ARGUMENT_PREFIX)) ) { - "initArguments: Invalid optional argument: " + args[i] + "setArgumentValues: Invalid optional argument: " + args[i] } if (args[i].substring(OPTIONAL_ARGUMENT_PREFIX_LENGTH) == arguments[currentIndex].name) { - require(i + 1 < argc) { "initArguments: Optional argument " + args[i] + " missing value" } - initArgument(currentIndex++, args[i + 1]) + require(i + 1 < argc) { + "setArgumentValues: Optional argument " + args[i] + " missing value" + } + setArgumentValue(currentIndex++, args[i + 1]) } i += 2 } } - fun initArgument(argIndex: Int, argValue: String?) { - arguments[argIndex].setValue(argValue!!) + private fun setArgumentValue(argIndex: Int, argValue: String) { + arguments[argIndex].setValue(argValue) } @Throws(Exception::class) abstract fun run() diff --git a/examples/java-matter-controller/java/src/com/matter/controller/commands/common/CommandManager.kt b/examples/java-matter-controller/java/src/com/matter/controller/commands/common/CommandManager.kt index 033e71086d610c..927be309aee9a2 100644 --- a/examples/java-matter-controller/java/src/com/matter/controller/commands/common/CommandManager.kt +++ b/examples/java-matter-controller/java/src/com/matter/controller/commands/common/CommandManager.kt @@ -35,13 +35,13 @@ class CommandManager { val command: Command? if (args.size < 1) { logger.log(Level.INFO, "Missing cluster name") - showClusters() + showHelpInfo() return } val commands = clusters[args[0]] if (commands == null) { logger.log(Level.INFO, "Unknown cluster: " + args[0]) - showClusters() + showHelpInfo() return } if (args.size < 2) { @@ -85,7 +85,7 @@ class CommandManager { // need skip over binary and command name and only get arguments val temp = Arrays.copyOfRange(args, 2, args.size) try { - command.initArguments(temp) + command.setArgumentValues(temp) } catch (e: IllegalArgumentException) { logger.log(Level.INFO, "Arguments init failed with exception: " + e.message) showCommand(args[0], command) @@ -108,7 +108,7 @@ class CommandManager { private fun getCommand(commands: List, commandName: String): Command? { for (command in commands) { - if (commandName == command.getName()) { + if (commandName == command.name) { return command } } @@ -121,14 +121,14 @@ class CommandManager { attributeName: String ): Command? { for (command in commands) { - if (commandName == command.getName() && attributeName == command.getAttribute()) { + if (commandName == command.name && attributeName == command.getAttribute()) { return command } } return null } - private fun showClusters() { + private fun showHelpInfo() { logger.log(Level.INFO, "Usage:") logger.log(Level.INFO, " java-matter-controller cluster_name command_name [param1 param2 ...]") logger.log(Level.INFO, "\n") @@ -176,7 +176,7 @@ class CommandManager { var subscribeEventCommand = false for (command in commands) { var shouldPrint = true - val cmdName = command.getName() + val cmdName = command.name if (isGlobalCommand(cmdName)) { if (cmdName == "read" && !readCommand) { readCommand = true @@ -227,7 +227,7 @@ class CommandManager { " +-------------------------------------------------------------------------------------+" ) for (command in commands) { - if (commandName == command.getName()) { + if (commandName == command.name) { System.out.printf(" | * %-82s|\n", command.getAttribute()) } } @@ -258,7 +258,7 @@ class CommandManager { " +-------------------------------------------------------------------------------------+" ) for (command in commands) { - if (commandName == command.getName()) { + if (commandName == command.name) { System.out.printf(" | * %-82s|\n", command.getAttribute()) } } @@ -270,7 +270,7 @@ class CommandManager { private fun showCommand(clusterName: String, command: Command) { logger.log(Level.INFO, "Usage:") - var arguments: String? = command.getName() + var arguments: String? = command.name var description = "" val argumentsCount = command.getArgumentsCount() for (i in 0 until argumentsCount) { @@ -295,7 +295,7 @@ class CommandManager { } } System.out.format(" java-matter-controller %s %s\n", clusterName, arguments) - val helpText = command.getHelpText() + val helpText = command.helpText if (helpText != null) { System.out.format("\n%s\n", helpText) } diff --git a/examples/java-matter-controller/java/src/com/matter/controller/commands/common/CredentialsIssuer.kt b/examples/java-matter-controller/java/src/com/matter/controller/commands/common/CredentialsIssuer.kt index 806eac1635d486..d3e6d4e84fb48b 100644 --- a/examples/java-matter-controller/java/src/com/matter/controller/commands/common/CredentialsIssuer.kt +++ b/examples/java-matter-controller/java/src/com/matter/controller/commands/common/CredentialsIssuer.kt @@ -18,8 +18,7 @@ package com.matter.controller.commands.common /** - * @brief Credentials Issuer for the Command - * @details Contains all credential information of the issuer of the command, such as operational - * credentials for a given fabric, the DAC verifier of the commisioner, etc .. + * Credentials Issuer which contains all credential information of the issuer of the command, such + * as operational credentials for a given fabric, the DAC verifier of the commisioner, etc .. */ class CredentialsIssuer diff --git a/examples/java-matter-controller/java/src/com/matter/controller/commands/common/FutureResult.kt b/examples/java-matter-controller/java/src/com/matter/controller/commands/common/FutureResult.kt index c7b16bdae2991c..c04abd99188e9b 100644 --- a/examples/java-matter-controller/java/src/com/matter/controller/commands/common/FutureResult.kt +++ b/examples/java-matter-controller/java/src/com/matter/controller/commands/common/FutureResult.kt @@ -18,6 +18,7 @@ package com.matter.controller.commands.common +import java.util.concurrent.TimeoutException import java.util.logging.Level import java.util.logging.Logger @@ -27,8 +28,6 @@ import java.util.logging.Logger * duration elapsed without receiving the expected realResult, the runtime exception would be * raised. */ -class RealResultException(message: String) : RuntimeException(message) - class FutureResult { private var realResult: RealResult? = null private var timeoutMs: Long = 0 @@ -42,7 +41,7 @@ class FutureResult { fun setRealResult(realResult: RealResult) { synchronized(lock) { if (this.realResult != null) { - throw RealResultException("Error, real result has been set!") + throw TimeoutException("Error, real result has been set!") } this.realResult = realResult lock.notifyAll() @@ -55,16 +54,16 @@ class FutureResult { while (realResult == null) { try { if (System.currentTimeMillis() > start + timeoutMs) { - throw RealResultException("Timeout!") + throw TimeoutException("Timeout!") } lock.wait() } catch (e: InterruptedException) { logger.log(Level.INFO, "Wait Result failed with exception: " + e.message) } } - if (realResult?.getResult() == false) { - logger.log(Level.INFO, "Error: ${realResult?.getError()}") - throw RealResultException("Received failure test result") + if (realResult?.result == false) { + logger.log(Level.INFO, "Error: ${realResult?.error}") + throw TimeoutException("Received failure test result") } } } diff --git a/examples/java-matter-controller/java/src/com/matter/controller/commands/common/MatterCommand.kt b/examples/java-matter-controller/java/src/com/matter/controller/commands/common/MatterCommand.kt index e2b3acf7efa314..8732ad6a3b521d 100644 --- a/examples/java-matter-controller/java/src/com/matter/controller/commands/common/MatterCommand.kt +++ b/examples/java-matter-controller/java/src/com/matter/controller/commands/common/MatterCommand.kt @@ -20,7 +20,6 @@ package com.matter.controller.commands.common import chip.devicecontroller.ChipDeviceController import java.util.concurrent.atomic.AtomicBoolean import java.util.concurrent.atomic.AtomicLong -import java.util.logging.Logger abstract class MatterCommand( private val chipDeviceController: ChipDeviceController, @@ -111,8 +110,4 @@ abstract class MatterCommand( fun clear() { futureResult.clear() } - - companion object { - private val logger = Logger.getLogger(MatterCommand::class.java.name) - } } diff --git a/examples/java-matter-controller/java/src/com/matter/controller/commands/common/RealResult.kt b/examples/java-matter-controller/java/src/com/matter/controller/commands/common/RealResult.kt index 5cd5d80051efba..5ea218e32a2522 100644 --- a/examples/java-matter-controller/java/src/com/matter/controller/commands/common/RealResult.kt +++ b/examples/java-matter-controller/java/src/com/matter/controller/commands/common/RealResult.kt @@ -25,7 +25,7 @@ package com.matter.controller.commands.common * contain either a `true` value for `Success` or a `false` value in which case the failure will * also have an error string explaining the reason of the failure associated with it. */ -class RealResult(private val result: Boolean, private val error: String?) { +class RealResult(val result: Boolean, val error: String?) { constructor() : this(true, null) constructor(error: String?) : this(false, error) @@ -39,12 +39,4 @@ class RealResult(private val result: Boolean, private val error: String?) { return RealResult(error) } } - - fun getResult(): Boolean { - return result - } - - fun getError(): String? { - return error - } }