Refactor/coding standards (#8)

* remove redundant comments to align with guidelines

* remove code duplication

* remove redundant code

Author: dotsem

internal/config/backup.go

@@ -30,7 +30,7 @@ func (m *Manager) EnsureFirstRunBackup() error {
 	metaPath := filepath.Join(backupDir, "metadata.json")
 
 	if _, err := os.Stat(metaPath); err == nil {
-		return nil // backup already exists
+		return nil
 	}
 
 	var existingFiles []string
@@ -43,7 +43,7 @@ func (m *Manager) EnsureFirstRunBackup() error {
 		}
 	}
 
-	// enforce secure, user-only read/write access to the backup folder
+	// why: openssh configuration backups must remain user-accessible only for security
 	if err := os.MkdirAll(backupDir, 0700); err != nil {
 		return fmt.Errorf("failed to create backup directory: %w", err)
 	}
@@ -73,7 +73,7 @@ func (m *Manager) EnsureFirstRunBackup() error {
 		return fmt.Errorf("failed to marshal backup metadata: %w", err)
 	}
 
-	// enforce secure user-only access on the metadata file
+	// why: metadata file stores sensitive backup paths and must be restricted
 	if err := os.WriteFile(metaPath, metaBytes, 0600); err != nil {
 		return fmt.Errorf("failed to write backup metadata: %w", err)
 	}

internal/config/backup_test.go

@@ -31,7 +31,7 @@ func TestEnsureFirstRunBackup(t *testing.T) {
 		backupDir := filepath.Join(tmpDir, "pre-tusshi")
 		assert.DirExists(t, backupDir)
 
-		// check folder permissions (must be 0700 on Unix)
+		// check folder permissions (must be 0700 on unix)
 		info, err := os.Stat(backupDir)
 		assert.NoError(t, err)
 		assert.Equal(t, os.ModeDir|0700, info.Mode().Perm()|os.ModeDir)

internal/config/config_file.go

@@ -28,7 +28,6 @@ func (m *Manager) AddConfigFile(targetPath string) error {
 		return err
 	}
 
-	// write a simple marker comment to represent a blank config file
 	if err := os.WriteFile(absTarget, []byte("# SSH config file created by tusshi\n"), 0600); err != nil {
 		return err
 	}

internal/config/editor.go

@@ -113,8 +113,7 @@ func (m *Manager) UpdateHost(originalAlias string, h *Host) error {
 	}
 
 	if newASTHost != nil {
-		// Keep any existing comment nodes from the original host block
-		// This makes sure we don't remove user generated comments from the file
+		// why: user comments must be preserved inside the edited host block to prevent data loss
 		for _, node := range targetCfg.Hosts[targetIdx].Nodes {
 			if _, isComment := node.(*ssh_config.Empty); isComment {
 				newASTHost.Nodes = append(newASTHost.Nodes, node)
@@ -223,13 +222,11 @@ func (m *Manager) registerInclude(includePath string) error {
 		}
 	}
 
-	// Decode the include line using parser to create the correct node type cleanly
 	decoded, err := ssh_config.Decode(strings.NewReader("Include " + relPath + "\n"))
 	if err != nil || len(decoded.Hosts) == 0 {
 		return fmt.Errorf("failed to generate include AST node: %w", err)
 	}
 
-	// Insert Include block at the very top of the primary config's Host list
 	primaryCfg.Hosts = append([]*ssh_config.Host{decoded.Hosts[0]}, primaryCfg.Hosts...)
 	return m.SaveFile(m.PrimaryPath)
 }

internal/config/manager.go

@@ -2,6 +2,7 @@ package config
 
 import (
 	"fmt"
+	"maps"
 	"os"
 	"path/filepath"
 	"reflect"
@@ -50,7 +51,6 @@ func (m *Manager) Load() error {
 // It automatically resolves wildcard settings for each specific host.
 func (m *Manager) GetHosts() []*Host {
 	var hosts []*Host
-	// We gather global wildcard configs to perform inheritance resolution later.
 	globalConfig := m.buildGlobalConfig()
 
 	for _, filePath := range m.FileOrder {
@@ -60,7 +60,7 @@ func (m *Manager) GetHosts() []*Host {
 		}
 
 		for _, astHost := range cfg.Hosts {
-			// Skip the implicit default "Host *" block added by the parser
+			// why: the parser injects a default implicit host block that we do not want to display
 			val := reflect.ValueOf(astHost)
 			if val.Kind() == reflect.Pointer && !val.IsNil() {
 				elem := val.Elem()
@@ -70,7 +70,6 @@ func (m *Manager) GetHosts() []*Host {
 				}
 			}
 
-			// Extract all aliases defined in this Host block.
 			for _, pat := range astHost.Patterns {
 				alias := pat.String()
 				if alias == "" {
@@ -85,26 +84,20 @@ func (m *Manager) GetHosts() []*Host {
 					Properties: make(map[string]string),
 				}
 
-				// Extract explicit key-value properties from the host block's nodes.
 				for _, node := range astHost.Nodes {
 					if kv, ok := node.(*ssh_config.KV); ok {
 						h.Properties[kv.Key] = kv.Value
 					}
 				}
 
-				// Map critical properties to top-level fields for convenience.
 				h.Name = h.Properties["HostName"]
 				h.User = h.Properties["User"]
 				h.Port = h.Properties["Port"]
 				h.IdentityFile = h.Properties["IdentityFile"]
 
-				// Resolve final values using global OpenSSH inheritance.
 				h.ResolvedProperties = make(map[string]string)
-				for k, v := range h.Properties {
-					h.ResolvedProperties[k] = v
-				}
+				maps.Copy(h.ResolvedProperties, h.Properties)
 
-				// Inject inherited properties from matching wildcard blocks.
 				if !isWildcard && globalConfig != nil {
 					for _, key := range []string{keyHostName, keyUser, keyPort, keyIdentityFile, keyForwardAgent, keyProxyJump} {
 						if _, explicit := h.Properties[key]; !explicit {
@@ -115,7 +108,6 @@ func (m *Manager) GetHosts() []*Host {
 					}
 				}
 
-				// Update resolved shortcuts.
 				if h.Name == "" && h.ResolvedProperties[keyHostName] != "" {
 					h.Name = h.ResolvedProperties[keyHostName]
 				}
@@ -150,7 +142,7 @@ func (m *Manager) loadPath(path string, depth int) error {
 	f, err := os.Open(filepath.Clean(path))
 	if err != nil {
 		if os.IsNotExist(err) && path == m.PrimaryPath {
-			// If primary file doesn't exist, we start with a clean empty config
+			// why: if primary file doesn't exist, we start with a clean empty config
 			m.Configs[path] = &ssh_config.Config{Hosts: []*ssh_config.Host{}}
 			return nil
 		}
@@ -167,7 +159,6 @@ func (m *Manager) loadPath(path string, depth int) error {
 
 	m.Configs[path] = cfg
 
-	// Scan AST nodes to discover and parse any Include directives.
 	for _, astHost := range cfg.Hosts {
 		for _, node := range astHost.Nodes {
 			if incl, ok := node.(*ssh_config.Include); ok {
@@ -208,7 +199,6 @@ func (m *Manager) resolveAndLoadIncludes(pattern string, depth int) {
 		}
 
 		if err := m.loadPath(absMatch, depth); err == nil {
-			// Track order of newly discovered files.
 			found := slices.Contains(m.FileOrder, absMatch)
 			if !found {
 				m.FileOrder = append(m.FileOrder, absMatch)
@@ -238,3 +228,14 @@ func expandTilde(path string) string {
 	}
 	return path
 }
+
+// FindConfigFile searches FileOrder for a path matching the given name,
+// base name, or extensionless nickname.
+func (m *Manager) FindConfigFile(name string) (string, bool) {
+	for _, file := range m.FileOrder {
+		if file == name || filepath.Base(file) == name || strings.TrimSuffix(filepath.Base(file), filepath.Ext(file)) == name {
+			return file, true
+		}
+	}
+	return "", false
+}

internal/config/manager_test.go

@@ -37,7 +37,7 @@ Host 10.200.1.46
 	assert.NoError(t, err)
 
 	hosts := mgr.GetHosts()
-	// Total hosts: Host *, prod-web-01, 10.200.1.46
+	// total hosts: Host *, prod-web-01, 10.200.1.46
 	assert.Len(t, hosts, 3)
 
 	var wildcardHost, prodHost, dbHost *Host
@@ -56,17 +56,17 @@ Host 10.200.1.46
 	assert.True(t, wildcardHost.IsWildcard)
 	assert.Equal(t, "default_user", wildcardHost.User)
 
-	// Verify prodHost details and explicit key mapping
+	// verify prodHost details and explicit key mapping
 	assert.NotNil(t, prodHost)
 	assert.False(t, prodHost.IsWildcard)
 	assert.Equal(t, "10.200.1.45", prodHost.Name)
 	assert.Equal(t, "deploy", prodHost.User)
 	assert.Equal(t, "~/.ssh/keys/work_rsa", prodHost.IdentityFile)
-	// Verify prodHost inherited port 2222 from wildcard Host *
+	// verify prodHost inherited port 2222 from wildcard Host *
 	assert.Equal(t, "2222", prodHost.ResolvedProperties["Port"])
 	assert.Equal(t, "yes", prodHost.ResolvedProperties[keyForwardAgent])
 
-	// Verify dbHost does not have alias (its alias is the IP itself)
+	// verify dbHost does not have alias (its alias is the IP itself)
 	assert.NotNil(t, dbHost)
 	assert.Equal(t, "10.200.1.46", dbHost.Alias)
 	// dbHost should inherit User, Port, and ForwardAgent from wildcard
@@ -136,7 +136,7 @@ Host my-host
 	err = mgr.AddHost(primaryPath, newHost)
 	assert.NoError(t, err)
 
-	// Reload to verify write
+	// reload to verify write
 	mgr2 := NewManager(primaryPath)
 	err = mgr2.Load()
 	assert.NoError(t, err)
@@ -169,7 +169,7 @@ Host my-host
 	err = mgr2.UpdateHost("added-host", updatedHost)
 	assert.NoError(t, err)
 
-	// Reload to verify update
+	// reload to verify update
 	mgr3 := NewManager(primaryPath)
 	err = mgr3.Load()
 	assert.NoError(t, err)
@@ -193,7 +193,7 @@ Host my-host
 	err = mgr3.DeleteHost("added-host-new")
 	assert.NoError(t, err)
 
-	// Reload to verify delete
+	// reload to verify delete
 	mgr4 := NewManager(primaryPath)
 	err = mgr4.Load()
 	assert.NoError(t, err)
@@ -213,21 +213,21 @@ func TestManagerConfigFileCRUD(t *testing.T) {
 
 	subPath := filepath.Join(tmpDir, "sub-config")
 
-	// 1. Add Config File
+	// 1. add config file
 	err = mgr.AddConfigFile(subPath)
 	assert.NoError(t, err)
 	assert.FileExists(t, subPath)
 	assert.Contains(t, mgr.FileOrder, subPath)
 
-	// Check if Include directive is added in primary
+	// check if Include directive is added in primary
 	// #nosec G304
 	primaryContent, err := os.ReadFile(primaryPath)
 	assert.NoError(t, err)
 	relSub, err := filepath.Rel(filepath.Dir(primaryPath), subPath)
 	assert.NoError(t, err)
 	assert.Contains(t, string(primaryContent), "Include "+relSub)
 
-	// 2. Rename Config File
+	// 2. rename config file
 	renamedPath := filepath.Join(tmpDir, "renamed-config")
 	err = mgr.RenameConfigFile(subPath, renamedPath)
 	assert.NoError(t, err)
@@ -236,7 +236,7 @@ func TestManagerConfigFileCRUD(t *testing.T) {
 	assert.Contains(t, mgr.FileOrder, renamedPath)
 	assert.NotContains(t, mgr.FileOrder, subPath)
 
-	// Check if Include is updated in primary
+	// check if Include is updated in primary
 	// #nosec G304
 	primaryContent2, err := os.ReadFile(primaryPath)
 	assert.NoError(t, err)
@@ -245,7 +245,7 @@ func TestManagerConfigFileCRUD(t *testing.T) {
 	assert.Contains(t, string(primaryContent2), "Include "+relRenamed)
 	assert.NotContains(t, string(primaryContent2), "Include "+relSub)
 
-	// 3. Prevent deleting if connections are present
+	// 3. prevent deleting if connections are present
 	h := &Host{
 		Alias: "test-host",
 		Name:  "127.0.0.1",
@@ -257,17 +257,17 @@ func TestManagerConfigFileCRUD(t *testing.T) {
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "connections are still present")
 
-	// Delete host first
+	// delete host first
 	err = mgr.DeleteHost("test-host")
 	assert.NoError(t, err)
 
-	// 4. Delete Config File successfully when no connections are present
+	// 4. delete config file successfully when no connections are present
 	err = mgr.DeleteConfigFile(renamedPath)
 	assert.NoError(t, err)
 	assert.NoFileExists(t, renamedPath)
 	assert.NotContains(t, mgr.FileOrder, renamedPath)
 
-	// Check if Include is removed from primary
+	// check if Include is removed from primary
 	// #nosec G304
 	primaryContent3, err := os.ReadFile(primaryPath)
 	assert.NoError(t, err)

internal/tui/commands/config.go

@@ -69,15 +69,8 @@ func RenameConfig(mgr *config.Manager, parts []string) func(Context) {
 			return
 		}
 
-		var oldPath string
-		for _, file := range mgr.FileOrder {
-			if file == oldName || filepath.Base(file) == oldName || strings.TrimSuffix(filepath.Base(file), filepath.Ext(file)) == oldName {
-				oldPath = file
-				break
-			}
-		}
-
-		if oldPath == "" {
+		oldPath, found := mgr.FindConfigFile(oldName)
+		if !found {
 			ctx.SetError(fmt.Sprintf("Config file %q not found", oldName))
 			return
 		}
@@ -122,15 +115,8 @@ func DeleteConfig(mgr *config.Manager, parts []string) func(Context) {
 			targetName = activeTab
 		}
 
-		var targetPath string
-		for _, file := range mgr.FileOrder {
-			if file == targetName || filepath.Base(file) == targetName || strings.TrimSuffix(filepath.Base(file), filepath.Ext(file)) == targetName {
-				targetPath = file
-				break
-			}
-		}
-
-		if targetPath == "" {
+		targetPath, found := mgr.FindConfigFile(targetName)
+		if !found {
 			ctx.SetError(fmt.Sprintf("Config file %q not found", targetName))
 			return
 		}

internal/tui/commands/move.go

@@ -3,7 +3,6 @@ package commands
 import (
 	"fmt"
 	"path/filepath"
-	"strings"
 
 	"tusshi/internal/config"
 )
@@ -17,15 +16,8 @@ func Move(mgr *config.Manager, selectedHost *config.Host, parts []string) func(C
 		}
 
 		targetNickname := parts[1]
-		var matchedFile string
-		for _, file := range mgr.FileOrder {
-			if filepath.Base(file) == targetNickname || strings.TrimSuffix(filepath.Base(file), filepath.Ext(file)) == targetNickname {
-				matchedFile = file
-				break
-			}
-		}
-
-		if matchedFile == "" {
+		matchedFile, found := mgr.FindConfigFile(targetNickname)
+		if !found {
 			matchedFile = filepath.Join(filepath.Dir(mgr.PrimaryPath), targetNickname)
 		}