Conversation
spaghetti-squash
left a comment
spaghetti-squash
left a comment
There was a problem hiding this comment.
The comments and docstrings here are an excellent example of how comments and docstrings are the worst thing to use LLMs for: they're excessively verbose, spend too much time elucidating simple concepts while eliding complex ones, and are somehow too dense to plod through while also containing remarkably little information.
| } as const; | ||
|
|
||
| type Pitch = keyof typeof PITCH_ELEMENTS; | ||
| type Element = (typeof PITCH_ELEMENTS)[Pitch]; |
There was a problem hiding this comment.
I would be pretty surprised to discover that this is the only notion of an element string type that exists or should-exist in libram. This type should probably be defined in lib and exported, if it doesn't already exist.
There was a problem hiding this comment.
I believe kolmafia's ElementType includes the fake-elements like Slime and Bad Spelling; now that we know they are for sure not real elements in kol, it may be sensible to retire them. cc @gausie for feedback. I suspect the answer will be "slime as a real element has nonzero utility due to slime resistance, and maybe supercold has something similar?", which I suppose would be enough to put a damper on that wild dream.
There was a problem hiding this comment.
The actual element doesn't matter; but I can import and use $element<element> instead of just the string if you'd prefer. I cannot stress enough, the actual element doesn't matter.
| const PITCHES_BY_ELEMENT: Record<Element, [Pitch, Pitch, Pitch]> = { | ||
| Hot: ["Throw Some Smoke", "Bring the Heat", "Schenectady Scorcher"], | ||
| Cold: ["Deep Freeze", "Snowball", "Ice Him Out"], | ||
| Spooky: ["Ghost Pitch", "Skullball", "Non-Euclidean Curveball"], | ||
| Stench: ["Garbageball", "Beanball", "Some Cheddar"], | ||
| Sleaze: ["Slurveball", "Bacon-Wrapped Slider", "Screwball"], | ||
| }; |
There was a problem hiding this comment.
Don't redundantly construct both objects, one should be constructed from the other using either a reduce or an Object.fromEntries
| * The first two entries are lesser pitches. The third entry is the greater | ||
| * pitch. | ||
| */ | ||
| const PITCHES_BY_ELEMENT: Record<Element, [Pitch, Pitch, Pitch]> = { |
There was a problem hiding this comment.
use the existing Tuple type in libram, rather than repeating Pitch thrice
| const PITCH_ELEMENTS = { | ||
| "Throw Some Smoke": "Hot", | ||
| "Bring the Heat": "Hot", | ||
| "Schenectady Scorcher": "Hot", | ||
|
|
||
| "Deep Freeze": "Cold", | ||
| Snowball: "Cold", | ||
| "Ice Him Out": "Cold", | ||
|
|
||
| "Ghost Pitch": "Spooky", | ||
| Skullball: "Spooky", | ||
| "Non-Euclidean Curveball": "Spooky", | ||
|
|
||
| Garbageball: "Stench", | ||
| Beanball: "Stench", | ||
| "Some Cheddar": "Stench", | ||
|
|
||
| Slurveball: "Sleaze", | ||
| "Bacon-Wrapped Slider": "Sleaze", | ||
| Screwball: "Sleaze", | ||
| } as const; |
There was a problem hiding this comment.
this should probably be the one that gets programmatically defined.
| monster1: Monster, | ||
| pitch1: Pitch, | ||
| monster2?: Monster, | ||
| pitch2?: Pitch, | ||
| monster3?: Monster, | ||
| pitch3?: Pitch, |
There was a problem hiding this comment.
I would make this accept a spread tuple of length >= 3 of { monster, pitch } objects--alternating arguments is just going to lead to more confusion, and requires a bizarre input validation rather than relying on TypeScript to do it for you.
| let pitch: Pitch; | ||
|
|
||
| if (state[index] < 2) { | ||
| pitch = PITCHES_BY_ELEMENT[element][0]; |
There was a problem hiding this comment.
Upon seeing PITCHES_BY_ELEMENT in action, I think it would probably make more sense to have two separate object, one for major pitches, one for minor; I didn't assume this on first reading the code, but I do now.
| monster1: Monster, | ||
| pitch1: Pitch, | ||
| monster2?: Monster, | ||
| pitch2?: Pitch, | ||
| monster3?: Monster, | ||
| pitch3?: Pitch, |
There was a problem hiding this comment.
same remark re: arg sequencing
| return false; | ||
| } | ||
|
|
||
| visitUrl( |
There was a problem hiding this comment.
use the directlyUse utility here
|
|
||
| const lineup = get("baseballTeam").split(",").map(Number); | ||
|
|
||
| if (lineup.length !== 9) { |
There was a problem hiding this comment.
This check is less computationally expensive and more "base" than doing findPitchOrder; it should be the first reason to return false
| const currentMonsterId = lineup[position]; | ||
|
|
||
| // Try matching an requested pitch if the monster at this index matches | ||
| for (let i = 0; i < unmatchedRequests.length; i++) { |
There was a problem hiding this comment.
iterating over the indices of an array isn't an antipattern in typescript, but you should in general view it as a sign that you may be on the wrong path. It seems like the only place where we use it is in filtering unmatchedRequests, which IS the array you're iterating over.
I think that probably this entire block can be replaced with some amount of map/reduce/flatMap use, be condensed significantly, and in doing so be made more legible. I can't discern exactly what this is trying to do easily enough to provide more specific guidance right now, although maybe after some changes I'll be able to.
2 participants
No description provided.