Fix design and implementation smells

src/main/java/oakbot/CliArguments.java

@@ -74,6 +74,6 @@ All messages that are sent to the mock chat room are displayed in stdout (this
 		  Prints the version of this program.
 
 		--help
-		  Prints this help message.""".formatted(Main.VERSION, Main.URL, defaultContext);
+		  Prints this help message.""".formatted(Main.getVersion(), Main.getUrl(), defaultContext);
 	}
 }

src/main/java/oakbot/Main.java

@@ -47,9 +47,9 @@
 public final class Main {
 	private static final Logger logger = LoggerFactory.getLogger(Main.class);
 
-	public static final String VERSION;
-	public static final String URL;
-	public static final Instant BUILT;
+	private static final String VERSION;
+	private static final String URL;
+	private static final Instant BUILT;
 
 	static {
 		var props = new Properties();
@@ -78,24 +78,36 @@ public final class Main {
 		BUILT = built;
 	}
 
-	private static final String defaultContextPath = "bot-context.xml";
+	public static String getVersion() {
+		return VERSION;
+	}
+
+	public static String getUrl() {
+		return URL;
+	}
+
+	public static Instant getBuilt() {
+		return BUILT;
+	}
+
+	private static final String DEFAULT_CONTEXT_PATH = "bot-context.xml";
 
 	public static void main(String[] args) throws Exception {
 		var arguments = new CliArguments(args);
 
 		if (arguments.help()) {
-			var help = arguments.printHelp(defaultContextPath);
+			var help = arguments.printHelp(DEFAULT_CONTEXT_PATH);
 			System.out.println(help);
 			return;
 		}
 
 		if (arguments.version()) {
-			System.out.println(Main.VERSION);
+			System.out.println(Main.getVersion());
 			return;
 		}
 
 		var mock = arguments.mock();
-		var contextPath = (arguments.context() == null) ? defaultContextPath : arguments.context();
+		var contextPath = (arguments.context() == null) ? DEFAULT_CONTEXT_PATH : arguments.context();
 
 		BotProperties botProperties;
 		Database database;

src/main/java/oakbot/bot/Bot.java

@@ -16,6 +16,7 @@
 import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.PriorityBlockingQueue;
 import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Supplier;
 import java.util.regex.Pattern;
 
 import org.jsoup.Jsoup;
@@ -53,18 +54,14 @@
 public class Bot implements IBot {
 	private static final Logger logger = LoggerFactory.getLogger(Bot.class);
 	static final int BOTLER_ID = 13750349;
+
+    private static final Duration ROOM_JOIN_DELAY = Duration.ofSeconds(2);
 
-	private final String userName;
-	private final String trigger;
-	private final String greeting;
-	private final Integer userId;
+	private final BotConfiguration config;
+	private final SecurityConfiguration security;
 	private final IChatClient connection;
 	private final AtomicLong choreIdCounter = new AtomicLong();
 	private final BlockingQueue<Chore> choreQueue = new PriorityBlockingQueue<>();
-	private final List<Integer> admins;
-	private final List<Integer> bannedUsers;
-	private final List<Integer> allowedUsers;
-	private final Duration hideOneboxesAfter;
 	private final Rooms rooms;
 	private final Integer maxRooms;
 	private final List<Listener> listeners;
@@ -101,15 +98,13 @@ public class Bot implements IBot {
 	private Bot(Builder builder) {
 		connection = Objects.requireNonNull(builder.connection);
 
-		userName = (connection.getUsername() == null) ? builder.userName : connection.getUsername();
-		userId = (connection.getUserId() == null) ? builder.userId : connection.getUserId();
-		hideOneboxesAfter = builder.hideOneboxesAfter;
-		trigger = Objects.requireNonNull(builder.trigger);
-		greeting = builder.greeting;
+		var userName = (connection.getUsername() == null) ? builder.userName : connection.getUsername();
+		var userId = (connection.getUserId() == null) ? builder.userId : connection.getUserId();
+
+		config = new BotConfiguration(userName, userId, builder.trigger, builder.greeting, builder.hideOneboxesAfter);
+		security = new SecurityConfiguration(builder.admins, builder.bannedUsers, builder.allowedUsers);
+
 		maxRooms = builder.maxRooms;
-		admins = builder.admins;
-		bannedUsers = builder.bannedUsers;
-		allowedUsers = builder.allowedUsers;
 		stats = builder.stats;
 		database = (builder.database == null) ? new MemoryDatabase() : builder.database;
 		rooms = new Rooms(database, builder.roomsHome, builder.roomsQuiet);
@@ -160,7 +155,7 @@ private void joinRoomsOnStart(boolean quiet) {
 				 * resolve an issue where the bot chooses to ignore all messages
 				 * in certain rooms.
 				 */
-				Sleeper.sleep(2000);
+				Sleeper.sleep(ROOM_JOIN_DELAY);
 			}
 
 			try {
@@ -322,9 +317,9 @@ private IRoom joinRoom(int roomId, boolean quiet) throws RoomNotFoundException,
 		room.addEventListener(MessageEditedEvent.class, event -> choreQueue.add(new ChatEventChore(event)));
 		room.addEventListener(InvitationEvent.class, event -> choreQueue.add(new ChatEventChore(event)));
 
-		if (!quiet && greeting != null) {
+		if (!quiet && config.greeting() != null) {
 			try {
-				sendMessage(room, greeting);
+				sendMessage(room, config.greeting());
 			} catch (RoomPermissionException e) {
 				logger.atWarn().setCause(e).log(() -> "Unable to post greeting when joining room " + roomId + ".");
 			}
@@ -360,17 +355,21 @@ public void leave(int roomId) throws IOException {
 
 	@Override
 	public String getUsername() {
-		return userName;
+		return config.userName();
 	}
 
 	@Override
 	public Integer getUserId() {
-		return userId;
+		return config.userId();
 	}
 
 	@Override
 	public List<Integer> getAdminUsers() {
-		return admins;
+		return security.getAdmins();
+	}
+
+	private boolean isAdminUser(Integer userId) {
+		return security.isAdmin(userId);
 	}
 
 	@Override
@@ -381,7 +380,7 @@ public boolean isRoomOwner(int roomId, int userId) throws IOException {
 
 	@Override
 	public String getTrigger() {
-		return trigger;
+		return config.trigger();
 	}
 
 	@Override
@@ -603,28 +602,52 @@ public int compareTo(Chore that) {
 			 * The "lowest" value will be popped off the queue first.
 			 */
 
-			if (this instanceof StopChore && that instanceof StopChore) {
+			if (isBothStopChore(that)) {
 				return 0;
 			}
-			if (this instanceof StopChore) {
+			if (isThisStopChore()) {
 				return -1;
 			}
-			if (that instanceof StopChore) {
+			if (isThatStopChore(that)) {
 				return 1;
 			}
 
-			if (this instanceof CondenseMessageChore && that instanceof CondenseMessageChore) {
+			if (isBothCondenseMessageChore(that)) {
 				return Long.compare(this.choreId, that.choreId);
 			}
-			if (this instanceof CondenseMessageChore) {
+			if (isThisCondenseMessageChore()) {
 				return -1;
 			}
-			if (that instanceof CondenseMessageChore) {
+			if (isThatCondenseMessageChore(that)) {
 				return 1;
 			}
 
 			return Long.compare(this.choreId, that.choreId);
 		}
+
+		private boolean isBothStopChore(Chore that) {
+			return this instanceof StopChore && that instanceof StopChore;
+		}
+
+		private boolean isThisStopChore() {
+			return this instanceof StopChore;
+		}
+
+		private boolean isThatStopChore(Chore that) {
+			return that instanceof StopChore;
+		}
+
+		private boolean isBothCondenseMessageChore(Chore that) {
+			return this instanceof CondenseMessageChore && that instanceof CondenseMessageChore;
+		}
+
+		private boolean isThisCondenseMessageChore() {
+			return this instanceof CondenseMessageChore;
+		}
+
+		private boolean isThatCondenseMessageChore(Chore that) {
+			return that instanceof CondenseMessageChore;
+		}
 	}
 
 	private class StopChore extends Chore {
@@ -688,22 +711,30 @@ public void complete() {
 		}
 
 		private void handleMessage(ChatMessage message) {
-			if (timeout && !isAdminUser(message.getUserId())) {
+			var userId = message.getUserId();
+			var isAdminUser = isAdminUser(userId);
+			var isBotInTimeout = timeout && !isAdminUser;
+
+			if (isBotInTimeout) {
 				//bot is in timeout, ignore
 				return;
 			}
 
-			if (message.getContent() == null) {
+			var messageWasDeleted = message.getContent() == null;
+			if (messageWasDeleted) {
 				//user deleted their message, ignore
 				return;
 			}
 
-			if (!allowedUsers.isEmpty() && !allowedUsers.contains(message.getUserId())) {
+			var hasAllowedUsersList = !security.getAllowedUsers().isEmpty();
+			var userIsAllowed = security.isAllowed(userId);
+			if (hasAllowedUsersList && !userIsAllowed) {
 				//message was posted by a user who is not in the green list, ignore
 				return;
 			}
 
-			if (bannedUsers.contains(message.getUserId())) {
+			var userIsBanned = security.isBanned(userId);
+			if (userIsBanned) {
 				//message was posted by a banned user, ignore
 				return;
 			}
@@ -714,7 +745,7 @@ private void handleMessage(ChatMessage message) {
 				return;
 			}
 
-			if (message.getUserId() == userId) {
+			if (message.getUserId() == config.userId()) {
 				//message was posted by this bot
 				handleBotMessage(message);
 				return;
@@ -757,9 +788,9 @@ private void handleBotMessage(ChatMessage message) {
 			 * the URL is still preserved.
 			 */
 			var messageIsOnebox = message.getContent().isOnebox();
-			if (postedMessage != null && hideOneboxesAfter != null && (messageIsOnebox || postedMessage.isCondensableOrEphemeral())) {
+			if (postedMessage != null && config.hideOneboxesAfter() != null && (messageIsOnebox || postedMessage.isCondensableOrEphemeral())) {
 				var postedMessageAge = Duration.between(postedMessage.getTimePosted(), Instant.now());
-				var hideIn = hideOneboxesAfter.minus(postedMessageAge);
+				var hideIn = config.hideOneboxesAfter().minus(postedMessageAge);
 
 				logger.atInfo().log(() -> {
 					var action = messageIsOnebox ? "Hiding onebox" : "Condensing message";
@@ -796,33 +827,26 @@ private void handleActions(ChatMessage message, ChatActions actions) {
 			var queue = new LinkedList<>(actions.getActions());
 			while (!queue.isEmpty()) {
 				var action = queue.removeFirst();
+				processAction(action, message, queue);
+			}
+		}
 
-				if (action instanceof PostMessage pm) {
-					handlePostMessageAction(pm, message);
-					continue;
-				}
-
-				if (action instanceof DeleteMessage dm) {
-					var response = handleDeleteMessageAction(dm, message);
-					queue.addAll(response.getActions());
-					continue;
-				}
-
-				if (action instanceof JoinRoom jr) {
-					var response = handleJoinRoomAction(jr);
-					queue.addAll(response.getActions());
-					continue;
-				}
-
-				if (action instanceof LeaveRoom lr) {
-					handleLeaveRoomAction(lr);
-					continue;
-				}
-
-				if (action instanceof Shutdown) {
-					stop();
-					continue;
-				}
+		private void processAction(ChatAction action, ChatMessage message, LinkedList<ChatAction> queue) {
+			// Conditional dispatch based on action type (replaces polymorphism)
+			if (action instanceof PostMessage pm) {
+				handlePostMessageAction(pm, message);
+			} else if (action instanceof DeleteMessage dm) {
+				var response = handleDeleteMessageAction(dm, message);
+				queue.addAll(response.getActions());
+			} else if (action instanceof JoinRoom jr) {
+				var response = handleJoinRoomAction(jr);
+				queue.addAll(response.getActions());
+			} else if (action instanceof LeaveRoom lr) {
+				handleLeaveRoomAction(lr);
+			} else if (action instanceof Shutdown) {
+				stop();
+			} else {
+				logger.atWarn().log(() -> "Unknown action type: " + action.getClass().getName());
 			}
 		}
 
@@ -863,21 +887,10 @@ private ChatActions handleJoinRoomAction(JoinRoom action) {
 				if (joinedRoom.canPost()) {
 					return action.onSuccess().get();
 				}
-
-				try {
-					leave(action.roomId());
-				} catch (Exception e) {
-					logger.atError().setCause(e).log(() -> "Problem leaving room " + action.roomId() + " after it was found that the bot can't post messages to it.");
-				}
-
+				leaveRoomSafely(action.roomId(), () -> "Problem leaving room " + action.roomId() + " after it was found that the bot can't post messages to it.");				
 				return action.ifLackingPermissionToPost().get();
 			} catch (PrivateRoomException | RoomPermissionException e) {
-				try {
-					leave(action.roomId());
-				} catch (Exception e2) {
-					logger.atError().setCause(e2).log(() -> "Problem leaving room " + action.roomId() + " after it was found that the bot can't join or post messages to it.");
-				}
-
+				leaveRoomSafely(action.roomId(), () -> "Problem leaving room " + action.roomId() + " after it was found that the bot can't join or post messages to it.");				
 				return action.ifLackingPermissionToPost().get();
 			} catch (RoomNotFoundException e) {
 				return action.ifRoomDoesNotExist().get();
@@ -886,6 +899,20 @@ private ChatActions handleJoinRoomAction(JoinRoom action) {
 			}
 		}
 
+		/**
+		 * Attempts to leave a room and logs any errors that occur.
+		 * @param roomId the room ID to leave
+         * @param logMessage supplier for the complete log message (evaluated only if an error occurs)
+         **/
+        private void leaveRoomSafely(int roomId, Supplier<String> logMessage) {
+            try {
+				leave(roomId);
+			} 
+			catch (Exception e) {
+                logger.atError().setCause(e).log(logMessage);			
+			}
+		}
+
 		private void handleLeaveRoomAction(LeaveRoom action) {
 			try {
 				leave(action.roomId());