Skip to content
This repository has been archived by the owner on Aug 7, 2023. It is now read-only.

Code review #1

Closed
wants to merge 1 commit into from
Closed

Code review #1

wants to merge 1 commit into from

Conversation

isovector
Copy link

Here's how I'd model this. I've only touched the polysemy stuff -- though I'd suggest breaking guessProg into many smaller functions (rather than the big where block).

The behavior is slightly different, in that the random numbers are fetched when they're needed in the IO version, rather than using a precomputed list.

Some guidelines for writing polysemy code:

  • Prefer the built-in effects if they'll get you where you're going
  • Prefer several fine-grained effects that do precisely what you want, rather than god effects that do everything you want. See Trace, Input Int, Input String instead of Guess
  • Don't be afraid to write new interpretations for existing effects!
  • Prefer Input over Reader whenever you don't need local. Likewise, prefer Trace > Output > Writer whenever you can get away with it.
  • The runMonadicX effects let you intersperse streaming pieces of functionality into everyday Inputs and Outputs

let GuessConfig (guessesallowed, (minguess, maxguess)) = config
trace ""
trace $ "I'm thinking of a number between " <> show minguess <> " and " <> show maxguess <> ". Guesses allowed: " <> show guessesallowed
correct <- input

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh, I find this excessively terse. With multiple Inputs in the effect list, I have to manually type check this code to work out what's going on! I think the original was actually easier to read WRT getNextRand.

The Input String and Trace effects are maybe OK, though I'd probably bundle them into a new single effect as you're really thinking about a single coherent entity that you're having a conversation with. You can always reinterpret such an effect into [Input String, Trace] later.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reasonable argument. I was thinking about this after the PR, and instead might suggest some newtypes: newtype Response = Response String, and then Input Response, and then

Response correct <- input

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That clarifies what input is doing, but it still obscures the fact that input and output are correlated as part of a unified conversation (be that with a user over stdin/stdout, or some remote user over sockets). But it's certainly better!

@KerfuffleV2
Copy link
Owner

@isovector I responded on reddit but I'm not sure you saw it. No reply is necessary as I'm sure you're quite busy, I just wanted to make sure you knew I really appreciated the time took to review my little example. I also updated the post and README here with a link to your changes.

Thanks again!

@KerfuffleV2
Copy link
Owner

@isovector Sorry to tag you here after a billion years.

For whatever reason, people are still following/starring this project even though it's sorely out of date and was archived. I've added a warning to the README to let people know it's probably pretty obsolete at this point.

On the off chance you'd be interested, I just want to say that I'd be happy to hand the repo over to you. No response necessary here unless you want to arrange that. I thought you should get the first crack at it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants