Simplify common.NewSAM

common/config.go

@@ -3,7 +3,6 @@ package common
 import (
 	"fmt"
 	"math/rand"
-	"net"
 	"strconv"
 	"strings"
 	"time"
@@ -11,51 +10,59 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
+const (
+	defaultSAMHost = "127.0.0.1"
+	defaultSAMPort = 7656
+)
+
 // Sam returns the SAM bridge address as a string in the format "host:port"
 func (f *I2PConfig) Sam() string {
-	// Set default values
-	host := "127.0.0.1"
-	port := 7656
-
-	// Override defaults if config values are set
-	if f.SamHost != "" {
-		host = f.SamHost
-	}
-	if f.SamPort != 0 {
-		port = f.SamPort
+	host := f.SamHost
+	if host == "" {
+		host = defaultSAMHost
 	}
 
-	// Log the SAM address being constructed
-	log.WithFields(logrus.Fields{
-		"host": host,
-		"port": port,
-	}).Debug("SAM address constructed")
+	port := f.SamPort
+	if port == 0 {
+		port = defaultSAMPort
+	}
 
-	// Return formatted SAM address
-	return net.JoinHostPort(host, strconv.Itoa(port))
+	return fmt.Sprintf("%s:%d", host, port)
+}
+
+// SAMAddress returns the SAM bridge address in the format "host:port"
+// This is a convenience method that uses the Sam() function to get the address.
+// It is used to provide a consistent interface for retrieving the SAM address.
+func (f *I2PConfig) SAMAddress() string {
+	// Return the SAM address in the format "host:port"
+	return f.Sam()
 }
 
 // SetSAMAddress sets the SAM bridge host and port from a combined address string.
 // If no address is provided, it sets default values for the host and port.
 func (f *I2PConfig) SetSAMAddress(addr string) {
 	if addr == "" {
-		f.SamHost = "127.0.0.1"
-		f.SamPort = 7656
+		f.SamHost = defaultSAMHost
+		f.SamPort = defaultSAMPort
 		return
 	}
 
-	host, port, err := net.SplitHostPort(addr)
+	host, portStr, err := SplitHostPort(addr)
 	if err != nil {
-		// Only set host if it looks valid
-		if net.ParseIP(addr) != nil || !strings.Contains(addr, ":") {
-			f.SamHost = addr
-		}
+		log.WithError(err).Warn("Failed to parse SAM address, using defaults")
+		f.SamHost = defaultSAMHost
+		f.SamPort = defaultSAMPort
 		return
 	}
 
 	f.SamHost = host
-	if p, err := strconv.Atoi(port); err == nil && p > 0 && p < 65536 {
-		f.SamPort = p
+
+	port, err := strconv.Atoi(portStr)
+	if err != nil || port < 1 || port > 65535 {
+		log.WithField("port", portStr).Warn("Invalid port, setting to 7656")
+		f.SamPort = defaultSAMPort
+	} else {
+		f.SamPort = port
 	}
 }
 

common/config_test.go

@@ -27,13 +27,13 @@ func TestSetSAMAddress_Cases(t *testing.T) {
 			name:     "invalid port uses default",
 			addr:     "localhost:99999",
 			wantHost: "localhost",
-			wantPort: 0,
+			wantPort: 7656, // Default port
 		},
 		{
 			name:     "just IP address",
 			addr:     "192.168.1.1",
 			wantHost: "192.168.1.1",
-			wantPort: 0,
+			wantPort: 7656,
 		},
 	}
 

common/new.go

@@ -7,68 +7,55 @@ import (
 	"github.com/samber/oops"
 )
 
-// Creates a new controller for the I2P routers SAM bridge.
-func OldNewSAM(address string) (*SAM, error) {
-	log.WithField("address", address).Debug("Creating new SAM instance")
-	var s SAM
-	// TODO: clean this up by refactoring the connection setup and error handling logic
-	conn, err := net.Dial("tcp", address)
-	if err != nil {
-		log.WithError(err).Error("Failed to dial SAM address")
-		return nil, oops.Errorf("error dialing to address '%s': %w", address, err)
-	}
-	if _, err := conn.Write(s.SAMEmit.HelloBytes()); err != nil {
-		log.WithError(err).Error("Failed to write hello message")
-		conn.Close()
-		return nil, oops.Errorf("error writing to address '%s': %w", address, err)
-	}
-	buf := make([]byte, 256)
-	n, err := conn.Read(buf)
-	if err != nil {
-		log.WithError(err).Error("Failed to read SAM response")
-		conn.Close()
-		return nil, oops.Errorf("error reading onto buffer: %w", err)
-	}
-	if strings.Contains(string(buf[:n]), HELLO_REPLY_OK) {
-		log.Debug("SAM hello successful")
-		s.SAMEmit.I2PConfig.SetSAMAddress(address)
-		s.Conn = conn
-		resolver, err := NewSAMResolver(&s)
-		s.SAMResolver = *resolver
-		if err != nil {
-			log.WithError(err).Error("Failed to create SAM resolver")
-			return nil, oops.Errorf("error creating resolver: %w", err)
-		}
-		return &s, nil
-	} else if string(buf[:n]) == HELLO_REPLY_NOVERSION {
-		log.Error("SAM bridge does not support SAMv3")
-		conn.Close()
-		return nil, oops.Errorf("That SAM bridge does not support SAMv3.")
-	} else {
-		log.WithField("response", string(buf[:n])).Error("Unexpected SAM response")
-		conn.Close()
-		return nil, oops.Errorf("%s", string(buf[:n]))
-	}
-}
-
+// NewSAM creates a new SAM instance by connecting to the specified address,
+// performing the hello handshake, and initializing the SAM resolver.
+// It returns a pointer to the SAM instance or an error if any step fails.
+// This function combines connection establishment and hello handshake into a single step,
+// eliminating the need for separate helper functions.
+// It also initializes the SAM resolver directly after the connection is established.
+// The SAM instance is ready to use for further operations like session creation or name resolution.
 func NewSAM(address string) (*SAM, error) {
 	logger := log.WithField("address", address)
 	logger.Debug("Creating new SAM instance")
 
-	conn, err := connectToSAM(address)
+	// Inline connection establishment - eliminates connectToSAM helper
+	conn, err := net.Dial("tcp", address)
 	if err != nil {
 		logger.WithError(err).Error("Failed to connect to SAM bridge")
-		return nil, err
+		return nil, oops.Errorf("failed to connect to SAM bridge at %s: %w", address, err)
 	}
 
 	s := &SAM{
 		Conn: conn,
 	}
 
-	if err = sendHelloAndValidate(conn, s); err != nil {
-		logger.WithError(err).Error("Failed to send hello and validate SAM connection")
+	// Inline hello handshake - eliminates sendHelloAndValidate helper
+	if _, err := conn.Write(s.SAMEmit.HelloBytes()); err != nil {
+		logger.WithError(err).Error("Failed to send hello message")
 		conn.Close()
-		return nil, err
+		return nil, oops.Errorf("failed to send hello message: %w", err)
+	}
+
+	buf := make([]byte, 256)
+	n, err := conn.Read(buf)
+	if err != nil {
+		logger.WithError(err).Error("Failed to read SAM response")
+		conn.Close()
+		return nil, oops.Errorf("failed to read SAM response: %w", err)
+	}
+
+	response := string(buf[:n])
+	switch {
+	case strings.Contains(response, HELLO_REPLY_OK):
+		logger.Debug("SAM hello successful")
+	case response == HELLO_REPLY_NOVERSION:
+		logger.Error("SAM bridge does not support SAMv3")
+		conn.Close()
+		return nil, oops.Errorf("SAM bridge does not support SAMv3")
+	default:
+		logger.WithField("response", response).Error("Unexpected SAM response")
+		conn.Close()
+		return nil, oops.Errorf("unexpected SAM response: %s", response)
 	}
 
 	s.SAMEmit.I2PConfig.SetSAMAddress(address)