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

Windowing System: Improvement Ideas #1600

Open
junedev opened this issue Jan 1, 2022 · 4 comments
Open

Windowing System: Improvement Ideas #1600

junedev opened this issue Jan 1, 2022 · 4 comments
Assignees

Comments

@junedev
Copy link
Member

junedev commented Jan 1, 2022

After seeing a couple of solutions to "Windowing System" during mentoring, I think the exercise could need some changes to better achieve it's learning objectives.

  • For Position and Size, the constructor and the method have the same implementation. I saw a solution that used the method in the constructor which is not a very typical example.
  • There is no reason to use the methods to update values so students often just set the property values directly.
  • The calculation for the window resize and move is rather involved which does not add value for the actual learning objectives but it seems students spend quite some time finding creative solutions there, e.g.
    image

Some ideas how to improve the exercise:

  • Maybe we should expect some/all properties to be marked as private with the underscore to discourage the direct assignment. That might also make it easier to pick up unwanted code with the analyzer later. (Not sure how to best test in this case though. Either the test disrespects the underscore which is not a good example or the test needs to fall back on some user-implemented serialization function. But if that function is wrong all the tests will fail which is also annoying as everything else the user did could be correct.)
  • The methods on Position and Size could change the values by some amount instead of just setting a value. That would make them distinct from the constructor. (Probably only needed if we stick with "public properties".)
  • Simplify the window resize/move method e.g. by saying some lower bound is always respected
@SleeplessByte
Copy link
Member

(Maybe related: https://javascript.info/private-protected-properties-methods)

Since this is a concept exercise, we can add some tests to remove some of this behaviour, if you'd like. Technically we could also start pushing the #private syntax (even though I hate it) given its current stage in the TC39 process.

@junedev
Copy link
Member Author

junedev commented Jan 3, 2022

I read a lot of negative posts about the real private properties lately so I don't feel like pushing them either. Also I prefer to stick to stage 4 as much as possible in the concepts.

@junedev
Copy link
Member Author

junedev commented Jan 15, 2022

I would love to fix the exercise but I am not sure how to best approach this. I don't think it would be good to just make the tests enforce the use of the methods instead of direct assignment if there is no actual difference. It makes it very hard to argue the case in mentoring. I would prefer if it would be clear from the type of the problem that the method needs to be used, that some property needs to be "private", etc.

I am happy to make bigger changes to the tasks and story if someone has an idea how to construct the exercise in a way that a desired solution comes more naturally.

Here the minimum I would like to get out of the exercise:

  • The student should create multiple entities (classes) so we can make them use both syntax variants and demonstrate they interoperate.
  • The student should define some fields and at least one method for each of the entities
  • The student should make use of the methods defined (it should make sense to use the method instead of assigning directly)
  • It would be good to demonstrate how to built up some more complex object like the window in this case from other smaller entities that classes themselves (like Position and Size)

The current instructions can be found here: https://exercism.org/tracks/javascript/exercises/windowing-system

Can anyone help with this? I have never worked in a real OOP style code base so it is hard for me to construct a sensible example here.

@SleeplessByte
Copy link
Member

SleeplessByte commented Feb 7, 2022

Ok. So. I solved the exercise @junedev , without looking at the exemplar, but purely reading the introduction and then following the instructions. This is what I came up with: https://exercism.org/tracks/javascript/exercises/windowing-system/solutions/SleeplessByte.

It is pretty close to the exemplar, so I think that if you know what you're doing, it's not a convoluted exercise at all. I do think the weakness in the exercises are as follows:

  • There seems to be little reason for Position and Size to exist, because you'll always need to violate the Law of Demeter at the moment to get the properties you need. In my opinion, I would expect x, y, width and height to be either properties or methods of a ProgramWindow, despite them perhaps being backed by a Position or Size class. I have worked with some game engines that definitely do it like this, it's just that I would likely not write it like this myself.
  • People struggling with what is effectively Math.clamp is not the fault of the exercise. I would recommend that we try to solve the issues at hand, but don't explicitly try to solve this. It does detract from the exercise, so perhaps we can drop it.
  • The power of prototypes/classes isn't shown at all, but I have an idea for this.
  • I don't think the problem is that the shape of Position and Size is the same, but you are effectively asked to do the same thing twice, with the only difference being a limit of 1 instead of 0.
  • However, the duplication of the naming (resize / move) between the ProgramWindow and the "helper" classes is annoying and makes the helpers pretty much "why are they there", because they don't do much.
  • There's a test missing that catches a naïve implementation of changeWindow (see my solution and try to figure out why I do what I do).
  • I would expect more methods/properties on the classes to further solidify why they exist. Let me think about it. Things like "fullScreen()" and "isFullScreen()"?

I have a few thoughts right now, but I will likely have more later this week. Most importantly, whatever we do to resolve your list of issues, I think we should not introduce inheritance, because we do not need it to solve these issues.

To show the power of the prototype we can introduce a story element that's something like:

You're building a modding system for the windows. The modding system will, when
activated, change _all_ existing windows and _future_ windows by adding 
sparkles ✨. Implement a function `activateSparkles` that:

- Defines a property on all program windows called `sparkling` and set it to true.
- Defines a method on all program windows called `safeForWork()` which turns off 
  the sparkles and resizes the window to full screen.

This will force the student to use ProgramWindow.prototype:

export function activateSparkles() {
  ProgramWindow.prototype.sparkles = true

  ProgramWindow.prototype.safeForWork = function safeForWork() {
    this.sparkles = false
 
    this.move(new Position())
    this.resize(this.screenSize)
  }
}

We would test that a window created in the test is also affected when calling activateSparkles.

// Initially windows are unmodded
const testWindow = new ProgramWindow()
expect(testWindow.sparkles).toBeUndefined()

// When modded, all windows, including already existing ones become sparkling
activateSparkles()
expect(testWindow.sparkles).toBeTrue()

// Open a brand new window
const brandNewWindow = new ProgramWindow()
brandNewWindow.move(200, 200)
brandNewWindow.resize(300, 300)

expect(brandNewWindow.sparkles).toBeTrue()

// Make it safe for work
brandNewWindow.safeForWork()

expect(brandNewWindow .sparkles).toBeFalse()
expect(brandNewWindow.size.width).toBe(800)
expect(brandNewWindow.size.height).toBe(600)

// The old window is still sparkling
expect(testWindow.sparkles).toBeTrue()

Note that I kept the exercise intact, just to show how it would be done in this version of the exercise.

We can further show the power of instance vs prototype separation by adding "bugfixes" to the story.

There is a window that misbehaves. When it is shown full screen, it actually only
fills half the screen, because of a drawing mistake. 

Write a function `patchSize(buggedWindow)` that will always double the width
of the `buggedWindow`, without affecting any other windows.

Depending on how we change the exercise, the student will need to overwrite the function on the instance / overwrite the property on the instance.

(A follow up excercise that deals with getters/setters etc. could then guard against this with Object.defineProperty and making a property non-configurable, or stuff like that. Cool shit).

These are my thoughts thus far. Sidenote: if we want people to not use properties, then we should have side-effects that are repeated, so you'll want to use the method.

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

No branches or pull requests

2 participants