Code review about inkino HOT 9 OPEN

Wow nice code review indeed.

from inkino.

@anasbud Thanks, glad you like it!

I'll create an issue ticket about parsing in a separate Isolate, and see if it makes sense here. The last time I tried it, which was in January, it seemed that at least parsing JSON was fast enough. IIRC, the overhead of switching from main to background Isolate took longer than parsing the JSON, and there wasn't any UI thread stutter to begin with.

Might be that some small optimizations could add up and in this project, it would actually make a lot of sense, but you never know until you investigate. :)

from inkino.

Thanks for sharing your code ! Amazing project, really !

You should take a look at https://flutter.io/cookbook/networking/background-parsing/, it could be interesting to implement it.

from inkino.

This is by far the most insightful code review I've ever received. Thanks a lot, really!

I'll address those points when I can, most likely tomorrow.

Update: had to do a release finally, incorporated some of those changes before that. Will address all of them when I have the time. Thanks again!

from inkino.

🎉 let me know if there's anything that's not clear.

from inkino.

I'll ask if something comes up! Can I tag you as a reviewer on the upcoming Pull Requests that will address this feedback? Meaning, do you have time / interest in reviewing the changes? :)

from inkino.

Sure, go for it 👍

from inkino.

Agreed. Amazing code review, and really great work! I generally concur with what's been said, and would suggest only minor changes.

Adding further comments:

• "Finally, have you considered organizing the app code by feature/domain" -- I generally agree with this sentiment. The only comment I'd make here: If you ever want to share the Redux logic with an angular app, it can be helpful to keep your "Domain Layer" and "UI Layer" separate. If not, putting everything into a single feature module can be really nice :)
• For Redux Authors (me and John): I think we should make TypedMiddleware a bit easier to extend. In ActorMiddleware you're extending MiddlewareClass and doing 1 type-check in the call implementation. It would be cool if you could just extend TypedMiddleware<AppState, FetchActorAvatarsAction> and implement a middleware method or something with the correct type information / knowing the action has been filtered for ya.
• Could you use the selector found in the theaters directory for this?

  inKino/lib/redux/show/show_middleware.dart

  Line 31 in 04667f8

  Theater theater = store.state.theaterState.currentTheater;

• I like the idea of putting the XML to parse in the test file as a string.
• +1 to the idea of putting tests next to the file they test. However, Dart tooling doesn't support that case very well in my experience, and the recommend approach is to put tests inside a test folder. I wonder if that's an issue we could file against the Dart SDK / Flutter repos?
• LOL:

  inKino/test/redux/actor/actor_reducer_test.dart

  Line 14 in c7d3fc9

  'Seth Ladd': new Actor(

• <3 Mockito, really nice example usage in this project
• Great demonstration of overriding the HttpClient! However, in some cases, I wonder if it makes more sense to inject an HttpClient or http.Client into the getRequest function rather than relying only a static _httpClient? If you were to extract these APIs into their own packages, you could consider using the http package and this technique to make it work cross-platform (not a requirement for a purely mobile app, just a thought)
• At this point I'm really just fishing for improvements. Really nice work overall :)

from inkino.

Thanks Brian! I'll address your feedback too when I can :)

from inkino.