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

Code review #52

Open
thibaudcolas opened this Issue Apr 9, 2018 · 9 comments

Comments

Projects
None yet
5 participants
@thibaudcolas

thibaudcolas commented Apr 9, 2018

This is the best format I could think of, to easily get embedded code snippets and links, and some notion of "actions".

Please note that my experience with mobile development only amounts to 1 React Native app, 1 iOS App, 0.1 Android app – and I did a bit of Dart once. I might be criticising things that are well-established conventions in the mobile / Dart world 🙂. I also nitpicked a lot.


General comments

  • The code looks great! Don't pay too much attention to all my other comments, it really is. It's very readable, even for someone like me who doesn't have a clue about Flutter but has looked at Dart code before, as well as JS and Java.
  • I think naming is overall really good, long names where appropriate and short ones where appropriate. That goes a long way.
  • I dunno if you could do a better usage of types – don't know Dart enough to judge of that.
  • There is quite a bit of boilerplate code. I imagine this comes with the language / Flutter (eg. all those redefinitions of the == operator or hashCode), but worth stating anyway.
  • I think more code could be written in a functional style (eg. map over forEach + mutating an array). There isn't that much complicated logic in the app right now that it would make a big difference, but it would definitely help with readability.
  • I'm surprised there isn't more reuse / abstraction for "styling" code (eg. font styles, colors, gradients, spacing). Not sure whether this is an issue, but it would seem to make it harder to ensure consistency in some of the key visual elements (animation timings, gradients, etc).

Things that are missing

I'm not sure they really are missing 😄 I'm just surprised not to see them:

  • A CI setup running the tests (with a badge, or link in the README)
  • Maybe some test coverage info or link to a collection service?
  • Linting tools
  • Some sort of script / task runner (eg. Make, or a package.json with yarn/npm in JS-land)
  • Installation instructions
  • Instructions to run the tests

I imagine these might come for free with the SDK though.


In no particular order, except for this first section,

File structure & naming

  • I see the test folder has the same structure as lib, and test files are suffixed with _test. Wouldn't this be easier to maintain if tests were located next to the file under test within lib? (I imagine there's some convention to follow here – personally I like that approach because it makes it much easier to see what has and doesn't have tests). You also might not have to do things like For more, see test/event_name_cleaner_test.dart. if the test file was right there.

  • Same for the test data inside test_assets at the moment. It would make it much easier to understand the XML parsing code in say models/event if the XML stub was right next to it.

  • The test data (https://github.com/roughike/inKino/tree/v1.0.0/test_assets) uses the assets terminology, but those files technically aren't bundled with the app – should use stub instead?

  • I'm not sure what to think of the terminology of having networking and model under data. If the API you were talking to was anything that's not understood as a data source, it would still fall under networking (eg. use the same networking utils) but wouldn't necessarily be data. Also model feels important enough that it should be top-level – and again models are more than just data (business logic)

  • Finally, have you considered organizing the app code by feature/domain (eg events, showtimes, etc) rather than by file type / function (eg. redux, models)? It is sort of done already for Redux and the UI (which naturally work well by domain), so would be worth considering doing the same for the whole app IMHO.

Tests

expect(paris1517.id, '302535');
expect(paris1517.title, '15:17 Pariisiin (2D dub)');
expect(paris1517.cleanedUpTitle, '15:17 Pariisiin');

  • I see the deserialisation tests a lot of manual assertions. I think it's fine/good for such a small test suite, but there might be a snapshot testing feature you could leverage in your test framework that would auto-generate those?

sut = new ShowMiddleware(mockApi);

  • sut – system under test?. Not the clearest variable name IMHO. middleware?

Utils

static final RegExp _pattern =
new RegExp(r"(\s([23]D$|\(([23]D|dub|orig|spanish|swe).*))");

Data

  • General comment – I notice the API response parsing code is somewhat coupled with the app's models. Generally I think it's a good idea if the app "owned" the API, but here that's not the case. Have you considered separating this out as a standalone Dart client for the Finnkino API? Since it could be reused beyond this one app and its models, this seems desirable IMHO.

var trailers = <String>[];
nodes.forEach((node) {
trailers.add(

document.findAllElements('Show').forEach((node) {
var title = tagContents(node, 'Title');
var originalTitle = tagContents(node, 'OriginalTitle');
shows.add(new Show(

  • Looks like these should be a map instead of a forEach. (same comment for other models potentially)

/// Entirely redundant theater, which isn't actually even a theater.
/// The API returns this as "Valitse alue/teatteri", which means "choose a
/// theater". Thanks Finnkino.
static const String kChooseTheaterId = '1029';

  • 😂 priceless

Networking

/// If this has a red underline, it means that you haven't created
/// the lib/tmdb_config.dart file. Refer to the README for instructions
/// on how to do so.
import 'package:inkino/tmdb_config.dart';
/// If this has a red underline, it means that you haven't created
/// the lib/tmdb_config.dart file. Refer to the README for instructions
/// on how to do so.

  • I know this is documented in the README but this still strikes me as very odd. Secrets shouldn't be inlined in application code, and if the code can compile without the API key actually being set up, it should. You could consider logging a dev-time warning instead.
  • If you want to keep it as-is, consider adding a tmdb_config.dart.sample file with its content as what the README shows, and a cp command in the README.
  • Also duped comment

UI

description:
description ?? 'There was an error while\nloading movies.',
onActionButtonTapped: onRetry,

  • I'm surprised to see text overflow manually handled like that. Is this idiomatic?

class TheaterListViewModel {
TheaterListViewModel({

  • Personal preference – I find it easier to understand if the view model is inlined within the view file, since the two are highly coupled.

description:
'Didn\'t find any movies\nabout to start for today. ¯\\_(ツ)_/¯',
);

  • Should this be "on the selected day" (or equivalent) instead of "today"?

style: new TextStyle(
fontWeight: FontWeight.w500,
fontSize: 16.0,
),
),

  • I'm surprised text styles like this aren't extracted to some sort of "stylesheet". It would make it easier to ensure the styles are consistent?

gradient: new LinearGradient(
begin: Alignment.bottomCenter,
end: Alignment.topCenter,
colors: <Color>[
const Color(0xFF222222),
const Color(0xFF424242),
],
),

decoration: new BoxDecoration(
gradient: new LinearGradient(
begin: Alignment.bottomCenter,
end: Alignment.topCenter,
colors: <Color>[
const Color(0xFF222222),
const Color(0xFF424242),
],
),
),

  • Same question as above – this gradient is duplicated once. Should it be extracted somewhere?

placeholder: ImageAssets.transparentImage,
image: event.images.portraitMedium ?? 'http://example.com',
width: size?.width,

  • This looks like it's hotlinking example.com, but I'm not entirely sure because I don't know how the placeholder works. Either way this seems like a bad idea – is there a better fallback value?

Redux

  • I would advocate for _reducer and _state (and potentially _selectors) to be co-located in the same file by default. All of this code is very tightly coupled, and is harder to follow in separate files (note: this might be true only in JS, maybe the IDE features for a language like Dart are good enough that it's less of an issue). That said, I know this isn't a very popular opinion even in JS land, and it only works if all those three things are relatively small, so take this with a grain of salt.

// YOLO! We don't need to handle this. If fetching actor avatars
// fails, we don't care.

  • 👍 maybe mention what happens UI-wise in that scenario, so someone looking at this knows they don't have to worry.

return new Store(
appReducer,
initialState: new AppState.initial(),

  • I don't think it's idiomatic redux to have a root reducer that does more than just combining other reducers – looking at the file structure, I would not expect app to be the one combining all of the others.

List<Event> _uniqueEvents(List<Event> original) {
var uniqueEventMap = new LinkedHashMap<String, Event>();
original.forEach((event) {
uniqueEventMap[event.originalTitle] = event;

  • Isn't there a LinkedSet type in Dart?
  • I'm not 100% sure but it looks like there is no place where we would want duplicate events. Why not do this in the middleware or reducer instead of the selectors?

Did I mention the code actually is very readable?

@roughike

This comment has been minimized.

Show comment
Hide comment
@roughike

roughike Apr 9, 2018

Owner

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!

Owner

roughike commented Apr 9, 2018

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!

@thibaudcolas

This comment has been minimized.

Show comment
Hide comment
@thibaudcolas

thibaudcolas Apr 11, 2018

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

thibaudcolas commented Apr 11, 2018

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

@roughike

This comment has been minimized.

Show comment
Hide comment
@roughike

roughike Apr 11, 2018

Owner

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? :)

Owner

roughike commented Apr 11, 2018

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? :)

@thibaudcolas

This comment has been minimized.

Show comment
Hide comment
@thibaudcolas

thibaudcolas Apr 11, 2018

Sure, go for it 👍

thibaudcolas commented Apr 11, 2018

Sure, go for it 👍

@anasbud

This comment has been minimized.

Show comment
Hide comment
@anasbud

anasbud Apr 11, 2018

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.

anasbud commented Apr 11, 2018

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.

@roughike

This comment has been minimized.

Show comment
Hide comment
@roughike

roughike Apr 11, 2018

Owner

@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. :)

Owner

roughike commented Apr 11, 2018

@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. :)

@brianegan

This comment has been minimized.

Show comment
Hide comment
@brianegan

brianegan Apr 11, 2018

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?
    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:
    '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 :)

brianegan commented Apr 11, 2018

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?
    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:
    '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 :)
@roughike

This comment has been minimized.

Show comment
Hide comment
@roughike

roughike Apr 11, 2018

Owner

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

Owner

roughike commented Apr 11, 2018

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

@Pacane

This comment has been minimized.

Show comment
Hide comment
@Pacane

Pacane Apr 11, 2018

Wow nice code review indeed.

Pacane commented Apr 11, 2018

Wow nice code review indeed.

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