Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ttt with a cli #2

Open
wants to merge 37 commits into
base: main
Choose a base branch
from
Open

ttt with a cli #2

wants to merge 37 commits into from

Conversation

liza-pizza
Copy link
Contributor

@liza-pizza liza-pizza commented Aug 31, 2022

Added Cli to tic tac toe.

@liza-pizza liza-pizza changed the title WIP PR of ttt with a cli WIP ttt with a cli Aug 31, 2022
@liza-pizza liza-pizza changed the title WIP ttt with a cli ttt with a cli Sep 2, 2022
@nid90
Copy link

nid90 commented Sep 2, 2022

A general observation: add a bit more words to the commit messages. it is not just about adding or updating files or fns, it is about giving a brief but thorough description of the changes going into that commit.

ex: add console is quite un-descriptive, I would reword that as add a console namespace to tic tac toe to add a CLI to the game, or something to that effect.

this has some good ideas: https://initialcommit.com/blog/git-commit-messages-best-practices

@@ -0,0 +1,46 @@
# Tic Tac Toe

Choose a reason for hiding this comment

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

suggestion: Try to think of the README as the first thing a potential user might be looking at. What would that person want to see here?

Some potential questions:

  • What does this repo do?
  • How do I set this up in my machine?
  • How do I test this out?
  • How to I play the game?

Thinking from this perspective will help pen down a more thoughtful README.


(def valid-game-pieces-set #{:o :x :e})

(def game-pieces-set #{:o :x})

Choose a reason for hiding this comment

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

question: Should we consider :x and :o as players rather than pieces? Even the wiki thinks of it that way. What do you think?

(assoc-in board coordinate game-piece)))


(defn winner-of-collection

Choose a reason for hiding this comment

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

question: What does collection refer to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the function can find the winner of any collection as opposed to a 3x3 board. Used the word collection to generalize

@@ -0,0 +1,30 @@
(ns ttt.user-input

Choose a reason for hiding this comment

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

suggestion/question: What do you think about naming this layer cli instead of user-input? The functions here are very specific to parsing user input from the command line. We might have an entirely different set of validations for user input if we were to create a web UI interface for the game, and so this namespace might not entirely make sense at that point. It's a stretch of course (since we don't plan on building a web UI), but still something to think about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand, the user-input namespace is never reading directly from the cli. This particular namespace has no idea that a cli exists, it's just parsing input that is not necessarily from the command line. Thats why I think naming it user-input works better.

Comment on lines 54 to 56
(str
(apply str "\n"
(interpose " " row))))))

Choose a reason for hiding this comment

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

nitpick: Using a thread last macro might make the section more readable:

(->>  row
     (interpose " ")
     (apply str "\n")
     str)))

Also, do we need to str at the end if we're doing apply str in the previous step?

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

Successfully merging this pull request may close these issues.

3 participants