Comments (9)
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. InActorMiddleware
you're extendingMiddlewareClass
and doing 1 type-check in thecall
implementation. It would be cool if you could justextend TypedMiddleware<AppState, FetchActorAvatarsAction>
and implement amiddleware
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? - 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:
- <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
orhttp.Client
into thegetRequest
function rather than relying only a static_httpClient
? If you were to extract these APIs into their own packages, you could consider using thehttp
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.
Related Issues (20)
- Tags! HOT 1
- AngularDart component tests
- Question: What's your impression of the dev experience of AngularDart? HOT 1
- Can the flutter sdk and websdk be installed together? HOT 1
- casts names are overlapping
- i have a question
- Mobile - Review UI/UX HOT 2
- Build showing errors
- Question about last release
- webdev could not run for this project.
- Reusing the software
- Accepted Media Resource Formats HOT 1
- Screen Rotation HOT 1
- No movie show in the list, api call seems not work HOT 3
- Can not run on mobile HOT 1
- F-Droid inclusion
- flutter pub get fails on 'master' branch HOT 3
- Is the complete code?
- dependencies getting out of date HOT 2
- No cast pictures HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from inkino.