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

Add: Negative sign if count is negative #7

Merged
merged 1 commit into from Jan 5, 2020
Merged

Conversation

@BKSpurgeon
Copy link
Contributor

BKSpurgeon commented Jan 1, 2020

Solution for #3

@brandly

This comment has been minimized.

Copy link
Owner

brandly commented Jan 2, 2020

this isn't quite the solution we want. on the top level, we don't want this leading blank spot:

Screen Shot 2020-01-01 at 9 52 38 PM

we just want 040 as before, except we'd like to also display a negative sign when needed. in the path of code that builds up the bitmaps for this display, you're currently waiting til the last moment and then tacking on a - or a . instead, let's catch the problem earlier.

you'll find some code like this.

str =
    minLen 3 (String.fromInt n)

String.fromInt does more than we're giving it credit for. it already gives us what we need.

String.fromInt -4 == "-4`

so what happens to that -? well, we feed these characters thru toInt, which is defined as follows.

toInt string =
    case String.toInt string of
        Just num ->
            num

        Nothing ->
            0

String.toInt "-" will fail to parse, so the - is abandoned and a 0 is left in its place. maybe you could intervene more directly in this chain of code that goes from an Int to a List (Element msg) called children.


we're currently going from an Int like -4 to a String "-4", parsing those substrings like "4" back into an Int 4, and then finding the appropriate Bitmap.

instead of forInt maybe we have a forChar : Char -> Element msg. then, we should be able to generate the String we want to show, map over its Chars, and find an appropriate Bitmap for each.

make sense?

p.s. run npm run format keep your code consistently formatted. you can most likely integrate elm-format into your text editor, which i really enjoy having.

|> List.map (toInt >> Bitmap.forInt >> digit >> (\c -> c [] [])))

addNumericalSign =
digit addPositiveOrNegativeSign |> (\c -> c [] [])

This comment has been minimized.

Copy link
@brandly

brandly Jan 2, 2020

Owner

just wanted to explain something here.

you copied (\c -> c [] []) from above. in the above case, it's a list of things, so we're mapping them into this function. but in this case, you just have a single item.

  addNumericalSign =
-     digit addPositiveOrNegativeSign |> (\c -> c [] [])
+     (digit addPositiveOrNegativeSign) [] []
@BKSpurgeon

This comment has been minimized.

Copy link
Contributor Author

BKSpurgeon commented Jan 2, 2020

Please consider the above patch.

image

As you can see: -2 translates to "0-2".

Bitmap.forInt is now translated to Bitmap.forChar.

@brandly

This comment has been minimized.

Copy link
Owner

brandly commented Jan 2, 2020

forChar looks good!

We’re splitting a string into a list of strings and then toChar is doing some work to get a Char. I think we could use this instead! https://package.elm-lang.org/packages/elm/core/latest/String#toList

@BKSpurgeon

This comment has been minimized.

Copy link
Contributor Author

BKSpurgeon commented Jan 3, 2020

single_digits

This is the output so far using toList. notice how there are only one/two/three digits shown as required. Should this be the desired behaviour, or should it be padded with some zeros?

@brandly

This comment has been minimized.

Copy link
Owner

brandly commented Jan 3, 2020

minLen was handling that. Are you still using it?

@BKSpurgeon

This comment has been minimized.

Copy link
Contributor Author

BKSpurgeon commented Jan 3, 2020

Try the lastest one now: I reverted the minLen change.

If you're happy I'll squash those commits and overwrite this branch by force.

@brandly

This comment has been minimized.

Copy link
Owner

brandly commented Jan 3, 2020

this is really nice! i'm glad we simplified some old code and improved functionality.

let me know when you're ready to merge

Fix: add a negative sign if the number of bombs placed are greater than then number existing

Fix: minesweeper.png to better display positive and negative signs

Fix:    display of negative signs

Formatting

Fix: to use String.toList

formatting
@BKSpurgeon BKSpurgeon force-pushed the BKSpurgeon:master branch from fcebdb2 to 06efad5 Jan 5, 2020
@BKSpurgeon

This comment has been minimized.

Copy link
Contributor Author

BKSpurgeon commented Jan 5, 2020

Hi Matt

I have squashed into a single commit. You should see it updated above.

regards

Ben

@brandly brandly merged commit 943ca2e into brandly:master Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.