fix: address code review findings
This commit is contained in:
8
internal/cache/preferences.go
vendored
8
internal/cache/preferences.go
vendored
@@ -105,7 +105,7 @@ func (p *Preferences) AddSavedCity(ctx context.Context, userID, cityCode, cityNa
|
||||
Date: "",
|
||||
Request: "",
|
||||
}
|
||||
if err := p.store.Set(ctx, cacheKey, data, CityTTL); err != nil {
|
||||
if err := p.store.Set(ctx, cacheKey, data, PreferenceTTL); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
@@ -148,7 +148,7 @@ func (p *Preferences) RemoveSavedCity(ctx context.Context, userID, cityCode stri
|
||||
return err
|
||||
}
|
||||
|
||||
return p.store.Set(ctx, key, data, CityTTL)
|
||||
return p.store.Set(ctx, key, data, PreferenceTTL)
|
||||
}
|
||||
|
||||
// GetSearchHistory returns the user's search history.
|
||||
@@ -217,7 +217,7 @@ func (p *Preferences) AddSearchHistory(ctx context.Context, userID, fromCity, to
|
||||
return err
|
||||
}
|
||||
|
||||
if err := p.store.Set(ctx, key, data, CityTTL); err != nil {
|
||||
if err := p.store.Set(ctx, key, data, PreferenceTTL); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
@@ -260,7 +260,7 @@ func (p *Preferences) RemoveOldSearchHistory(ctx context.Context, userID string,
|
||||
return err
|
||||
}
|
||||
|
||||
if err := p.store.Set(ctx, key, data, CityTTL); err != nil {
|
||||
if err := p.store.Set(ctx, key, data, PreferenceTTL); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
|
||||
3
internal/cache/store.go
vendored
3
internal/cache/store.go
vendored
@@ -147,6 +147,9 @@ const (
|
||||
|
||||
// SearchFarTermTTL is the time-to-live for search results with far-term dates (7 days).
|
||||
SearchFarTermTTL = 7 * 24 * time.Hour
|
||||
|
||||
// PreferenceTTL is the time-to-live for user preferences (7 days).
|
||||
PreferenceTTL = 7 * 24 * time.Hour
|
||||
)
|
||||
|
||||
// GetCityKey returns the cache key for a city code.
|
||||
|
||||
@@ -808,6 +808,8 @@ func (g *Graph) ApplyMCT(itinerary *Itinerary, mctBase int) *Itinerary {
|
||||
adjustedLegs := make([]RouteLeg, len(itinerary.Legs))
|
||||
copy(adjustedLegs, itinerary.Legs)
|
||||
|
||||
totalMCT := 0
|
||||
|
||||
for i := 1; i < len(adjustedLegs); i++ {
|
||||
prevLeg := &adjustedLegs[i-1]
|
||||
currLeg := &adjustedLegs[i]
|
||||
@@ -828,9 +830,36 @@ func (g *Graph) ApplyMCT(itinerary *Itinerary, mctBase int) *Itinerary {
|
||||
}
|
||||
|
||||
// Add the MCT to the total duration (as waiting time at transfer)
|
||||
itinerary.TotalDuration += mct
|
||||
totalMCT += mct
|
||||
}
|
||||
|
||||
// Update leg durations to include MCT for transfer legs
|
||||
for i := 1; i < len(adjustedLegs); i++ {
|
||||
prevLeg := &adjustedLegs[i-1]
|
||||
currLeg := &adjustedLegs[i]
|
||||
|
||||
// Determine MCT based on node types and transfer kinds
|
||||
mct := mctBase
|
||||
|
||||
// Reduce MCT for city hub transfers (the transfer point node is a city)
|
||||
// The transfer point is the destination of the previous leg / start of current leg
|
||||
transferPoint := prevLeg.To // = currLeg.From
|
||||
if transferPoint.Type == NodeTypeCity {
|
||||
mct = mctBase / 2 // 30 min -> 15 min for city hub transfers
|
||||
}
|
||||
|
||||
// Increase MCT for mode changes (different transport types)
|
||||
if prevLeg.Transport != currLeg.Transport {
|
||||
mct = mctBase + 600 // 30 min + 10 min for mode change
|
||||
}
|
||||
|
||||
// Add MCT to the current leg's duration (transfer wait time)
|
||||
adjustedLegs[i].Duration += mct
|
||||
}
|
||||
|
||||
// Update total duration
|
||||
itinerary.TotalDuration += totalMCT
|
||||
|
||||
// Recalculate leg structure with proper transfer timing
|
||||
itinerary.Legs = adjustedLegs
|
||||
return itinerary
|
||||
@@ -894,14 +923,25 @@ func (g *Graph) FindRoutesPareto(originID, destID string, opts SearchOptions, cl
|
||||
var allItineraries []*Itinerary
|
||||
|
||||
// Search with different max transfer limits to find diverse routes
|
||||
for maxTransfers := 0; maxTransfers <= opts.MaxTransfers; maxTransfers++ {
|
||||
if opts.MaxTransfers < 0 {
|
||||
// No limit on transfers - use a reasonable default
|
||||
optsCopy := opts
|
||||
optsCopy.MaxTransfers = maxTransfers
|
||||
optsCopy.MaxTransfers = 5
|
||||
|
||||
result := g.FindRoute(originID, destID, optsCopy, closedStations, neighbors)
|
||||
if result != nil && result.TotalDuration > 0 {
|
||||
allItineraries = append(allItineraries, result)
|
||||
}
|
||||
} else {
|
||||
for maxTransfers := 0; maxTransfers <= opts.MaxTransfers; maxTransfers++ {
|
||||
optsCopy := opts
|
||||
optsCopy.MaxTransfers = maxTransfers
|
||||
|
||||
result := g.FindRoute(originID, destID, optsCopy, closedStations, neighbors)
|
||||
if result != nil && result.TotalDuration > 0 {
|
||||
allItineraries = append(allItineraries, result)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Sort according to the specified RankingMode
|
||||
@@ -1089,9 +1129,9 @@ func (g *Graph) checkRouteForChanges(itinerary *Itinerary) bool {
|
||||
|
||||
// rescheduleRoute performs a re-search for the route with updated graph data.
|
||||
// This is called when significant changes are detected in the route legs.
|
||||
func (g *Graph) rescheduleRoute(originID, destID string, opts SearchOptions) *Itinerary {
|
||||
func (g *Graph) rescheduleRoute(originID, destID string, opts SearchOptions, closedStations map[string]bool, neighbors map[string][]storage.StationNeighbor) *Itinerary {
|
||||
// Re-run the search with the same options to get an updated route
|
||||
result := g.FindRoute(originID, destID, opts, nil, nil)
|
||||
result := g.FindRoute(originID, destID, opts, closedStations, neighbors)
|
||||
if result != nil {
|
||||
result.LastChecked = time.Now().Unix()
|
||||
result.NeedsReSearch = false
|
||||
@@ -1102,9 +1142,9 @@ func (g *Graph) rescheduleRoute(originID, destID string, opts SearchOptions) *It
|
||||
|
||||
// CheckAndRescheduleRoute checks a route for changes and returns an updated route if needed.
|
||||
// This is the main entry point for flight change notification logic.
|
||||
func (g *Graph) CheckAndRescheduleRoute(itinerary *Itinerary, originID, destID string, opts SearchOptions) *Itinerary {
|
||||
func (g *Graph) CheckAndRescheduleRoute(itinerary *Itinerary, originID, destID string, opts SearchOptions, closedStations map[string]bool, neighbors map[string][]storage.StationNeighbor) *Itinerary {
|
||||
if g.checkRouteForChanges(itinerary) {
|
||||
return g.rescheduleRoute(originID, destID, opts)
|
||||
return g.rescheduleRoute(originID, destID, opts, closedStations, neighbors)
|
||||
}
|
||||
return itinerary
|
||||
}
|
||||
|
||||
@@ -388,7 +388,7 @@ func TestRouteReSearchOnChange(t *testing.T) {
|
||||
|
||||
// Since LastChecked is 2 hours ago (> 3600s ago), the recent-check skip won't apply
|
||||
// and checkRouteForChanges will run full evaluation
|
||||
checked := graph.CheckAndRescheduleRoute(itinerary, "s1", "s3", SearchOptions{MaxTransfers: 5})
|
||||
checked := graph.CheckAndRescheduleRoute(itinerary, "s1", "s3", SearchOptions{MaxTransfers: 5}, nil, nil)
|
||||
|
||||
t.Logf("Initial - NeedsReSearch: %v, ReSearchReason: %s", itinerary.NeedsReSearch, itinerary.ReSearchReason)
|
||||
t.Logf("Initial - checked route ID: %s, NeedsReSearch: %v", checked.ID, checked.NeedsReSearch)
|
||||
@@ -414,7 +414,7 @@ func TestRouteReSearchOnChange(t *testing.T) {
|
||||
}
|
||||
|
||||
// Re-check for changes after simulating cancellation
|
||||
checked2 := graph.CheckAndRescheduleRoute(itinerary, "s1", "s3", SearchOptions{MaxTransfers: 5})
|
||||
checked2 := graph.CheckAndRescheduleRoute(itinerary, "s1", "s3", SearchOptions{MaxTransfers: 5}, nil, nil)
|
||||
t.Logf("After cancellation - NeedsReSearch: %v, ReSearchReason: %s", checked2.NeedsReSearch, checked2.ReSearchReason)
|
||||
t.Logf("After cancellation - route ID: %s", checked2.ID)
|
||||
|
||||
@@ -451,7 +451,7 @@ func TestRouteReSearchOnChange(t *testing.T) {
|
||||
}
|
||||
|
||||
// Re-check for major delay
|
||||
checked3 := graph.CheckAndRescheduleRoute(itinerary2, "s1", "s3", SearchOptions{MaxTransfers: 5})
|
||||
checked3 := graph.CheckAndRescheduleRoute(itinerary2, "s1", "s3", SearchOptions{MaxTransfers: 5}, nil, nil)
|
||||
t.Logf("After major delay - NeedsReSearch: %v, ReSearchReason: %s", checked3.NeedsReSearch, checked3.ReSearchReason)
|
||||
t.Logf("After major delay - route ID: %s", checked3.ID)
|
||||
|
||||
|
||||
@@ -15,6 +15,9 @@ import (
|
||||
"trip-planner/internal/metrics"
|
||||
)
|
||||
|
||||
// rng is a seeded random number generator for jitter calculations.
|
||||
var rng = rand.New(rand.NewSource(time.Now().UnixNano()))
|
||||
|
||||
// Client represents a Yandex Schedules API client with rate limiting,
|
||||
// circuit breaking, and retry capabilities.
|
||||
type Client struct {
|
||||
@@ -421,6 +424,5 @@ func applyJitter(backoff time.Duration) time.Duration {
|
||||
}
|
||||
|
||||
func randFloat64() float64 {
|
||||
// Use math/rand with a seed based on function call index for variability
|
||||
return rand.Float64()
|
||||
return rng.Float64()
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user