Conversation
| this.#redis = redis; | ||
| this.#challengeToWordService = new ChallengeToWordService(redis); | ||
| this.#challengeToPostService = new ChallengeToPostService(redis); | ||
| this.#mode = mode; |
There was a problem hiding this comment.
thought: It feels odd for the service to be instantiated to only handle one mode type. I would have imagined the mode would be passed into the method calls.
There was a problem hiding this comment.
I disagree - though perhaps the word "service" here muddles things - I don't think it's a particularly good name tbh. Maybe ChallengeManager, WordListManager etc better conveys the purpose of these classes? Happy to change them if you think that's better.
Generally I expect the pattern to be something like this:
Devvit.addTrigger(
{
// Get the mode in whatever way
mode = getMode()
myService = new ChallengeService(redis, mode);
// do a bunch of things with the service.
}
)
Further, most (but not all) of the time, a post only exists with the context of one GameMode - making that a parameter implies that it might change, when in practice one instance will only ever deal with one mode.
cytommi
left a comment
There was a problem hiding this comment.
non-blocking suggestions
| private redis: RedisClient, | ||
| private mode: Mode |
There was a problem hiding this comment.
suggestion: prefer using # prefix for private properties. Also suggest prefixing with readonly keyword if we don't anticipate modifications to the property
There was a problem hiding this comment.
Ack - this was already discussed the prior PR. You can't do that with shorthand constructors.
| #mode: Mode; | ||
|
|
||
| constructor(redis: RedisClient) { | ||
| constructor(redis: RedisClient, mode: Mode) { |
There was a problem hiding this comment.
suggestion: could default mode to regular mode by using a default value
| constructor(redis: RedisClient, mode: Mode) { | |
| constructor(redis: RedisClient, mode: Mode = 'regular') { |
There was a problem hiding this comment.
I think doing this is extremely error prone and I want to explain why:
Every time ChallengeService is used - it needs to know whether it's in the context of regular or hardcore, and if that's ever determined incorrectly, it will result in bugs. Making this default to "regular" has the slight advantage of making it so this particular PR can be a little smaller - I won't need to update every instantiation. However - every instantiation does need to be updated before we ship - each and every one needs to do something to determine whether it's in the context of hardcore or in the context of regular - and changing them all now forces me to add TODO's for each of them to burn down in the future. I didn't want to do that in this PR because that's a harder thing to do (as each instantiation will need to be handled slightly differently) - but I really don't want mode to have a default long term so adding it in the immediate term doesn't seem like the right idea.
💸 TL;DR
Parameterize all the things using "mode" - defaulted to regular. Will be fixing each of those in the future.
Also fix issues with playtesting.