Skip to content

Conversation

@AliSoftware
Copy link
Collaborator

@AliSoftware AliSoftware commented Apr 3, 2020

Here's my initial global review of your project. I've tried to read it in the same mindset I'd have been if I were a recruiter / reviewing it for an interview and taking notes along the way.

  • I've written all my notes in ReviewNotes.md. That's probably where you want to start reading
    • Please note that those notes were taken while going thru your project, so the wording and tone might not always be the best. Don't be offended if it might sometimes be worded poorly 😅
  • Then I've created one commit per topic I'm speaking about in those notes. The ReviewNotes.md contains links to the individual commits showing how to fix the things I suggested

I didn't take the time to review the Unit Tests yet though, might do that later.

Didn't really review your ViewControllers either, as I focused on architecture and Models, ViewModels and NetworkLayer

TL;DR:

  • At a glance the project is well structured and architecture seems on a good start 👍 Very easy to navigate too, and good separation of concerns overall
  • The main issue is with how you handle the async load of your images. This is currently done on your View Layer via an extension on UIImageView. That causes cell reuse bugs, and image loading and refreshing should be handled by the ViewModel layer instead anyway

I'll let you go thru the long ReviewNotes.md doc, and thru the commits one by one, you have plenty to read 😄

Olivier Halligon added 16 commits April 3, 2020 19:26
Put it in Dependencies.swift, could have moved it to a files in `Globals/` folder instead, ultimate goal is to make it easily extractable to later find a way to not commit it at all
Though this doesn't solve the problem that it shouldn't be the responsibility of the View layer to load an image and that it's gonna lead to cell recycling bugs
Olivier Halligon added 2 commits April 4, 2020 14:04
@AliSoftware
Copy link
Collaborator Author

Added two new commits

  1. Fixing the fact that the Network layer should be the one injecting the APIKey, not your APIRequest model objects. And you should make the app setup pass the key in the NetworkSession init
    Think of it as the Network layer being a micro-framework that should be independant of your app and usage of it in this app; it should be able to provide an APIClient for anyone willling to use the API and not be tied to your specific APIKey… nor even your Dependencies at this level

  2. Fixing some logic in the tests: you can’t compare URLs in tests because if a URL produces either "?a=1&b=2" or "?b=2&a=1" both are valid. So I tested the components instead.

During my shower thoughts, I realised that my suggestion and fix of moving the apiKey to your Dependencies was not right, because it makes your NetworkSession implicitly depend on a singleton that is your Dependencies. Meaning you wouldn't be able to extract your Network layer into a separate micro-framework and making it independent to respect the SOLID principle and separation of concerns and layers. Injecting the APIKey during NetworkSession.init is way better architecture-wise (the place where you store that API Key to avoid leaking it in GH and code is another, kinda unrelated question)

PS: If you wanted to add a test to check that the NetworkSession properly constructs the right URLRequest – now that this part of the construction is owned by the NetworkSession rather than the APIRequest, you can still write a test that creates a real NetworkSession(apiKey: "foo") instance and call its urlRequest(from: apiRequest) method then check the headers etc.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants