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

Don't bind to Message class #87

Open
JakeGinnivan opened this issue Feb 2, 2015 · 7 comments
Open

Don't bind to Message class #87

JakeGinnivan opened this issue Feb 2, 2015 · 7 comments
Labels

Comments

@JakeGinnivan
Copy link
Member

We need to remove mutation from carnacs Rx stream and based on messages (or some intermediate type) the UI can be updated.

This is the next step in cleaning up carnac's codebase.

@RolandPheasant
Copy link

@JakeGinnivan I read your blog post about bringing Rx into this project and I agree having a mutable message passing through the stream is far from ideal and is best avoided.

I have prototyped a working solution where the message is immutable on a standalone project just so I could try out. Also I managed to reduce the amount of code required by a fair degree.

I hope you are happy for me to take ownership of the issue. If so I will try and get it done very soon.

@JakeGinnivan
Copy link
Member Author

Hey @RolandPheasant

Would love to see what you are doing, I know @LeeCampbell has been working on this as well at #91

@RolandPheasant
Copy link

It is a bit radical. The concept is to create an intermediate object to handle the key press collations (as you suggested above). Then each change produces a new message which is a new object - an update which replaces the old item in the collection. This enables me to make KeysController a single line of code. I will try and get it done over the weekend then invite you to look at my branch. if you like it only then will I do a PR

@LeeCampbell
Copy link
Contributor

@RolandPheasant, have you reviewed the #91 Refactor PR? It sounds like you are requesting something very similar to what I have already done. Just waiting on an approver to accept the PR (or reject with reasons!) cough cough.

@RolandPheasant
Copy link

@LeeCampbell just reviewed your code. I like it. Your solution is conceptually 80% the same as mine and I see you have been very thorough and paid excellent attention to detail. The main difference between our ideas is I used immutable intermediate objects to collate / aggregate the key presses leaving the resulting message as a simple state container with no logic.

Also in my PoC solution I added another layer which simplified the KeysController to a single line but that single line does not quite work with your solution. Anyway, before I jump in, I will wait first to see whether your changes are accepted. But whether they are or not I have enjoyed studying your solution.

@JakeGinnivan
Copy link
Member Author

@RolandPheasant have merged @LeeCampbell's PR, feel free to build on that then submit another PR

@LeeCampbell
Copy link
Contributor

Cheers @RolandPheasant
My intention wasn't to create a perfect solution, but to help the guys take a few steps toward more idiomatic Rx. I wanted to get that work in before Jake gives a Presentation on Rx gotchas using Carnac as an example code base.

Most changes are basically my standard old tricks : Immutable where possible, Methods with their data, intention revealing naming...and not passing IObservable<T> into methods ;-)
I am sure that there is still plenty of tweaks that can be applied!

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

No branches or pull requests

3 participants