Code review

[thibaudcolas]

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

inKino/test/data/event_test.dart

Lines 15 to 17 in ead83ac

	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?

inKino/test/redux/show/show_middleware_test.dart

Line 32 in ead83ac

sut = new ShowMiddleware(mockApi);

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

Utils

inKino/lib/utils/event_name_cleaner.dart

Lines 10 to 11 in d885528

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

• This is just a personal thing but when I make this type of regex I like to add a link to it to a service like https://regexper.com/#%28%5Cs%28%5B23%5DD%24%7C%5C%28%28%5B23%5DD%7Cdub%7Corig%7Cspanish%7Cswe%29.*%29%29 to make it easier to understand. Of course it might fall out of date compared to the actual regex. Tradeoffs 😶

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.

inKino/lib/data/models/event.dart

Lines 95 to 98 in d885528

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

inKino/lib/data/models/show.dart

Lines 32 to 36 in d885528

	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)

inKino/lib/data/models/theater.dart

Lines 8 to 11 in d885528

	/// 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

inKino/lib/data/networking/tmdb_api.dart

Lines 7 to 13 in d885528

	/// 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

inKino/lib/ui/common/info_message_view.dart

Lines 15 to 17 in d885528

	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?

inKino/lib/ui/theater_list/theater_list_view_model.dart

Lines 7 to 9 in d885528

	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.

inKino/lib/ui/showtimes/showtime_list.dart

Lines 19 to 21 in d885528

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

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

inKino/lib/ui/events/event_grid_item.dart

Lines 37 to 41 in d885528

	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?

inKino/lib/ui/events/event_poster.dart

Lines 38 to 45 in d885528

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

inKino/lib/ui/event_details/event_backdrop_photo.dart

Lines 39 to 48 in a54af3d

	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?

inKino/lib/ui/events/event_poster.dart

Lines 86 to 88 in d885528

	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.

inKino/lib/redux/app/app_middleware.dart

Lines 32 to 33 in d885528

	// 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.

inKino/lib/redux/store.dart

Lines 20 to 22 in d885528

	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.

inKino/lib/redux/event/event_selectors.dart

Lines 21 to 24 in d885528

	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 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]

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

[roughike]

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]

Sure, go for it 👍

[anasbud]

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]

@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]

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

[roughike]

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

[Pacane]

Wow nice code review indeed.