Code review request

[Thomas Ballinger]

Hi Elm folks! I've enjoyed reading this list for a bit. I've written my first Elm thing over the last couple weeks and would love to hear any kind of feedback on it. It's an unfinished game jam piece I kept running with so the title doesn't make sense.

code: https://github.com/thomasballinger/loveinthetimeoftetris

live: love.ballingt.com (takes about 70 seconds to play all of)

I was going to clean things up the way I know how, but I need to take a break to get some other things done and I thought I'd learn more by asking how someone else might clean it up. Please don't assume I know what I'm doing in the slightest :)

Any feedback would be great, but if prompts are helpful:

* what does this code make it look like I'm missing about Elm?

* what do you think of the extensible record type aliases? I think the way I've used them is mostly terrible, I designed them up front instead of letting them evolve.

* code style?

* I'm using an elm autoformatter, how's my formatting? Is this style common?

* I don't think I'll be using evancz/elm-graphics in the future since I'll be doing less gamey stuff or want to work with canvas more directly. How is this usually done?

* I abandoned elm reactor once I started embedding in html, is that a viable workflow I should have stuck with for longer?

* I was tempted to start a utils file or look for an external lib but was trying to focus on learning the stdlib. Are there pretty common util libs folks use? I sure missed some list functions.

* I escaped to JavaScript anytime I thought it would be hard to do something with the stdlib, presumably it would be nice to use Elm for some of these things?

Thanks so much, and feel free to contact off list if you prefer at m...@ballingt.com - if you do I'll report back what I learned to the list.

[Will White]

Hi Thomas!

I've played the game and I'd like to give you my thoughts on the UX. I may be able to review the code later. I wish I'd recorded my thoughts as I played.

I ran right (it's Mario), bumped into a red wall. Ran left, same. What do I do? Ran right, green block has appeared. Jump over it. Oh, it's a Tetris block. Oh, the grey is where the block's coming down. 1 I get squashed and it bugs out (screenshot). I think I'm able to play on (arena not reset). "Tetris controls IJKL" Oh cool, I can control the Tetris blocks too!

Having a zoomed out view to start with (and then zooming in) would have got rid of all the thoughts up to 1. Knowing the Tetris controls earlier would have been fairer.

Cool idea!

[Will White]

Screenshot

[John Bugner]

I just browsed around a bit, and noticed the following things:

(1) Put your code in a source folder. Having all your elm files in the same folder as elm-package.json, .gitignore, index.html, etc is distracting. To do this, add your source folder to the "source-directories" property in elm-package.json, like this:

currently:

    source-directories": [

        "."

    ],

change it to:

    "source-directories": [

        "src"

    ],

(2) Auto-formatter? I don't like this one. It needlessly deeply indents things, like the 'rotations'' function here: https://github.com/thomasballinger/loveinthetimeoftetris/blob/master/Piece.elm#L172 I prefer to put 'case ... of' on one line, the same line as the '=' it follows, because it's usually short. Too much indentation makes code unreadable. I think the official coding style (unfortunately) promotes this though...

(3) 'rotate' looks very inefficient: https://github.com/thomasballinger/loveinthetimeoftetris/blob/master/Piece.elm#L172 It's just a bunch of 'if's. Why not turn 'Piece' into an enum type, and then have 'rotate' pattern match the enum? ... The texture property looks like it should just be a wrapper: 'type Textured a = Textured Int a'. ... Using an 'Int' as an index for some list/array (I assume that's how it's working.) is very C-like. Unless there's a nearly unlimited amount of textures (one for each integer value), use an enumeration with a fixed amount of constructors instead. To me, a type represents not only the shape of its underlying data, *but also its **semantics** *. (I admit, Elm doesn't fully embrace this philosophy yet, (partly because nearly all the languages that have come before it don't either, so people still have trouble thinking outside the C-like imperative-OO box), but I think it's the way forward, because when it's paired with a strong type system like Haskell's or Elm's, it prevents many logic errors, like adding 1 meter to 2 feet and getting 3 feet, or 4 ticks to 5 seconds and getting 9 seconds, or 7 apples to 8 oranges and getting 15 oranges, etc.)

(4) You should always give a function a type annotation: https://github.com/thomasballinger/loveinthetimeoftetris/blob/master/Progression.elm#L23 . 

>* I don't think I'll be using evancz/elm-graphics in the future since I'll be doing less gamey stuff or want to work with canvas more directly. How is this usually done?

By using Svg instead: http://package.elm-lang.org/packages/elm-lang/svg/1.1.1

[Thomas Ballinger]

Thanks so much for taking a look! I've fixed (a likely cause of) the collision bug. It's fun to hear about your experience playing, despite the current lack of game experience. I'm a fan of the "ooooh it's tetris" realization (and ideally would like to time the music's transition to the tetris theme to this moment) so don't want to start out zoomed out, but plan to put off decisions about that until there's some gameplay. Let me know if you get a chance to look at the code.

[Nick H]

Couple of things I noticed:

You are handling window resizing with a port. Elm has a Window module that can handle this for you.

Your Util.range can be replaced by built in syntax [ 0 .. max ]

For list utilities, I highly recommend looking at elm-community/list-extra. Similarly, elm-community has dict-extra, maybe-extra, and a whole lot more. Just search the package manager for "extra" and you will find a lot of stuff!
 

As far as formatting goes, yours looks pretty standard. Were you using elm-format?

Don't feel bad about abandoning the reactor. Evan has said it's going to be replaced with a "debug" flag in the compiler. That will allow some of the nice reactor features to be used in programs that are embedded in HTML.

--
You received this message because you are subscribed to the Google Groups "Elm Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elm-discuss+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Nick H]

(Well, I don't know if the reactor will be replaced, but a debug mode will be added, and it sounds like it will be much more useful for serious projects.)

[Nick H]

Regarding the extensible records... Movable (Standable (Collidable (Drawable {}))) sure is a mouthful isn't it? :-)  It looks like every thing in your game is the union of all four type aliases... so why not just make it one type alias?

OK, one last thing. This type definition...

type PossibleCollision = NoCollision | Collision CollisionType Float (Float, Float)

I think the more idiomatic thing to do (but this might just be personal taste) would be to define

type alias Collision =
    { collisionType : CollisionType

    , overlap : Float

    , velocity : Float
    }

And then in the places where you are using "PossibleCollision," use "Maybe Collision" instead. The Maybe is super useful for dealing with cases where you have either something or nothing. I bet that you could shorten your collision detection code a lot by switching to using Maybes. (For instance, you could use List.filterMap).

[Thomas Ballinger]

Thanks very much John. The auto-formatter I'm using is https://github.com/avh4/elm-format, I also found it to be a bit much. I'm a big fan of automatic formatting so might look at changing these settings in elm-format, or if anyone knows of other autoformatters please let me know.

While adding type annotations I encountered something I didn't understand that I worked around at https://github.com/thomasballinger/loveinthetimeoftetris/blob/3f968afad490ebab54b4f0c3bafdf45b779ebc4b/src/Main.elm#L278, I tried to simplify the behavior and got it down to this https://gist.github.com/thomasballinger/a0d8b38fa7186ee2e608d4772f2ebe7e which I'd appreciate a hand from anyone in understanding.

I did the rest of these except for switching to SVG, but I'm looking forward to that too.

[Thomas Ballinger]

Thanks very much for all this Nick!

Re Maybe use, good call

Re List methods: I had seen elm-community/list-extra but wanted to make sure I wasn't missing anything in the stdlib first. Great to hear that's pretty normal to use.

 

Re type alias + maybe instead of a new type, that seems pretty reasonable.

Re: extensible records, I certainly concede they're not paying off here. I am using different combinations a bit, but I don't need so many. I also ran into some type annotating behavior that confuses me with extensible types, see https://gist.github.com/thomasballinger/a0d8b38fa7186ee2e608d4772f2ebe7e (also described in a sibling thread)

To unsubscribe from this group and stop receiving emails from it, send an email to elm-discuss...@googlegroups.com.

[John Bugner]

>While adding type annotations I encountered something I didn't understand that I worked around at https://github.com/thomasballinger/loveinthetimeoftetris/blob/3f968afad490ebab54b4f0c3bafdf45b779ebc4b/src/Main.elm#L278, I tried to simplify the behavior and got it down to this https://gist.github.com/thomasballinger/a0d8b38fa7186ee2e608d4772f2ebe7e which I'd appreciate a hand from anyone in understanding.

I haven't used extensible records very much, but I think it's because the signatures don't match.

The first is:

>hasBothXAndY : HasXAndY (HasX a)

but the other is:

>fieldOfBothTypes : HasX (HasXAndY {})
The order matters.

(Btw, the (<|) function is the opposite of (|>). I personally use only the former, because it does things in the same order that you normally write a function. The latter makes things look "backwards".)

[Joey Eremondi]

Thanks very much John. The auto-formatter I'm using is https://github.com/avh4/elm-format, I also found it to be a bit much. I'm a big fan of automatic formatting so might look at changing these settings in elm-format, or if anyone knows of other autoformatters please let me know.

That's the official formatter, so you're definitely doing the right thing by using it. Nobody can agree on the settings they like best, but for standardization, a deliberate choice has been made not to allow users to tweak the settings of elm-format.

I think most people on this mailing list would advocate that you use it, to maximize the uniformity and readability of your code, even if it does odd things with indentation.

--

You received this message because you are subscribed to the Google Groups "Elm Discuss" group.

To unsubscribe from this group and stop receiving emails from it, send an email to elm-discuss+unsubscribe@googlegroups.com.

[Nick H]

(Btw, the (<|) function is the opposite of (|>). I personally use only the former, because it does things in the same order that you normally write a function. The latter makes things look "backwards".)

I personally use only the latter, because it works like the pipe operator in bash :-)

On Thu, Aug 18, 2016 at 8:44 PM, John Bugner <john....@gmail.com> wrote:

--

[Thomas Ballinger]

I can see experimentally this is the case, but would someone mind spelling this out a bit for me? I think I don't understand

* why the order of lines 55/56 matter

* how HasX (HasXAndY {}) and HasXAndY (Has X a) could behave differently

* what the error message is trying to say; what is the difference between HasX { ..., x : ... } and HasX {...}

but those may be the wrong questions.

[Thomas Ballinger]

I thought I'd bump this, I'd appreciate a pointer of something to read or an example that might clarify things.

[Janis Voigtländer]

It’s some form of compiler bug. If in your gist, you replace the output type HasX a by { a | x : Float }, which is its definition, then the gist will suddenly compile. So I think you should try to bring this example down to a minimal case and then open a bug report at https://github.com/elm-lang/elm-compiler/issues (after you have checked there isn’t an issue covering this alreay).

About some of your specific questions:

• how HasX (HasXAndY {}) and HasXAndY (Has X a) could behave differently

They are different types. The first one is a record type that contains two (!) fields x (of which only one is accessible) and one field y. The second one is a record type that contains those two/three plus whatever fields the record type has to which a will be instantiated.

• what the error message is trying to say; what is the difference between HasX { …, x : … } and HasX {…}

It’s trying to say that the first of these types has an x field that the second of these types does not have. Likely a consequence of the double presence of x (see above) in one of the two types. Like: HasX { ..., x : ... } has two xs, but HasX {...} has only one.

​

--