refactor: deduplicate engine internals, replace bubble sorts, remove dead code

Extract shared code that was duplicated across functions:
- taskJSON struct (MarshalJSON/UnmarshalJSON) to package-level type
- scanTask(scanner) helper for GetTask/GetTasks (~70 identical lines)
- monthNames map for parseMonthName/parseDayAndMonth
- applyNonDateAttribute helper for Apply/ApplyToNew
- resolveDisplayID calls replace inline loops in FormatTaskListWithFormat

Replace O(n²) bubble sorts with sort.Slice in all four report sort
functions (sortByUrgency, NewestReport, NextReport, OldestReport).

Remove dead code: formatTimeWithColor (unused, also used time.Now()
instead of timeNow()), getCurrentTimestamp (unnecessary wrapper).

Remove ~20 comments that restated the next line of code.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

opal-task/internal/engine/database.go

@@ -334,7 +334,7 @@ func runMigrations() error {
 			if _, err := tx.Exec(
 				"INSERT INTO schema_version (version, applied_at) VALUES (?, ?)",
 				migration.version,
-				getCurrentTimestamp(),
+				GetCurrentTimestamp(),
 			); err != nil {
 				tx.Rollback()
 				return fmt.Errorf("failed to record migration %d: %w", migration.version, err)
@@ -349,14 +349,9 @@ func runMigrations() error {
 	return nil
 }
 
-// getCurrentTimestamp returns the current Unix timestamp
-func getCurrentTimestamp() int64 {
-	return timeNow().Unix()
-}
-
 // GetCurrentTimestamp returns the current Unix timestamp (exported for API use)
 func GetCurrentTimestamp() int64 {
-	return getCurrentTimestamp()
+	return timeNow().Unix()
 }
 
 // CleanupChangeLog removes old change log entries based on retention policy

opal-task/internal/engine/dateparse.go

@@ -7,6 +7,21 @@ import (
 	"time"
 )
 
+var monthNames = map[string]time.Month{
+	"jan": time.January, "january": time.January,
+	"feb": time.February, "february": time.February,
+	"mar": time.March, "march": time.March,
+	"apr": time.April, "april": time.April,
+	"may": time.May,
+	"jun": time.June, "june": time.June,
+	"jul": time.July, "july": time.July,
+	"aug": time.August, "august": time.August,
+	"sep": time.September, "september": time.September,
+	"oct": time.October, "october": time.October,
+	"nov": time.November, "november": time.November,
+	"dec": time.December, "december": time.December,
+}
+
 // DateParser handles all date/time/duration parsing with configurable options
 type DateParser struct {
 	base      time.Time
@@ -238,22 +253,7 @@ func (p *DateParser) parseWeekday(s string) (time.Time, bool) {
 
 // parseMonthName handles month names (jan, january, feb, february, etc.)
 func (p *DateParser) parseMonthName(s string) (time.Time, bool) {
-	months := map[string]time.Month{
-		"jan": time.January, "january": time.January,
-		"feb": time.February, "february": time.February,
-		"mar": time.March, "march": time.March,
-		"apr": time.April, "april": time.April,
-		"may": time.May,
-		"jun": time.June, "june": time.June,
-		"jul": time.July, "july": time.July,
-		"aug": time.August, "august": time.August,
-		"sep": time.September, "september": time.September,
-		"oct": time.October, "october": time.October,
-		"nov": time.November, "november": time.November,
-		"dec": time.December, "december": time.December,
-	}
-
-	month, ok := months[s]
+	month, ok := monthNames[s]
 	if !ok {
 		return time.Time{}, false
 	}
@@ -316,22 +316,7 @@ func (p *DateParser) parseDayAndMonth(dayStr, monthStr string) (int, time.Month,
 		return 0, 0, false
 	}
 
-	months := map[string]time.Month{
-		"jan": time.January, "january": time.January,
-		"feb": time.February, "february": time.February,
-		"mar": time.March, "march": time.March,
-		"apr": time.April, "april": time.April,
-		"may": time.May,
-		"jun": time.June, "june": time.June,
-		"jul": time.July, "july": time.July,
-		"aug": time.August, "august": time.August,
-		"sep": time.September, "september": time.September,
-		"oct": time.October, "october": time.October,
-		"nov": time.November, "november": time.November,
-		"dec": time.December, "december": time.December,
-	}
-
-	month, ok := months[monthStr]
+	month, ok := monthNames[monthStr]
 	if !ok {
 		return 0, 0, false
 	}

opal-task/internal/engine/display.go

@@ -29,15 +29,9 @@ func FormatTaskListWithFormat(tasks []*Task, ws *WorkingSet, format string) stri
 	if format == "minimal" {
 		result := ""
 		for i, task := range tasks {
-			displayID := i + 1
-			if ws != nil {
-				// Use working set display ID if available
-				for id, uuid := range ws.byID {
-					if uuid == task.UUID {
-						displayID = id
-						break
-					}
-				}
+			displayID := resolveDisplayID(task, ws)
+			if displayID == 0 {
+				displayID = i + 1
 			}
 			urgency := task.CalculateUrgency(coeffs)
 			urgencyColor := getUrgencyColor(urgency)
@@ -71,15 +65,9 @@ func FormatTaskListWithFormat(tasks []*Task, ws *WorkingSet, format string) stri
 
 	// Add rows
 	for i, task := range tasks {
-		displayID := i + 1
-		if ws != nil {
-			// Use working set display ID if available
-			for id, uuid := range ws.byID {
-				if uuid == task.UUID {
-					displayID = id
-					break
-				}
-			}
+		displayID := resolveDisplayID(task, ws)
+		if displayID == 0 {
+			displayID = i + 1
 		}
 
 		urgency := task.CalculateUrgency(coeffs)
@@ -270,8 +258,6 @@ func FormatTagCounts(tagCounts map[string]int) string {
 	return t.Render()
 }
 
-// Helper functions
-
 func formatStatus(status Status) string {
 	switch status {
 	case StatusPending:
@@ -322,7 +308,6 @@ func formatUrgency(urgency float64) string {
 }
 
 func getUrgencyColor(urgency float64) *color.Color {
-	// Returns color for minimal format
 	if urgency >= 10.0 {
 		return color.New(color.FgHiRed, color.Bold)
 	} else if urgency >= 5.0 {
@@ -362,14 +347,6 @@ func formatDue(due *time.Time) string {
 	return rel
 }
 
-func formatTimeWithColor(t time.Time) string {
-	now := time.Now()
-	if t.Before(now) {
-		return color.RedString(t.Format("2006-01-02 15:04"))
-	}
-	return t.Format("2006-01-02 15:04")
-}
-
 func formatTime(t time.Time) string {
 	return t.Format("2006-01-02 15:04")
 }

opal-task/internal/engine/filter.go

@@ -71,10 +71,8 @@ func (f *Filter) ToSQL() (string, []interface{}) {
 	conditions := []string{}
 	args := []interface{}{}
 
-	// Track if we have an explicit status filter
 	hasStatusFilter := false
 
-	// Status filter
 	if status, ok := f.Attributes["status"]; ok {
 		hasStatusFilter = true
 
@@ -104,13 +102,11 @@ func (f *Filter) ToSQL() (string, []interface{}) {
 		}
 	}
 
-	// Project filter
 	if project, ok := f.Attributes["project"]; ok {
 		conditions = append(conditions, "project = ?")
 		args = append(args, project)
 	}
 
-	// Priority filter
 	if priority, ok := f.Attributes["priority"]; ok {
 		priorityInt := priorityStringToInt(priority)
 		conditions = append(conditions, "priority = ?")
@@ -138,7 +134,6 @@ func (f *Filter) ToSQL() (string, []interface{}) {
 		args = append(args, tag)
 	}
 
-	// UUID filter
 	if len(f.UUIDs) > 0 {
 		placeholders := strings.Repeat("?,", len(f.UUIDs))
 		placeholders = placeholders[:len(placeholders)-1]

opal-task/internal/engine/modifier.go

@@ -92,7 +92,6 @@ func (m *Modifier) Apply(task *Task) error {
 	for _, key := range m.AttributeOrder {
 		valuePtr := m.SetAttributes[key]
 
-		// Handle date attributes with relative expression support
 		if dateKeys[key] {
 			if err := applyDateAttribute(key, valuePtr, task, resolvedDates); err != nil {
 				return err
@@ -100,30 +99,11 @@ func (m *Modifier) Apply(task *Task) error {
 			continue
 		}
 
-		// Handle non-date attributes
-		switch key {
-		case "priority":
-			if valuePtr == nil {
-				task.Priority = PriorityDefault
-			} else {
-				task.Priority = Priority(priorityStringToInt(*valuePtr))
-			}
-		case "project":
-			task.Project = valuePtr
-		case "recur":
-			if valuePtr == nil {
-				task.RecurrenceDuration = nil
-			} else {
-				duration, err := ParseRecurrencePattern(*valuePtr)
-				if err != nil {
-					return fmt.Errorf("invalid recurrence: %w", err)
-				}
-				task.RecurrenceDuration = &duration
-			}
+		if err := applyNonDateAttribute(key, valuePtr, task); err != nil {
+			return err
 		}
 	}
 
-	// Apply tag changes
 	for _, tag := range m.AddTags {
 		if err := task.AddTag(tag); err != nil {
 			return err
@@ -160,7 +140,6 @@ func (m *Modifier) ApplyToNew(task *Task) error {
 	for _, key := range m.AttributeOrder {
 		valuePtr := m.SetAttributes[key]
 
-		// Handle date attributes with relative expression support
 		if dateKeys[key] {
 			if err := applyDateAttribute(key, valuePtr, task, resolvedDates); err != nil {
 				return err
@@ -168,7 +147,17 @@ func (m *Modifier) ApplyToNew(task *Task) error {
 			continue
 		}
 
-		// Handle non-date attributes
+		if err := applyNonDateAttribute(key, valuePtr, task); err != nil {
+			return err
+		}
+	}
+
+	// Note: Tags are added after task is saved (in CreateTask function)
+	return nil
+}
+
+// applyNonDateAttribute applies a non-date attribute (priority, project, recur) to a task.
+func applyNonDateAttribute(key string, valuePtr *string, task *Task) error {
 	switch key {
 	case "priority":
 		if valuePtr == nil {
@@ -189,9 +178,6 @@ func (m *Modifier) ApplyToNew(task *Task) error {
 			task.RecurrenceDuration = &duration
 		}
 	}
-	}
-
-	// Note: Tags are added after task is saved (in CreateTask function)
 	return nil
 }
 

opal-task/internal/engine/parser.go

@@ -14,20 +14,16 @@ func ParseKeyValueFormat(data string, skipComments bool) (map[string]string, err
 	lines := strings.Split(data, "\n")
 
 	for i, line := range lines {
-		// Trim whitespace
 		line = strings.TrimSpace(line)
 
-		// Skip empty lines
 		if line == "" {
 			continue
 		}
 
-		// Skip comments if requested
 		if skipComments && strings.HasPrefix(line, "#") {
 			continue
 		}
 
-		// Split on first ':'
 		parts := strings.SplitN(line, ":", 2)
 		if len(parts) != 2 {
 			return nil, fmt.Errorf("line %d: invalid format (expected 'key:value')", i+1)

opal-task/internal/engine/report.go

@@ -2,6 +2,7 @@ package engine
 
 import (
 	"fmt"
+	"sort"
 )
 
 // DisplayFormat defines how tasks should be displayed
@@ -131,16 +132,11 @@ func NewestReport() *Report {
 		BaseFilter:    filter,
 		DisplayFormat: DisplayFormatTable,
 		SortFunc: func(tasks []*Task) []*Task {
-			// Sort by created descending
 			sorted := make([]*Task, len(tasks))
 			copy(sorted, tasks)
-			for i := 0; i < len(sorted)-1; i++ {
-				for j := i + 1; j < len(sorted); j++ {
-					if sorted[i].Created.Before(sorted[j].Created) {
-						sorted[i], sorted[j] = sorted[j], sorted[i]
-					}
-				}
-			}
+			sort.Slice(sorted, func(i, j int) bool {
+				return sorted[i].Created.After(sorted[j].Created)
+			})
 			return sorted
 		},
 		LimitFunc: func(tasks []*Task) []*Task {
@@ -164,23 +160,14 @@ func NextReport() *Report {
 		BaseFilter:    filter,
 		DisplayFormat: DisplayFormatTable,
 		SortFunc: func(tasks []*Task) []*Task {
-			// Sort by urgency descending
 			cfg, _ := GetConfig()
 			coeffs := BuildUrgencyCoefficients(cfg)
 
 			sorted := make([]*Task, len(tasks))
 			copy(sorted, tasks)
-
-			for i := 0; i < len(sorted)-1; i++ {
-				for j := i + 1; j < len(sorted); j++ {
-					urgI := sorted[i].CalculateUrgency(coeffs)
-					urgJ := sorted[j].CalculateUrgency(coeffs)
-					if urgI < urgJ {
-						sorted[i], sorted[j] = sorted[j], sorted[i]
-					}
-				}
-			}
-
+			sort.Slice(sorted, func(i, j int) bool {
+				return sorted[i].CalculateUrgency(coeffs) > sorted[j].CalculateUrgency(coeffs)
+			})
 			return sorted
 		},
 		LimitFunc: func(tasks []*Task) []*Task {
@@ -208,16 +195,11 @@ func OldestReport() *Report {
 		BaseFilter:    filter,
 		DisplayFormat: DisplayFormatTable,
 		SortFunc: func(tasks []*Task) []*Task {
-			// Sort by created ascending (already default, but explicit)
 			sorted := make([]*Task, len(tasks))
 			copy(sorted, tasks)
-			for i := 0; i < len(sorted)-1; i++ {
-				for j := i + 1; j < len(sorted); j++ {
-					if sorted[i].Created.After(sorted[j].Created) {
-						sorted[i], sorted[j] = sorted[j], sorted[i]
-					}
-				}
-			}
+			sort.Slice(sorted, func(i, j int) bool {
+				return sorted[i].Created.Before(sorted[j].Created)
+			})
 			return sorted
 		},
 	}
@@ -429,18 +411,13 @@ func sortByUrgency(tasks []*Task) []*Task {
 	sorted := make([]*Task, len(tasks))
 	copy(sorted, tasks)
 
-	// Calculate and store urgency on each task
 	for _, t := range sorted {
 		t.Urgency = t.CalculateUrgency(coeffs)
 	}
 
-	for i := 0; i < len(sorted)-1; i++ {
-		for j := i + 1; j < len(sorted); j++ {
-			if sorted[i].Urgency < sorted[j].Urgency {
-				sorted[i], sorted[j] = sorted[j], sorted[i]
-			}
-		}
-	}
+	sort.Slice(sorted, func(i, j int) bool {
+		return sorted[i].Urgency > sorted[j].Urgency
+	})
 
 	return sorted
 }

opal-task/internal/engine/task.go

@@ -83,9 +83,8 @@ type Task struct {
 	Urgency float64  `json:"urgency"`
 }
 
-// MarshalJSON emits Task with unix timestamps (int64) instead of RFC3339 strings.
-func (t Task) MarshalJSON() ([]byte, error) {
-	type taskJSON struct {
+// taskJSON is the wire format for Task, using unix timestamps instead of time.Time.
+type taskJSON struct {
 	UUID               uuid.UUID    `json:"uuid"`
 	ID                 int          `json:"id"`
 	Status             Status       `json:"status"`
@@ -105,8 +104,10 @@ func (t Task) MarshalJSON() ([]byte, error) {
 	Annotations        []Annotation `json:"annotations,omitempty"`
 	Tags               []string     `json:"tags"`
 	Urgency            float64      `json:"urgency"`
-	}
+}
 
+// MarshalJSON emits Task with unix timestamps (int64) instead of RFC3339 strings.
+func (t Task) MarshalJSON() ([]byte, error) {
 	toUnix := func(tp *time.Time) *int64 {
 		if tp == nil {
 			return nil
@@ -146,28 +147,6 @@ func (t Task) MarshalJSON() ([]byte, error) {
 
 // UnmarshalJSON parses Task from JSON with unix timestamps (int64) and duration in seconds.
 func (t *Task) UnmarshalJSON(data []byte) error {
-	type taskJSON struct {
-		UUID               uuid.UUID    `json:"uuid"`
-		ID                 int          `json:"id"`
-		Status             Status       `json:"status"`
-		Description        string       `json:"description"`
-		Project            *string      `json:"project"`
-		Priority           Priority     `json:"priority"`
-		Created            int64        `json:"created"`
-		Modified           int64        `json:"modified"`
-		Start              *int64       `json:"start,omitempty"`
-		End                *int64       `json:"end,omitempty"`
-		Due                *int64       `json:"due,omitempty"`
-		Scheduled          *int64       `json:"scheduled,omitempty"`
-		Wait               *int64       `json:"wait,omitempty"`
-		Until              *int64       `json:"until,omitempty"`
-		RecurrenceDuration *int64       `json:"recurrence_duration,omitempty"`
-		ParentUUID         *uuid.UUID   `json:"parent_uuid,omitempty"`
-		Annotations        []Annotation `json:"annotations,omitempty"`
-		Tags               []string     `json:"tags"`
-		Urgency            float64      `json:"urgency"`
-	}
-
 	var raw taskJSON
 	if err := json.Unmarshal(data, &raw); err != nil {
 		return err
@@ -366,21 +345,13 @@ func CreateTaskWithModifier(description string, mod *Modifier) (*Task, error) {
 	return task, nil
 }
 
-// GetTask retrieves a task by UUID
-func GetTask(taskUUID uuid.UUID) (*Task, error) {
-	db := GetDB()
-	if db == nil {
-		return nil, fmt.Errorf("database not initialized")
-	}
-
-	query := `
-		SELECT id, uuid, status, description, project, priority,
-		       created, modified, start, end, due, scheduled, wait, until_date,
-		       recurrence_duration, parent_uuid, annotations
-		FROM tasks
-		WHERE uuid = ?
-	`
+// scanner is satisfied by both *sql.Row and *sql.Rows.
+type scanner interface {
+	Scan(dest ...interface{}) error
+}
 
+// scanTask reads a single task row from a scanner and populates all fields including tags.
+func scanTask(s scanner) (*Task, error) {
 	task := &Task{}
 	var (
 		uuidStr        string
@@ -398,7 +369,7 @@ func GetTask(taskUUID uuid.UUID) (*Task, error) {
 		annotationsStr interface{}
 	)
 
-	err := db.QueryRow(query, taskUUID.String()).Scan(
+	err := s.Scan(
 		&task.ID,
 		&uuidStr,
 		&task.Status,
@@ -417,22 +388,18 @@ func GetTask(taskUUID uuid.UUID) (*Task, error) {
 		&parentUUIDStr,
 		&annotationsStr,
 	)
-
 	if err != nil {
-		return nil, fmt.Errorf("failed to get task: %w", err)
+		return nil, err
 	}
 
-	// Parse UUID
 	task.UUID, err = uuid.Parse(uuidStr)
 	if err != nil {
 		return nil, fmt.Errorf("failed to parse UUID: %w", err)
 	}
 
-	// Convert timestamps
 	task.Created = time.Unix(created, 0)
 	task.Modified = time.Unix(modified, 0)
 
-	// Convert nullable fields
 	task.Project = sqlToStringPtr(project)
 	task.Start = sqlToTime(start)
 	task.End = sqlToTime(end)
@@ -444,7 +411,6 @@ func GetTask(taskUUID uuid.UUID) (*Task, error) {
 	task.ParentUUID = sqlToUUIDPtr(parentUUIDStr)
 	task.Annotations = sqlToAnnotations(annotationsStr)
 
-	// Load tags
 	tags, err := task.GetTags()
 	if err != nil {
 		return nil, fmt.Errorf("failed to load tags: %w", err)
@@ -454,6 +420,29 @@ func GetTask(taskUUID uuid.UUID) (*Task, error) {
 	return task, nil
 }
 
+// GetTask retrieves a task by UUID
+func GetTask(taskUUID uuid.UUID) (*Task, error) {
+	db := GetDB()
+	if db == nil {
+		return nil, fmt.Errorf("database not initialized")
+	}
+
+	query := `
+		SELECT id, uuid, status, description, project, priority,
+		       created, modified, start, end, due, scheduled, wait, until_date,
+		       recurrence_duration, parent_uuid, annotations
+		FROM tasks
+		WHERE uuid = ?
+	`
+
+	task, err := scanTask(db.QueryRow(query, taskUUID.String()))
+	if err != nil {
+		return nil, fmt.Errorf("failed to get task: %w", err)
+	}
+
+	return task, nil
+}
+
 // GetTasks retrieves all tasks with optional filtering
 func GetTasks(filter *Filter) ([]*Task, error) {
 	db := GetDB()
@@ -489,76 +478,10 @@ func GetTasks(filter *Filter) ([]*Task, error) {
 	tasks := []*Task{}
 
 	for rows.Next() {
-		task := &Task{}
-		var (
-			uuidStr        string
-			project        interface{}
-			created        int64
-			modified       int64
-			start          interface{}
-			end            interface{}
-			due            interface{}
-			scheduled      interface{}
-			wait           interface{}
-			until          interface{}
-			recurDuration  interface{}
-			parentUUIDStr  interface{}
-			annotationsStr interface{}
-		)
-
-		err := rows.Scan(
-			&task.ID,
-			&uuidStr,
-			&task.Status,
-			&task.Description,
-			&project,
-			&task.Priority,
-			&created,
-			&modified,
-			&start,
-			&end,
-			&due,
-			&scheduled,
-			&wait,
-			&until,
-			&recurDuration,
-			&parentUUIDStr,
-			&annotationsStr,
-		)
-
+		task, err := scanTask(rows)
 		if err != nil {
 			return nil, fmt.Errorf("failed to scan task: %w", err)
 		}
-
-		// Parse UUID
-		task.UUID, err = uuid.Parse(uuidStr)
-		if err != nil {
-			return nil, fmt.Errorf("failed to parse UUID: %w", err)
-		}
-
-		// Convert timestamps
-		task.Created = time.Unix(created, 0)
-		task.Modified = time.Unix(modified, 0)
-
-		// Convert nullable fields
-		task.Project = sqlToStringPtr(project)
-		task.Start = sqlToTime(start)
-		task.End = sqlToTime(end)
-		task.Due = sqlToTime(due)
-		task.Scheduled = sqlToTime(scheduled)
-		task.Wait = sqlToTime(wait)
-		task.Until = sqlToTime(until)
-		task.RecurrenceDuration = sqlToDuration(recurDuration)
-		task.ParentUUID = sqlToUUIDPtr(parentUUIDStr)
-		task.Annotations = sqlToAnnotations(annotationsStr)
-
-		// Load tags
-		tags, err := task.GetTags()
-		if err != nil {
-			return nil, fmt.Errorf("failed to load tags: %w", err)
-		}
-		task.Tags = tags
-
 		tasks = append(tasks, task)
 	}