-
Notifications
You must be signed in to change notification settings - Fork 24
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
Another batch of improvements :) #9
Conversation
…P oriented and more typesafe.
Remove PlayerColor
if (neighborString.get.equals(goString)) | ||
this.replaceString(neighborString.get.withLiberty(Point(point._1, point._2))) | ||
goString.stones.foreach { point => | ||
neighborMap((point._1, point._1)).foreach { neighbor => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it normal that we pass (point._1, point._1)
to neighborMap
here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
omg, no! good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like me to fix this or you'll fix it ?
…has a better proposition
var numWhiteTerritory = 0 | ||
var numDame = 0 | ||
|
||
for ((_, status) <- territoryMap) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI @guizmaii the best way here would be to import cats: territoryMap.values.foldMap(t => Map(t -> 1))
(it uses the Int
Monoid
and return a Map[Type, Int]
), and probably use cats in more places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guizmaii @sderosiaux let's leave cats out, please. You might see this differently, but it's often the beginning of the end of a project's readability :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure @maxpumperla, no worries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sderosiaux thanks ! I didn't know that :) but as requested, I'll not add Cats ;)
@guizmaii assuming you continue your refactoring odyssee, would you mind branching off of current master? 👍 |
@maxpumperla It's my last PR. I'll not have any more time to spend on this project until a long time. I have a lot more work to do on other projects. I hope that these PRs were useful to you. If you have any question on Scala or FP you can always contact me on Twitter. It was fun to work on your project. I don't know the Go game and discovered some rules by reading the code. 🙂 Again, I don't want to be cocky but here some advices about your use of Scala:
You can always declare "value classes" (https://docs.scala-lang.org/overviews/core/value-classes.html) to express the exact domain of your program.
Because they are value classes, they don't bring runtime overhead but only brings you type safety and accuracy when you code. Maybe you already know all that, I don't know. I talk only on what I saw in your code. Thanks for the time you spent reviewing my PRs 🙂 |
if (allNeighborsInDanger) return true | ||
false | ||
|
||
friendlyStrings.forall(_.numLiberties == 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be improved:
! friendlyStrings.exists(_.numLiberties != 1)
And @sderosiaux worked on simplifying your
GameResult.collectRegion
function.He'll push you a PR too ;)
(as usual, can you please squash my commits ? thanks 🙂)