From 972b4c11c596c6d6592762cd9b50196905207d8d Mon Sep 17 00:00:00 2001
From: Slavi Pantaleev <slavi@devture.com>
Date: Sat, 27 Aug 2022 19:21:18 +0300
Subject: [PATCH] Do not call getSettings() for each option in help

Retrieving the settings object for each and every option was wasteful

I don't like how we're doing custom formatting for optionMailbox in many
different places. This should be redone.
---
 bot/command.go  | 38 +++++++++++++++++++++++++-------------
 bot/settings.go | 19 -------------------
 2 files changed, 25 insertions(+), 32 deletions(-)

diff --git a/bot/command.go b/bot/command.go
index d514c2f..ebf08c9 100644
--- a/bot/command.go
+++ b/bot/command.go
@@ -162,6 +162,11 @@ func (b *Bot) sendIntroduction(ctx context.Context, roomID id.RoomID) {
 func (b *Bot) sendHelp(ctx context.Context, roomID id.RoomID) {
 	evt := eventFromContext(ctx)
 
+	settings, settingsErr := b.getSettings(evt.RoomID)
+	if settingsErr != nil {
+		b.Error(ctx, evt.RoomID, settingsErr.Error())
+	}
+
 	var msg strings.Builder
 	msg.WriteString("The following commands are supported:\n\n")
 	for _, command := range commands {
@@ -171,16 +176,16 @@ func (b *Bot) sendHelp(ctx context.Context, roomID id.RoomID) {
 		msg.WriteString(command.key)
 		msg.WriteString("`**")
 
-		if command.isOption {
-			value, err := b.getSettingsOption(evt.RoomID, command.key)
-			if err != nil {
-				b.Error(ctx, evt.RoomID, err.Error())
+		if command.isOption && settingsErr == nil {
+			value := settings.Get(command.key)
+
+			if value == "" {
+				msg.WriteString(" (currently not set)")
 			} else {
-				if value == nil {
-					msg.WriteString(" (currently not set)")
-				} else {
-					msg.WriteString(fmt.Sprintf(" (currently `%v`)", value))
+				if command.key == optionMailbox {
+					value = fmt.Sprintf("%s@%s", value, b.domain)
 				}
+				msg.WriteString(fmt.Sprintf(" (currently `%v`)", value))
 			}
 		}
 
@@ -232,19 +237,26 @@ func (b *Bot) handleOption(ctx context.Context, command []string) {
 }
 
 func (b *Bot) getOption(ctx context.Context, name string) {
-	evt := eventFromContext(ctx)
+	msg := "`%s` of this room is `%s`"
 
-	value, err := b.getSettingsOption(evt.RoomID, name)
+	evt := eventFromContext(ctx)
+	cfg, err := b.getSettings(evt.RoomID)
 	if err != nil {
-		b.Error(ctx, evt.RoomID, err.Error())
+		b.Error(ctx, evt.RoomID, "failed to retrieve settings: %v", err)
 		return
 	}
-	if value == nil {
+
+	value := cfg.Get(name)
+	if value == "" {
 		b.Notice(ctx, evt.RoomID, fmt.Sprintf("`%s` is not set, kupo.", name))
 		return
 	}
 
-	b.Notice(ctx, evt.RoomID, fmt.Sprintf("`%s` of this room is `%s`", name, value))
+	if name == optionMailbox {
+		value = fmt.Sprintf("%s@%s", value, b.domain)
+	}
+
+	b.Notice(ctx, evt.RoomID, fmt.Sprintf(msg, name, value))
 }
 
 func (b *Bot) setOption(ctx context.Context, name, value string) {
diff --git a/bot/settings.go b/bot/settings.go
index 332d74f..ad37899 100644
--- a/bot/settings.go
+++ b/bot/settings.go
@@ -1,7 +1,6 @@
 package bot
 
 import (
-	"fmt"
 	"strconv"
 	"strings"
 
@@ -116,24 +115,6 @@ func (b *Bot) getSettings(roomID id.RoomID) (settings, error) {
 	return config, err
 }
 
-func (b *Bot) getSettingsOption(roomID id.RoomID, name string) (any, error) {
-	cfg, err := b.getSettings(roomID)
-	if err != nil {
-		return nil, fmt.Errorf("failed to retrieve settings: %v", err)
-	}
-
-	value := cfg.Get(name)
-	if value == "" {
-		return nil, nil
-	}
-
-	if name == optionMailbox {
-		value = fmt.Sprintf("%s@%s", value, b.domain)
-	}
-
-	return value, nil
-}
-
 func (b *Bot) setSettings(roomID id.RoomID, cfg settings) error {
 	return b.lp.GetClient().SetRoomAccountData(roomID, settingskey, cfg)
 }