refactor(optimiser): include slots with unknown venue locations and several code quality improvements#4340
Conversation
|
@thejus03 is attempting to deploy a commit to the modsbot's projects Team on Vercel. A member of the Team first needs to authorize it. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4340 +/- ##
==========================================
+ Coverage 54.52% 56.64% +2.12%
==========================================
Files 274 308 +34
Lines 6076 6964 +888
Branches 1455 1682 +227
==========================================
+ Hits 3313 3945 +632
- Misses 2763 3019 +256 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jloh02
left a comment
There was a problem hiding this comment.
Thanks for the updates. it makes sense. sorry for backtracking on some of the older PR stuff. wanted to clean up the code now that i have time to properly review. would be good to check out go best practices when you have time too
…alidCoordinates function
…o include recordings and free days maps
…" for consistency
…improved validation
…ed memory efficiency
…-existent modules, and method restrictions
…nfiguration options
|
@jloh02 Since we have decided to refactor the code now, I have made several changes in the code and abstracted it out. I also made few performance improvements due to unnecessary copies that were being made. I have updated the PR description to match the changes. Beside including slots without venue locations (the original PR) there has been no other functional changes. |
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| website/api/optimiser/_modules/modules.go | Core logic change: slots with unknown venues now included (given InvalidCoordinates) instead of being skipped. Contains a bug where pre-allocated weeksStrings slice produces malformed WeeksString (e.g. "3,,5") when non-float64 week values are encountered. |
| website/api/optimiser/_solver/solver.go | Significant refactor: Solve() and beamSearch() reorganised, Score field now pre-computed before sorting (avoids repeated scoring), variable shadowing fixed, DaysPerWeek constant used. Validation errors from GetAllModuleSlots are returned as HTTP 500 instead of the more appropriate 400. |
| website/api/optimiser/_models/models.go | Clean additions: ParseOptimiserRequestFields() with empty-modules guard, json:"-" tags on computed fields, Score field on TimetableState, SolveResponse moved here, WeeksSet changed to map[int]struct{}. |
| website/api/optimiser/_constants/constants.go | Magic numbers replaced with named constants (BeamWidth, BranchingFactor, DaysPerWeek, NoVenuePenalty), EVenues map changed to map[string]struct{}, var→const for URL constants, InvalidCoordinates sentinel added. |
| website/api/optimiser/_client/client.go | HTTP status check added — non-200 responses from NUSMods API now return a descriptive error instead of silently forwarding bad data. GetVenues() correctly moved to _modules package. |
| website/api/optimiser/_solver/nusmods_link.go | serializeLessonIndices simplified from fmt.Sprint/strings.Fields pipeline to a clean strconv.Itoa loop; CreateConfig/SerializeConfig made unexported; GenerateNUSModsShareableLink moved to top of file. |
| website/api/optimiser/_test/api_test.go | Six new integration tests added covering empty modules, invalid time format, non-existent module, method not allowed, shareable link validation, and assignment consistency. Recordings format updated to pipe separator throughout. |
| website/api/optimiser/optimise.go | JSON decode error now returns a generic "Invalid request format" message instead of leaking internal error details; doc comment converted to contiguous godoc block. |
| website/src/views/optimiser/OptimiserContainer/OptimiserContent.tsx | recordings payload switched from displayText to lessonKey, aligning the frontend with the new pipe-separated format (e.g. "CS1010S |
| website/src/views/optimiser/OptimiserResults.tsx | Removes "Missing venue information" from the unscheduled-lessons explanation, accurately reflecting that slots with unknown venues are now included in optimisation. |
Sequence Diagram
sequenceDiagram
participant FE as Frontend (OptimiserContent.tsx)
participant H as Handler (optimise.go)
participant S as Solve (_solver/solver.go)
participant M as GetAllModuleSlots (_modules/modules.go)
participant C as GetModuleData (_client/client.go)
participant API as NUSMods API
participant BS as beamSearch (_solver/solver.go)
participant L as GenerateNUSModsShareableLink (_solver/nusmods_link.go)
FE->>H: POST /api/optimiser/optimise (recordings as "MODULE|LessonType")
H->>H: json.Decode → OptimiserRequest
H->>S: Solve(w, optimiserRequest)
S->>M: GetAllModuleSlots(&req)
M->>M: ParseOptimiserRequestFields() [validates empty modules, time formats]
M->>M: getVenues() [unmarshal venues.json]
loop for each module
M->>C: GetModuleData(acadYear, module)
C->>API: GET /v2/{year}/modules/{module}.json
API-->>C: 200 OK / non-200 error
C-->>M: []byte or error (non-200 now returns error)
M->>M: parse weeks with comma-ok float64 assertion
M->>M: mergeAndFilterModuleSlots() [unknown venues → InvalidCoordinates]
end
M-->>S: moduleSlots, defaultSlots, recordingsMap
S->>BS: beamSearch(lessons sorted by MRV, ...)
BS->>BS: expand beam, score states (pre-compute Score field), prune to BeamWidth
BS-->>S: best TimetableState
S->>L: GenerateNUSModsShareableLink(assignments, defaultSlots, ...)
L-->>S: shareableLink, defaultShareableLink
S-->>H: JSON encode SolveResponse
H-->>FE: 200 OK { Assignments, DaySlots, Score, shareableLink, defaultShareableLink }
Last reviewed commit: 576ae48
| weeksStrings := make([]string, len(weeks)) | ||
|
|
||
| for j, week := range weeks { | ||
| weekInt := int(week.(float64)) | ||
| moduleTimetable[i].WeeksSet[weekInt] = true | ||
| weekFloat, ok := week.(float64) | ||
| if !ok { | ||
| continue | ||
| } | ||
| weekInt := int(weekFloat) | ||
| moduleTimetable[i].WeeksSet[weekInt] = struct{}{} | ||
| weeksStrings[j] = strconv.Itoa(weekInt) | ||
| } | ||
| moduleTimetable[i].WeeksString = strings.Join(weeksStrings, ",") |
There was a problem hiding this comment.
Malformed WeeksString when non-float64 week value is encountered
weeksStrings is pre-allocated with make([]string, len(weeks)), so each element starts as "". When the comma-ok check fails and continue is hit for index j, weeksStrings[j] remains "". The subsequent strings.Join then produces a string with empty segments, e.g. "3,,5,6" or ",4,5".
This malformed WeeksString is used to build the combinationKey for deduplication (line 228 in mergeAndFilterModuleSlots), which could cause incorrect deduplication of class options.
Switch to an append-based approach so failed conversions are simply omitted:
| weeksStrings := make([]string, len(weeks)) | |
| for j, week := range weeks { | |
| weekInt := int(week.(float64)) | |
| moduleTimetable[i].WeeksSet[weekInt] = true | |
| weekFloat, ok := week.(float64) | |
| if !ok { | |
| continue | |
| } | |
| weekInt := int(weekFloat) | |
| moduleTimetable[i].WeeksSet[weekInt] = struct{}{} | |
| weeksStrings[j] = strconv.Itoa(weekInt) | |
| } | |
| moduleTimetable[i].WeeksString = strings.Join(weeksStrings, ",") | |
| moduleTimetable[i].WeeksSet = make(map[int]struct{}) | |
| weeks := moduleTimetable[i].Weeks.([]any) | |
| var weeksStrings []string | |
| for _, week := range weeks { | |
| weekFloat, ok := week.(float64) | |
| if !ok { | |
| continue | |
| } | |
| weekInt := int(weekFloat) | |
| moduleTimetable[i].WeeksSet[weekInt] = struct{}{} | |
| weeksStrings = append(weeksStrings, strconv.Itoa(weekInt)) | |
| } | |
| moduleTimetable[i].WeeksString = strings.Join(weeksStrings, ",") |
Prompt To Fix With AI
This is a comment left during a code review.
Path: website/api/optimiser/_modules/modules.go
Line: 84-95
Comment:
**Malformed `WeeksString` when non-float64 week value is encountered**
`weeksStrings` is pre-allocated with `make([]string, len(weeks))`, so each element starts as `""`. When the comma-ok check fails and `continue` is hit for index `j`, `weeksStrings[j]` remains `""`. The subsequent `strings.Join` then produces a string with empty segments, e.g. `"3,,5,6"` or `",4,5"`.
This malformed `WeeksString` is used to build the `combinationKey` for deduplication (line 228 in `mergeAndFilterModuleSlots`), which could cause incorrect deduplication of class options.
Switch to an `append`-based approach so failed conversions are simply omitted:
```suggestion
moduleTimetable[i].WeeksSet = make(map[int]struct{})
weeks := moduleTimetable[i].Weeks.([]any)
var weeksStrings []string
for _, week := range weeks {
weekFloat, ok := week.(float64)
if !ok {
continue
}
weekInt := int(weekFloat)
moduleTimetable[i].WeeksSet[weekInt] = struct{}{}
weeksStrings = append(weeksStrings, strconv.Itoa(weekInt))
}
moduleTimetable[i].WeeksString = strings.Join(weeksStrings, ",")
```
How can I resolve this? If you propose a fix, please make it concise.| slots, defaultSlots, recordings, err := modules.GetAllModuleSlots(&req) | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| return | ||
| } |
There was a problem hiding this comment.
Validation errors returned as HTTP 500 instead of 400
GetAllModuleSlots can return errors from two very different sources:
- Client errors (user-caused): empty modules list, invalid time format — these originate from
ParseOptimiserRequestFields()and should be a400 Bad Request. - Server errors (infrastructure): failed HTTP request to NUSMods API, JSON parse failure — these warrant a
500 Internal Server Error.
Currently both cases are returned as 500, which misrepresents client errors and may confuse frontend error handling. Consider distinguishing them — for example by using a sentinel error type or by returning a status code alongside the error from GetAllModuleSlots:
slots, defaultSlots, recordings, err := modules.GetAllModuleSlots(&req)
if err != nil {
// Distinguish user errors (validation) from server errors
// e.g. wrap validation errors with a distinct type and check here
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}TestOptimiser_EmptyModules and TestOptimiser_InvalidTimeFormat only verify a non-200 response, so they pass today with a 500 — but the semantically correct response is 400.
Prompt To Fix With AI
This is a comment left during a code review.
Path: website/api/optimiser/_solver/solver.go
Line: 23-27
Comment:
**Validation errors returned as HTTP 500 instead of 400**
`GetAllModuleSlots` can return errors from two very different sources:
1. **Client errors** (user-caused): empty modules list, invalid time format — these originate from `ParseOptimiserRequestFields()` and should be a `400 Bad Request`.
2. **Server errors** (infrastructure): failed HTTP request to NUSMods API, JSON parse failure — these warrant a `500 Internal Server Error`.
Currently both cases are returned as `500`, which misrepresents client errors and may confuse frontend error handling. Consider distinguishing them — for example by using a sentinel error type or by returning a status code alongside the error from `GetAllModuleSlots`:
```go
slots, defaultSlots, recordings, err := modules.GetAllModuleSlots(&req)
if err != nil {
// Distinguish user errors (validation) from server errors
// e.g. wrap validation errors with a distinct type and check here
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
```
`TestOptimiser_EmptyModules` and `TestOptimiser_InvalidTimeFormat` only verify a non-200 response, so they pass today with a 500 — but the semantically correct response is 400.
How can I resolve this? If you propose a fix, please make it concise.|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
|
||
| // Beam search parameters | ||
| const ( | ||
| BeamWidth = 5000 |
There was a problem hiding this comment.
@jloh02 I doubled the beam width from 2500 to 5000 now for deeper and more accurate searches. This is the only other logical change in the code besides adding back the slots with no venue info.
There was a problem hiding this comment.
Sorry I'm not super well versed. Does this potentially lead to longer runtimes?
|
@jloh02 wondering if the vercel preview for the website still works? I wanted to see if there were any performance improvements for optimising large module count (10 mods) timetables from the one in prod. |
@thejus03 mybad i broke it partially in #4350 #4328. will try to push a fix out in the next 1-2 days |
jloh02
left a comment
There was a problem hiding this comment.
changes make sense to me. will approve on resolve of my 1 comment. sorry facing a financial crunch a lil
|
|
||
| // Beam search parameters | ||
| const ( | ||
| BeamWidth = 5000 |
There was a problem hiding this comment.
Sorry I'm not super well versed. Does this potentially lead to longer runtimes?
|
My bad I should have expanded on that. Yes it does increase runtime because we are searching through double the amount of timetables as before. But overall I think latency should have decreased from the one in prod now because I reduced latency by more than half from before. So we can afford to search deeper? We can test out how fast it is compared to prod using the vercel preview link. Also the reason why I doubled it is because for one of my friend's 10 mod timetable it was not returning a completely optimised timetable (sometimes) because we didn't search deep enough. But after modifying, it was able to find it every time. 
Prod
Current PR
|
Just confirming but we are using Vercel's free tier right? |
Context
#4339
Summary
Logic Changes
Bug Fixes
Weeksfield (_modules/modules.go,_solver/solver.go): Added comma-ok checks beforeweek.(float64)assertions to prevent panics on malformed NUSMods API dataGetModuleData(_client/client.go): Non-200 responses from the NUSMods API now return a descriptive error instead of silently passing bad data downstreamoptimise.go): JSON decode errors now return a generic"Invalid request format"message instead of exposing internal error detailsInput Validation
_models/models.go):ParseOptimiserRequestFieldsnow returns an error if the modules list is empty_models/models.go): Parsed/computed fields (EarliestMin,LatestMin, etc.) annotated withjson:"-"to exclude them from JSON decode/encodeCode Quality
_constants/constants.go):BeamWidth = 2500,BranchingFactor = 100,DaysPerWeek = 6— all usages in_solver/solver.goupdated_solver/solver.go): Inner loop variableirenamed toslotIdxto avoid shadowing the outer loop indexserializeLessonIndicessimplified (_solver/nusmods_link.go): Replaced a convolutedfmt.Sprint+strings.Fields+strings.Trimpipeline with a straightforwardstrconv.Itoaloop; also addsstrconvimportHandlerdoc comment fixed (optimise.go): Blank line between comment blocks causedreviveto report a malformed doc comment — merged into a single contiguous blockscoreConsecutiveHoursOfStudy), block-to-line comment conversion, duplicate map construction removed, recordings format standardised toMODULE|LessonTypepipe separatorTests
TestOptimiser_EmptyModules— validates the new empty-modules guard returns a non-200 responseTestOptimiser_InvalidTimeFormat— validates malformed time strings are rejectedTestOptimiser_NonExistentModule— validates unknown module codes are rejected (exercises the HTTP status check inGetModuleData)TestOptimiser_MethodNotAllowed— validates GET requests return 405TestOptimiser_ShareableLinks— verifies both shareable links are well-formed URLs containing all requested module codesTestOptimiser_AllSlotsHaveAssignments— internal consistency check that every slot inDaySlotshas a corresponding entry inAssignmentsDocumentation
_directory prefix convention (Vercel serverless requirement)shareableLinkvsdefaultShareableLinkexplained)venues.jsonembed, deduplication logic)Test plan
pnpm start:optimiser, thengo test ./_test/... -v🤖 Generated with Claude Code