diff --git a/.DS_Store b/.DS_Store deleted file mode 100644 index fdd704c..0000000 Binary files a/.DS_Store and /dev/null differ diff --git a/README.md b/README.md index 2d6737c..e2513ca 100644 --- a/README.md +++ b/README.md @@ -7,15 +7,15 @@ Create an app to show the images from Unsplash App. - [x] Show description of the image on hover - [x] Open the image on full screen -![full image](/full-image.gif) +![full image](./full-image.gif) - [x] On full screen you can download and share the image ( you can see the author's name too ) - [x] Be able to search for specific keywords -![search](/search.gif) +![search](./search.gif) - [x] Add big keywords on search `collections` `photos` `curated` -- [x ] Make app universal +- [x] Make app universal - [ ] Animations transitions between controllers - [ ] Unit tests - [ ] Integration tests diff --git a/ReviewNotes.md b/ReviewNotes.md new file mode 100644 index 0000000..d0f902b --- /dev/null +++ b/ReviewNotes.md @@ -0,0 +1,166 @@ +# Review Notes + +## First impressions + + - Good outlook of project + - You have your files nicely grouped in a clear way View, ViewModels, etc. Makes it also clear that you're using an architecture that makes use of ViewModels, I like that too + - You seem to have tests, that's a good start too. The Unit Test target [does not compile though](https://github.com/bertadevant/UnsplashAPIApp/commit/b7b8e59f06ca91196afccf42cbe5eea0644b2b94), and once I fix it to make it compile, I have 3 tests failing + - [README](https://github.com/bertadevant/UnsplashAPIApp/commit/89ead7af925b4f87d8fbe419d6e557ff59231218): I know it's just a private project for now, but for demo projects intended for interviews I suggest you to add some notes in the future, like: + - What does this demo shows: That's already part of your README so, great + - What architecture choices did you make and why. There's no silver bullets in architecture, the most important thing, as least to my PoV, is that you are able to explain your choices and why you went with one solution rather than another. Is it to make it testable? To make it flexible? Easy to reason about? Be SOLID? etc + - You had (easy to fix) warnings when opening the project: not a great look for reviewer opening your code + - some warnings about code [commit](https://github.com/bertadevant/UnsplashAPIApp/commit/98eeded041080b085ff152032ca6470ad882a971) + - some warnings about assets [commit](https://github.com/bertadevant/UnsplashAPIApp/commit/7c95db452f68b8021b26a4ee9a5fe4ba42889b77) + - You don't have any dependencies in that project. That great. + - I mean, if that was a sign that you instead reinvented every complex thing yourself instead of using an existing lib (NIH syndrome) that wouldn't be a good sign + - Or if it was because you instead copy/pasted lib code into your project reather than using a dep manager, that wouldn't be good either + - But in your case at least I can see that since you didn't need any dependency, you didn't feel the urge to have a `Podfile` or `Cartfile` like some other + people that I see including well-known pods in their demo project "just because it's a pod I see everywhere and add no questions asked" like AFNetworks or similar, + while they actually don't need it. In that sense that's a good sign that your project is clean and simple without any useless deps! + +### UX + +When running the app in the simulator (usually the first thing I do on a interviewee project, also to see if it compiles and works out of the box), UI felt nice and + enjoyable. +But the time it took for the images to load felt quite long, and I only got a solid-fill rectangle in the ImageView while waiting for the actual image to load. + +Maybe at least add a spinner or something while loading the image to let the user know that it's loading instead of feeling like it's a bug. +[commit](https://github.com/bertadevant/UnsplashAPIApp/commit/5991ec7ac10c3f993b23d5c52be87a6f3f2937d8) – though that won't help with the recycling bug (see below) + +Ideally starting by loading lower resolutions of the image first, and only load full-res when you click to show the image full-screen would also probably improve the UX a lot (idk if the Unsplash API provides those lower-res images?). + +That's also probably a feature you just didn't have time to develop just yet, but in that case it would still be worth mentioning that in the README for the recruiter as "Future Improvements" to let them know that you thought about it. + +### Cell recycling bug + +Because you use `UIImageView` method that asynchronously sets its own image after an async network request, there is an issue with cell recycling. +Because if the user scrolls while the image is loading, your CollectionView will recycling the views that are going offscreen even if they were loading, and reuse them for a different image. + +So basically: + - if you have set an image `urlA` on an `UIImageView` in cell X by calling your method that set the image from URL + - then the user scrolls down in the `UICollectionView`, so cell X goes offscreen, and is recycled and reused for a later `IndexPath` down the road, this time corresponding to `urlB` + - so you call your method to set the image of that cell to `urlB` (same cell, since it's recycled from previous cell X you used for `urlA`) + - at that stage there are two network requests in the air, one for `urlA` and one for `urlB`, that will ultimately update the `self.image` of the same `UIImageView` instance + - so if `urlA` 's response arrives before `urlB`'s response, you'll see image of `urlA` in a cell that has since been recycled and is supposed to contain image of `urlB`… and only a bit later once `urlB`'s response arrives will you see that `UIImageView` be updated with the expected image (that is, if the user didn't continue scrolling yet another `IndexPath`that started to load `urlC`… and so on) + - if `urlB`'s response arrives before `urlA`'s response, then you'll see the expected image in your cell… but then later the old and unexpected one + +⚠️ So overall it's best to handle that loading logic at the level of your `ImageListViewModel` instead. Instead of holding a list of images (your `typealias ImageList` in your VM), your VM could for example hold a list of `[ImageState]` with `ImageState` could be an `enum ImageState { case loading(URL); case loaded(Image) }`. + +It's then the responsability of your ViewModel to decide if it needs to provide the cell with a loading view with placeholder, or with a loaded view with image. This responsability should not be owned by the view layer (your `UIImageView` extension in your case) + + +## Project Notes + + - Some files have names that don't match the class they containing an implementation for. This makes it hard to find back where each class is defined. + - For example `URLSessionExtension+Load` contains `protocol Session` and `class NetworkSession` (but no `extension URLSession`). + - I figure that's legacy from a former impl, but it's still nice to tidy things up + - Some classes are declared in places that seems unlikely or unrelated. For example you declare your color constants in `Global/Colors.swift` (👍) but you declare your Font constants in… `CellStyle.swift`? [commit](https://github.com/bertadevant/UnsplashAPIApp/commit/2b7c4f93e1886a67f16c929817e46b1997fa5653) + + +## Code Notes + +### `CellSize.swift` + +[commit](https://github.com/bertadevant/UnsplashAPIApp/commit/2b9250901187c510e70bfef1891df18da59df596) + +I don't really like the use of `UIApplication.shared.keyWindow` here for multiple reasons: + +1. the `keyWindow` can change during the lifecycle of the app, so depending on when those static var are (lazily) created, this will affect the default size +2. this relies on using a shared singleton instance, `UIApplication` even. Not good for testability + +But actually I didn't even see that `defaultSize` property used anywhere anyway, so ended up deleting it – not a good idea to keep dead code in a demo project 😅 – but still wanted to note those suggestions here. + +### Creating viewModels from Models + +I'm not sure if I can really justify why, as it's more like a feel and convention, but it feels strange to provide extensions on your models to build ViewModels from it, rather than provide an init to your ViewModels to create them from your Models. + +I think the intuition I get from this is that ViewModels should be build from models, like there's a sense of hierarchy in the architecture layers here. A ViewController has/is constructed from a ViewModel which is constructed from a Model. So you pass the ViewModel to the Controller to build it, so also makes sense that you pass the Model to the `ViewModel.init` to build it + + +You also seem to use `ViewModel` and `ViewState` interchangeably or at least confuse the two sometimes, e.g. `func viewModel() -> AuthorViewState` on `extension Author` in `ImageViewState.swift` + +Also, your type `struct Colors` has a name that is a little too close from your `struct Color` you use in … `Colors.swift` to declare your color constants. +And since it's only used as an inside type for your `ImageViewState`, best solution is to just declare it as a subtype of `ImageViewState`, both to scope it and avoid any confusion. + +[commit](https://github.com/bertadevant/UnsplashAPIApp/commit/2c383d1bb099618f871198f3b7a3abe96d29058c) + +### `UIButton+LoadingIndicator` + +It's quite bad practice to use `UIView.tag`. It's a hack in UIKit that should never have existed, and it's generally viewed poorly when seen in code. + +I highly suggest that you create a subclass `class LoadingButton: UIButton` which adds the loadingIndicator as subview on its `init`, store that in a private property of your `LoadingButton`, recenters it on `layoutSubviews`, and provide a public API to toggle the loadingIndicator instead. + +## NetworkLayer + +### `LoadAPIRequest` + +It's a bit confusing that you build a LoadAPIRequest from a full URL string, then delete the https://{host} from that String, only to later re-inject it via the components… and rebuild the URL on your Network Layer… + +Besides repeating the scheme and host twice (in `var components` and in the `init` when you remove it), this is not super testable either… + +[commit](https://github.com/bertadevant/UnsplashAPIApp/commit/05ab305eec99b72bb601776b8a96e1eaebaf29c1) + +### `APIRequest` + +Not a good look to have your API key hardcoded here, not only for security reasons but also because keys like that should not be defined deep inside the network layer code, as they would be hard to find later and don't make your layer independant. + +This is also because your `NetworkLayer` should be agnostic of who is using it. It should be a client that works for anyone willing to use the Unspalsh API. Hard-coding a Client-ID inside the NetworkLayer itself is not a good separation of concern there. The best solution is to inject that key when instantiating your `NetworkSession` + +As for the security part of things, that's less trivial to fix: ideally you should not commit your API keys to GitHub. You could for example have your keys in some `plist` file that you include in your project and read at runtime, but never commit that `plist` and document in the README that the user is supposed to create it with their own APIKey. + +It would still be ok to provide the plist with your own Key to a recruiter as part of a ZIP containing your project so that they are able to run the project immediately after unzipping, but at least you'd have shown that you didn't hardcode the APIKey. + +Other solutions exists (the best probably being CocoaPods-Keys, but since you don't use CocoaPods in this project, probably not worth the effort for a simple toy project like that) + +BTW this is typically the kind of things that you could document in your README for a recruiter btw (e.g. "I hardcoded the API Key in this config file that I provided here, to make it simple for reviewers to be able to run the project out of the box, but in a real project I'd have used X or Y instead") + +[commit](https://github.com/bertadevant/UnsplashAPIApp/commit/844c93bef1acbc10bd99c37e87f59d9197f44e5b) + +Another note: why the need for the `APIRequest` protocol to have both a `URLComponent` and a `queryItems`, while you always include the queryItems as part of the `URLComponents` anyway? Also, you repeat the `https` and `api.unsplash.com` multiple times… [commit](https://github.com/bertadevant/UnsplashAPIApp/commit/79dc9ee2a6bdacab315afc13e09c27969a0580fe) + +### `Resource.swift` + +I liked that very much! The concept of bundling `APIRequest` + `parse` method and making it monadic by providing a `map` on it is a very good choice. + +Only thing I would change is to not discard the potential error there. i.e. change `parse` to a `(Data) throws -> A` and throw the error instead of returning `nil` + +### `URLSessionExtension+Load.swift` (now `NetworkSession.swift`) + +I like the idea of having a `Session` protocol, makes things testable + +I'd not make `NetworkSession` inherit from `URLSession` though. There is no need for that, and it introduces the risk of allowing you to write methods taking `URLSession` (instead of `Sessions`) parameters and still compile when you pass a `NetworkSession` without warning you that you used the wrong parameter type (as expecting a `URLSession` parameter instead of using your `Session` abstraction won't make it testable). + +[commit](https://github.com/bertadevant/UnsplashAPIApp/commit/76607870a8de2f4dc2ac777af47d85d40c5362ab) + +## Other + +I liked that you use `struct SearchCategory` with and `extension SearchCategory` containing `static` constants. Those should be `let` not `var` though. + +But otherwise it's a nice solution to use rather than an `enum`. Some people would have used an `enum` instead but an `enum` is supposed to be closed (a finite set of values that have no intention to grow or change) and something for which using a `switch` on would make sense, which is not the case here. So using a `struct` + `extension with static let` was a good choice, because it makes it clear that those are some suggested values but that it's also valid to add more, and to also allow other devs using your code to create their own `SearchCategory` rather than being limited on using only a fixed/closed set of the ones provided. 👍 + +--- + +Speaking of `enum` vs `struct`, when declaring types that only serve as namespaces, like `Color` or `Fonts` (oh, inconsistent naming here, singular vs plural…), it's common practice to: + + - Either declare `private init(){}` on your struct serving as namespace, because that type is only containing `static` properties and is not intended to be instantiated + - Or more commonly, use `enum` (even if it's a `case`-less enum and kind of working around the original purpose of `enum`), which ultimately provides the same behavior (declaring a type just for the sake of being used as a namespace containing only `static` properties) but doesn't require you to declare a `private init(){}` explicitly, since an `enum` with no case (known in the jargon as an "inhabited type") already can't be instanciated. + - Use `let` instead of `var` for those `static` constants. They are constants, after all! + + A use case where you can use `struct` instead of `enum` is if you _do_ intend to (or that it _does_ makes sense to) let a developer using your code to create other things of that type. + - For `enum Fonts` and `enum Colors` those are namespaces returning objects of a different type (those enum are an inhabited type anyway so you couldn't create an instance of those), namely `UIColor` and `UIFont` here. You don't intend other devs to do `let f = Fonts()` + - But for `struct SearchCategory` you provide a type, then provide default common values for it via `static let` properties returning `SearchCategories` too, so that's a bit different. You don't want to forbid other devs to create theyr own if they need/want to (why would you prevent others to call `SearchCategory(…, …)` init after all) so for this one it makes sense to keep it as a `struct` + +[commit](https://github.com/bertadevant/UnsplashAPIApp/commit/7a427175dc1101f83815127bfe352a9fc8bcef3c) + +## TODO + +For me as a reviewer: + +- [ ] Review the code of the Tests, I haven't taken the time yet to go thru them +- [ ] Review the `ViewController`, I kinda skipped them as I focused on architecture and the Models/ViewModels/Network layers + +For you, things that I didn't provide a commit/fix for: + +- [ ] Image Loading: move loading logic (network request) to ViewModel layer, + see if we can't load low-res first +- [ ] UIButton subclass with loader instead of `extensions` + `view.tag` +- [ ] Fix your Unit Tests diff --git a/UnsplashApiApp.xcodeproj/project.pbxproj b/UnsplashApiApp.xcodeproj/project.pbxproj index 5f1a9cf..882d1ee 100644 --- a/UnsplashApiApp.xcodeproj/project.pbxproj +++ b/UnsplashApiApp.xcodeproj/project.pbxproj @@ -9,11 +9,11 @@ /* Begin PBXBuildFile section */ 0E134ECC22FAFE7600810484 /* HttpMethod.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0E134ECB22FAFE7600810484 /* HttpMethod.swift */; }; 0E134ECF22FAFE9A00810484 /* Image.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0E134ECE22FAFE9A00810484 /* Image.swift */; }; - 0E134ED122FAFEC400810484 /* URLSessionExtension+Load.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0E134ED022FAFEC400810484 /* URLSessionExtension+Load.swift */; }; + 0E134ED122FAFEC400810484 /* NetworkSession.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0E134ED022FAFEC400810484 /* NetworkSession.swift */; }; 0E134ED322FAFF2300810484 /* APIRequest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0E134ED222FAFF2300810484 /* APIRequest.swift */; }; 0E134ED622FC5DAC00810484 /* APIRequestsTestDoubles.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0E134ED522FC5DAC00810484 /* APIRequestsTestDoubles.swift */; }; 0E134EDB22FC620300810484 /* Dependencies.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0E134EDA22FC620300810484 /* Dependencies.swift */; }; - 0E134EDD22FC670000810484 /* URLSessionTestSession.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0E134EDC22FC670000810484 /* URLSessionTestSession.swift */; }; + 0E134EDD22FC670000810484 /* SessionSpy.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0E134EDC22FC670000810484 /* SessionSpy.swift */; }; 0E134EDF22FC691E00810484 /* ResourceAndResponse.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0E134EDE22FC691E00810484 /* ResourceAndResponse.swift */; }; 0E134EE122FC6AFF00810484 /* Image+TestData.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0E134EE022FC6AFF00810484 /* Image+TestData.swift */; }; 0E134EE32300AA7A00810484 /* APIRequestTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0E134EE22300AA7A00810484 /* APIRequestTests.swift */; }; @@ -41,6 +41,7 @@ 0ED7463C243255A300E8E625 /* TestSpy.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0ED7463B243255A300E8E625 /* TestSpy.swift */; }; 0EDA929623018F3F0044F35A /* ImageFullScreenView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0EDA929523018F3F0044F35A /* ImageFullScreenView.swift */; }; 0EDA92982304874B0044F35A /* ImageFullViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0EDA92972304874A0044F35A /* ImageFullViewController.swift */; }; + 380BAFD12437CB0000C8ABE9 /* Fonts.swift in Sources */ = {isa = PBXBuildFile; fileRef = 380BAFD02437CB0000C8ABE9 /* Fonts.swift */; }; /* End PBXBuildFile section */ /* Begin PBXContainerItemProxy section */ @@ -56,11 +57,11 @@ /* Begin PBXFileReference section */ 0E134ECB22FAFE7600810484 /* HttpMethod.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = HttpMethod.swift; sourceTree = ""; }; 0E134ECE22FAFE9A00810484 /* Image.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Image.swift; sourceTree = ""; }; - 0E134ED022FAFEC400810484 /* URLSessionExtension+Load.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "URLSessionExtension+Load.swift"; sourceTree = ""; }; + 0E134ED022FAFEC400810484 /* NetworkSession.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NetworkSession.swift; sourceTree = ""; }; 0E134ED222FAFF2300810484 /* APIRequest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = APIRequest.swift; sourceTree = ""; }; 0E134ED522FC5DAC00810484 /* APIRequestsTestDoubles.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = APIRequestsTestDoubles.swift; sourceTree = ""; }; 0E134EDA22FC620300810484 /* Dependencies.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Dependencies.swift; sourceTree = ""; }; - 0E134EDC22FC670000810484 /* URLSessionTestSession.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = URLSessionTestSession.swift; sourceTree = ""; }; + 0E134EDC22FC670000810484 /* SessionSpy.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SessionSpy.swift; sourceTree = ""; }; 0E134EDE22FC691E00810484 /* ResourceAndResponse.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ResourceAndResponse.swift; sourceTree = ""; }; 0E134EE022FC6AFF00810484 /* Image+TestData.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "Image+TestData.swift"; sourceTree = ""; }; 0E134EE22300AA7A00810484 /* APIRequestTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = APIRequestTests.swift; sourceTree = ""; }; @@ -92,6 +93,7 @@ 0ED7463B243255A300E8E625 /* TestSpy.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TestSpy.swift; sourceTree = ""; }; 0EDA929523018F3F0044F35A /* ImageFullScreenView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ImageFullScreenView.swift; sourceTree = ""; }; 0EDA92972304874A0044F35A /* ImageFullViewController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ImageFullViewController.swift; sourceTree = ""; }; + 380BAFD02437CB0000C8ABE9 /* Fonts.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Fonts.swift; sourceTree = ""; }; /* End PBXFileReference section */ /* Begin PBXFrameworksBuildPhase section */ @@ -146,7 +148,7 @@ children = ( 0ED7463B243255A300E8E625 /* TestSpy.swift */, 0E134ED522FC5DAC00810484 /* APIRequestsTestDoubles.swift */, - 0E134EDC22FC670000810484 /* URLSessionTestSession.swift */, + 0E134EDC22FC670000810484 /* SessionSpy.swift */, 0ED746392432554F00E8E625 /* ImageListViewModelDelegateTestDouble.swift */, ); path = TestDoubles; @@ -157,6 +159,7 @@ children = ( 0E134EDA22FC620300810484 /* Dependencies.swift */, 0E6432A0230B677000B2A027 /* Colors.swift */, + 380BAFD02437CB0000C8ABE9 /* Fonts.swift */, ); path = Global; sourceTree = ""; @@ -206,7 +209,7 @@ children = ( 0E57708422FAFA2F004F78DE /* Resource.swift */, 0E134ECB22FAFE7600810484 /* HttpMethod.swift */, - 0E134ED022FAFEC400810484 /* URLSessionExtension+Load.swift */, + 0E134ED022FAFEC400810484 /* NetworkSession.swift */, 0E134ED222FAFF2300810484 /* APIRequest.swift */, ); path = "Network Layer"; @@ -383,7 +386,7 @@ isa = PBXSourcesBuildPhase; buildActionMask = 2147483647; files = ( - 0E134ED122FAFEC400810484 /* URLSessionExtension+Load.swift in Sources */, + 0E134ED122FAFEC400810484 /* NetworkSession.swift in Sources */, 0E134EF52300CFBD00810484 /* UIView+AutoLayout.swift in Sources */, 0E134F002300E0DC00810484 /* CellStyle.swift in Sources */, 0E7435A1230B5889007C1EA5 /* SearchCategory.swift in Sources */, @@ -399,6 +402,7 @@ 0E743597230AEE1C007C1EA5 /* SearchParameters.swift in Sources */, 0E134EF72300D0F000810484 /* ImageViewState.swift in Sources */, 0E57708522FAFA2F004F78DE /* Resource.swift in Sources */, + 380BAFD12437CB0000C8ABE9 /* Fonts.swift in Sources */, 0E134EDB22FC620300810484 /* Dependencies.swift in Sources */, 0E134EEC2300CDEA00810484 /* ImageListViewController.swift in Sources */, 0E134EF92300D14200810484 /* UIColor+HexInit.swift in Sources */, @@ -418,7 +422,7 @@ 0E134EDF22FC691E00810484 /* ResourceAndResponse.swift in Sources */, 0E134ED622FC5DAC00810484 /* APIRequestsTestDoubles.swift in Sources */, 0E7435922305FC27007C1EA5 /* UIColorExtensionsTests.swift in Sources */, - 0E134EDD22FC670000810484 /* URLSessionTestSession.swift in Sources */, + 0E134EDD22FC670000810484 /* SessionSpy.swift in Sources */, 0ED7463A2432554F00E8E625 /* ImageListViewModelDelegateTestDouble.swift in Sources */, 0E134EE32300AA7A00810484 /* APIRequestTests.swift in Sources */, 0E134EE122FC6AFF00810484 /* Image+TestData.swift in Sources */, diff --git a/UnsplashApiApp/Assets.xcassets/AppIcon.appiconset/AppStore.png b/UnsplashApiApp/Assets.xcassets/AppIcon.appiconset/AppStore.png new file mode 100644 index 0000000..68de8aa Binary files /dev/null and b/UnsplashApiApp/Assets.xcassets/AppIcon.appiconset/AppStore.png differ diff --git a/UnsplashApiApp/Assets.xcassets/AppIcon.appiconset/Contents.json b/UnsplashApiApp/Assets.xcassets/AppIcon.appiconset/Contents.json index 5b0ac8d..299517e 100644 --- a/UnsplashApiApp/Assets.xcassets/AppIcon.appiconset/Contents.json +++ b/UnsplashApiApp/Assets.xcassets/AppIcon.appiconset/Contents.json @@ -78,8 +78,9 @@ "scale" : "1x" }, { - "idiom" : "ipad", "size" : "76x76", + "idiom" : "ipad", + "filename" : "icon_ipad@2x.png", "scale" : "2x" }, { @@ -89,8 +90,9 @@ "scale" : "2x" }, { - "idiom" : "ios-marketing", "size" : "1024x1024", + "idiom" : "ios-marketing", + "filename" : "AppStore.png", "scale" : "1x" } ], @@ -98,4 +100,4 @@ "version" : 1, "author" : "xcode" } -} \ No newline at end of file +} diff --git a/UnsplashApiApp/Assets.xcassets/AppIcon.appiconset/icon_ipad@2x.png b/UnsplashApiApp/Assets.xcassets/AppIcon.appiconset/icon_ipad@2x.png new file mode 100644 index 0000000..1f72fb2 Binary files /dev/null and b/UnsplashApiApp/Assets.xcassets/AppIcon.appiconset/icon_ipad@2x.png differ diff --git a/UnsplashApiApp/Global/Colors.swift b/UnsplashApiApp/Global/Colors.swift index 5a6f1d5..ea58af5 100644 --- a/UnsplashApiApp/Global/Colors.swift +++ b/UnsplashApiApp/Global/Colors.swift @@ -8,7 +8,7 @@ import UIKit -struct Color { - static var darkGray: UIColor = UIColor(hexString: "#999999") ?? .gray - static var lightGray: UIColor = UIColor(hexString: "#EEEEEE") ?? .gray +enum Colors { + static let darkGray: UIColor = UIColor(hexString: "#999999") ?? .gray + static let lightGray: UIColor = UIColor(hexString: "#EEEEEE") ?? .gray } diff --git a/UnsplashApiApp/Global/Dependencies.swift b/UnsplashApiApp/Global/Dependencies.swift index 6cc294b..3744442 100644 --- a/UnsplashApiApp/Global/Dependencies.swift +++ b/UnsplashApiApp/Global/Dependencies.swift @@ -8,11 +8,11 @@ import Foundation +// FIXME: Extract this in some local-only config file to avoid commiting this to GitHub +private let apiKey = "40c9db0853526d8cf7d3338a9b6a14722de5ae8adb7efb83e5ea7954d4809dce" + struct Dependencies { - var session: Session + var session: Session = NetworkSession(apiKey: apiKey) + static var enviroment = Dependencies() - - init(session: Session = NetworkSession()) { - self.session = session - } } diff --git a/UnsplashApiApp/Global/Fonts.swift b/UnsplashApiApp/Global/Fonts.swift new file mode 100644 index 0000000..83f7a4f --- /dev/null +++ b/UnsplashApiApp/Global/Fonts.swift @@ -0,0 +1,13 @@ +// +// Fonts.swift +// UnsplashApiApp +// +// Created by Olivier Halligon on 03/04/2020. +// Copyright © 2020 Berta Devant. All rights reserved. +// + +import UIKit + +enum Fonts { + static let regular = UIFont(name: "HelveticaNeue", size: 16) ?? UIFont.systemFont(ofSize: 16) +} diff --git a/UnsplashApiApp/Helpers/CellStyle.swift b/UnsplashApiApp/Helpers/CellStyle.swift index 00b8b26..9079331 100644 --- a/UnsplashApiApp/Helpers/CellStyle.swift +++ b/UnsplashApiApp/Helpers/CellStyle.swift @@ -13,25 +13,18 @@ import UIKit struct CellStyle { let reuseIdentifier: String let insets: UIEdgeInsets - let defaultSize: CGSize let itemsPerRow: CGFloat } extension CellStyle { static var iphone = CellStyle(reuseIdentifier: "ImageCell", insets: UIEdgeInsets(top: 16, left: 16, bottom: 16, right: 16), - defaultSize: CGSize(width: UIApplication.shared.keyWindow?.bounds.width ?? 300, height: 300), itemsPerRow: 1) static var ipad = CellStyle(reuseIdentifier: "ImageCell", insets: UIEdgeInsets(top: 16, left: 16, bottom: 16, right: 16), - defaultSize: CGSize(width: UIApplication.shared.keyWindow?.bounds.width ?? 300, height: 300), itemsPerRow: 2) } -struct Fonts { - static var regular = UIFont(name: "HelveticaNeue", size: 16) -} - extension ImageViewState { func sizeFor(collectionWidth: CGFloat, cellStyle: CellStyle) -> CGSize { let padding: CGFloat = cellStyle.insets.left + cellStyle.insets.right diff --git a/UnsplashApiApp/Helpers/UIColor+HexInit.swift b/UnsplashApiApp/Helpers/UIColor+HexInit.swift index 24509bc..4f3b94b 100644 --- a/UnsplashApiApp/Helpers/UIColor+HexInit.swift +++ b/UnsplashApiApp/Helpers/UIColor+HexInit.swift @@ -7,35 +7,23 @@ extension UIColor { .replacingOccurrences(of: "#", with: "") var rgbValue: UInt32 = 0 - if Scanner(string: hexString).scanHexInt32(&rgbValue) { - self.init( - red: CGFloat((rgbValue & 0xFF0000) >> 16) / 255.0, - green: CGFloat((rgbValue & 0x00FF00) >> 8) / 255.0, - blue: CGFloat(rgbValue & 0x0000FF) / 255.0, - alpha: CGFloat(1.0) - ) - } else { + guard Scanner(string: hexString).scanHexInt32(&rgbValue) else { return nil } + self.init( + red: CGFloat((rgbValue & 0xFF0000) >> 16) / 255.0, + green: CGFloat((rgbValue & 0x00FF00) >> 8) / 255.0, + blue: CGFloat(rgbValue & 0x0000FF) / 255.0, + alpha: CGFloat(1.0) + ) } // Check if the color is light or dark, as defined by the injected lightness threshold. // Some people report that 0.7 is best. // A false value is returned if the lightness couldn't be determined. - func isLight(threshold: Float = 0.5) -> Bool { - let originalCGColor = self.cgColor - - // Now we need to convert it to the RGB colorspace. UIColor.white / UIColor.black are greyscale and not RGB. - // If you don't do this then you will crash when accessing components index 2 below when evaluating greyscale colors. - let RGBCGColor = originalCGColor.converted(to: CGColorSpaceCreateDeviceRGB(), intent: .defaultIntent, options: nil) - guard let components = RGBCGColor?.components else { - return false - } - guard components.count >= 3 else { - return false - } - - let brightness = Float(((components[0] * 299) + (components[1] * 587) + (components[2] * 114)) / 1000) + func isLight(threshold: CGFloat = 0.5) -> Bool { + var brightness: CGFloat = 0.0 + self.getHue(nil, saturation: nil, brightness: &brightness, alpha: nil) return (brightness > threshold) } } diff --git a/UnsplashApiApp/Helpers/UIImageView+AsyncDownload.swift b/UnsplashApiApp/Helpers/UIImageView+AsyncDownload.swift index 6b7bf03..1c5ce64 100644 --- a/UnsplashApiApp/Helpers/UIImageView+AsyncDownload.swift +++ b/UnsplashApiApp/Helpers/UIImageView+AsyncDownload.swift @@ -10,30 +10,40 @@ import UIKit import Foundation extension UIImageView { - func imageFromServerURL(_ imageURL: String, placeHolder: UIImage?) { - func setPlaceHolder() { - DispatchQueue.main.async { - self.image = placeHolder - } - } - func setImage(_ downloadedImage: UIImage) { - DispatchQueue.main.async { - self.image = downloadedImage - } - } + func setImage(fromURL imageURL: String, placeHolder: UIImage?) { self.image = nil let request = LoadAPIRequest(imageURL: imageURL) let resource = Resource(get: request) + let indicator = addLoadingIndicator() Dependencies.enviroment.session.download(resource) { imageData, _ in - guard let data = imageData, - let image = UIImage(data: data) else { - setPlaceHolder() - return + let image = imageData.map(UIImage.init(data:)) + DispatchQueue.main.async { + self.image = image ?? placeHolder + indicator.removeFromSuperview() } - setImage(image) } - - + } + + private func addLoadingIndicator() -> UIView { + let roundedSquare = UIView() + roundedSquare.backgroundColor = Colors.darkGray.withAlphaComponent(0.3) + roundedSquare.layer.cornerRadius = 20.0 + roundedSquare.layer.masksToBounds = true + self.addSubview(roundedSquare) + roundedSquare.addWidthConstraint(with: 80) + roundedSquare.addHeightConstraint(with: 80) + roundedSquare.centerXToSuperview() + roundedSquare.centerYToSuperview() + + let indicator = UIActivityIndicatorView(style: .whiteLarge) + roundedSquare.addSubview(indicator) + indicator.sizeToFit() + indicator.centerXToSuperview() + indicator.centerYToSuperview() + + indicator.startAnimating() + + return roundedSquare } } diff --git a/UnsplashApiApp/Helpers/UIView+AutoLayout.swift b/UnsplashApiApp/Helpers/UIView+AutoLayout.swift index 0a3f0ec..f22700f 100644 --- a/UnsplashApiApp/Helpers/UIView+AutoLayout.swift +++ b/UnsplashApiApp/Helpers/UIView+AutoLayout.swift @@ -23,7 +23,7 @@ enum Edge { // MARK: Pin superview extension UIView { - func pinToSuperview(edges: [Edge], constant: CGFloat = 0, priority: UILayoutPriority = UILayoutPriority.required) { + func pinToSuperview(edges: [Edge], constant: CGFloat = 0, priority: UILayoutPriority = .required) { for edge in edges { switch edge { case .top: pinToSuperviewTop(constant: constant, priority: priority) @@ -35,7 +35,7 @@ extension UIView { } @discardableResult func pinToSuperviewTop(constant: CGFloat = 0, - priority: UILayoutPriority = UILayoutPriority.required, + priority: UILayoutPriority = .required, relatedBy relation: NSLayoutConstraint.Relation = .equal) -> NSLayoutConstraint { guard let superview = self.superview else { preconditionFailure("view has no superview") @@ -45,7 +45,7 @@ extension UIView { } @discardableResult func pinToSuperviewLeft(constant: CGFloat = 0, - priority: UILayoutPriority = UILayoutPriority.required, + priority: UILayoutPriority = .required, relatedBy relation: NSLayoutConstraint.Relation = .equal) -> NSLayoutConstraint { guard let superview = self.superview else { preconditionFailure("view has no superview") @@ -55,7 +55,7 @@ extension UIView { } @discardableResult func pinToSuperviewRight(constant: CGFloat = 0, - priority: UILayoutPriority = UILayoutPriority.required, + priority: UILayoutPriority = .required, relatedBy relation: NSLayoutConstraint.Relation = .equal) -> NSLayoutConstraint { guard let superview = self.superview else { preconditionFailure("view has no superview") @@ -65,7 +65,7 @@ extension UIView { } @discardableResult func pinToSuperviewBottom(constant: CGFloat = 0, - priority: UILayoutPriority = UILayoutPriority.required, + priority: UILayoutPriority = .required, relatedBy relation: NSLayoutConstraint.Relation = .equal) -> NSLayoutConstraint { guard let superview = self.superview else { preconditionFailure("view has no superview") @@ -75,7 +75,7 @@ extension UIView { } @discardableResult func limitFromSuperviewBottom(withMinimumConstant constant: CGFloat = 0, - priority: UILayoutPriority = UILayoutPriority.required) -> NSLayoutConstraint { + priority: UILayoutPriority = .required) -> NSLayoutConstraint { guard let superview = self.superview else { preconditionFailure("view has no superview") } @@ -88,7 +88,7 @@ extension UIView { } @discardableResult func limitFromSuperviewRight(withMinimumConstant constant: CGFloat = 0, - priority: UILayoutPriority = UILayoutPriority.required) -> NSLayoutConstraint { + priority: UILayoutPriority = .required) -> NSLayoutConstraint { guard let superview = self.superview else { preconditionFailure("view has no superview") } @@ -116,28 +116,28 @@ extension UIView { extension UIView { @discardableResult func pinTop(to view: UIView, constant: CGFloat = 0, - priority: UILayoutPriority = UILayoutPriority.required, + priority: UILayoutPriority = .required, relatedBy relation: NSLayoutConstraint.Relation = .equal) -> NSLayoutConstraint { return pin(edge: .top, to:.top, of: view, constant: constant, priority: priority, relatedBy: relation) } @discardableResult func pinBottom(to view: UIView, constant: CGFloat = 0, - priority: UILayoutPriority = UILayoutPriority.required, + priority: UILayoutPriority = .required, relatedBy relation: NSLayoutConstraint.Relation = .equal) -> NSLayoutConstraint { return pin(edge: .bottom, to:.bottom, of: view, constant: constant, priority: priority, relatedBy: relation) } @discardableResult func pinLeft(to view: UIView, constant: CGFloat = 0, - priority: UILayoutPriority = UILayoutPriority.required, + priority: UILayoutPriority = .required, relatedBy relation: NSLayoutConstraint.Relation = .equal) -> NSLayoutConstraint { return pin(edge: .left, to:.left, of: view, constant: constant, priority: priority, relatedBy: relation) } @discardableResult func pinRight(to view: UIView, constant: CGFloat = 0, - priority: UILayoutPriority = UILayoutPriority.required, + priority: UILayoutPriority = .required, relatedBy relation: NSLayoutConstraint.Relation = .equal) -> NSLayoutConstraint { return pin(edge: .right, to:.right, of: view, constant: constant, priority: priority, relatedBy: relation) } @@ -153,7 +153,7 @@ extension UIView { to otherEdge: Edge, of view: UIView, constant: CGFloat = 0, - priority: UILayoutPriority = UILayoutPriority.required, + priority: UILayoutPriority = .required, relatedBy relation: NSLayoutConstraint.Relation = .equal) -> NSLayoutConstraint { guard let superview = self.superview else { preconditionFailure("view has no superview") @@ -182,19 +182,19 @@ extension UIView { extension UIView { @discardableResult func addMaxWidthConstraint(with constant: CGFloat, - priority: UILayoutPriority = UILayoutPriority.required) -> NSLayoutConstraint { + priority: UILayoutPriority = .required) -> NSLayoutConstraint { return addWidthConstraint(with: constant, priority: priority, relatedBy: .lessThanOrEqual) } @discardableResult func addMinWidthConstraint(with constant: CGFloat, - priority: UILayoutPriority = UILayoutPriority.required) -> NSLayoutConstraint { + priority: UILayoutPriority = .required) -> NSLayoutConstraint { return addWidthConstraint(with: constant, priority: priority, relatedBy: .greaterThanOrEqual) } @discardableResult func addWidthConstraint(with constant: CGFloat, - priority: UILayoutPriority = UILayoutPriority.required, + priority: UILayoutPriority = .required, relatedBy relation: NSLayoutConstraint.Relation = .equal) -> NSLayoutConstraint { translatesAutoresizingMaskIntoConstraints = false @@ -212,7 +212,7 @@ extension UIView { } @discardableResult func addHeightConstraint(with constant: CGFloat, - priority: UILayoutPriority = UILayoutPriority.required, + priority: UILayoutPriority = .required, relatedBy relation: NSLayoutConstraint.Relation = .equal) -> NSLayoutConstraint { translatesAutoresizingMaskIntoConstraints = false @@ -235,7 +235,7 @@ extension UIView { extension UIView { @discardableResult func centerYToSuperview(constant: CGFloat = 0, - priority: UILayoutPriority = UILayoutPriority.required, + priority: UILayoutPriority = .required, relatedBy relation: NSLayoutConstraint.Relation = .equal) -> NSLayoutConstraint { guard let superview = self.superview else { preconditionFailure("view has no superview") @@ -256,7 +256,7 @@ extension UIView { } @discardableResult func centerXToSuperview(constant: CGFloat = 0, - priority: UILayoutPriority = UILayoutPriority.required, + priority: UILayoutPriority = .required, relatedBy relation: NSLayoutConstraint.Relation = .equal) -> NSLayoutConstraint { guard let superview = self.superview else { preconditionFailure("view has no superview") diff --git a/UnsplashApiApp/Model/ImageViewState.swift b/UnsplashApiApp/Model/ImageViewState.swift index 2f69040..2435199 100644 --- a/UnsplashApiApp/Model/ImageViewState.swift +++ b/UnsplashApiApp/Model/ImageViewState.swift @@ -9,6 +9,12 @@ import UIKit struct ImageViewState { + struct Colors { + let imageColor: UIColor + let textColor: UIColor + let containerColor: UIColor + } + let id: String let colors: Colors let size: CGSize @@ -19,26 +25,23 @@ struct ImageViewState { let author: AuthorViewState } -extension Image { - func viewState() -> ImageViewState { - return ImageViewState(id: self.id, - colors: Colors(imageColor: UIColor(hexString: self.color)), - size: CGSize(width: self.width, height: self.height), - description: self.description, - imageSmall: self.urls.small, - imageRegular: self.urls.regular, - imageFull: self.urls.full, - author: self.user.viewModel()) +extension ImageViewState { + init(image: Image) { + self.init( + id: image.id, + colors: .init(imageColor: UIColor(hexString: image.color)), + size: CGSize(width: image.width, height: image.height), + description: image.description, + imageSmall: image.urls.small, + imageRegular: image.urls.regular, + imageFull: image.urls.full, + author: .init(author: image.user) + ) } } -struct Colors { - let imageColor: UIColor - let textColor: UIColor - let containerColor: UIColor -} -extension Colors { +extension ImageViewState.Colors { init(imageColor: UIColor?) { guard let imageColor = imageColor else { self.imageColor = .white @@ -56,8 +59,8 @@ struct AuthorViewState { let name: String } -extension Author { - func viewModel() -> AuthorViewState { - return AuthorViewState(name: self.name) +extension AuthorViewState { + init(author: Author) { + self.init(name: author.name) } } diff --git a/UnsplashApiApp/Model/SearchCategory.swift b/UnsplashApiApp/Model/SearchCategory.swift index d15e7f8..4e96a43 100644 --- a/UnsplashApiApp/Model/SearchCategory.swift +++ b/UnsplashApiApp/Model/SearchCategory.swift @@ -14,9 +14,9 @@ struct SearchCategory { } extension SearchCategory { - static var barcelona = SearchCategory(name: "Barcelona", query: "barcelona") - static var wallpaper = SearchCategory(name: "Wallpaper", query: "wallpaper") - static var architecture = SearchCategory(name: "Architecture", query: "architecture") - static var experimental = SearchCategory(name: "Experimental", query: "experimental") - static var textures = SearchCategory(name: "Textures & Patterns", query: "textures-patterns") + static let barcelona = SearchCategory(name: "Barcelona", query: "barcelona") + static let wallpaper = SearchCategory(name: "Wallpaper", query: "wallpaper") + static let architecture = SearchCategory(name: "Architecture", query: "architecture") + static let experimental = SearchCategory(name: "Experimental", query: "experimental") + static let textures = SearchCategory(name: "Textures & Patterns", query: "textures-patterns") } diff --git a/UnsplashApiApp/Network Layer/APIRequest.swift b/UnsplashApiApp/Network Layer/APIRequest.swift index df81a04..6c3d633 100644 --- a/UnsplashApiApp/Network Layer/APIRequest.swift +++ b/UnsplashApiApp/Network Layer/APIRequest.swift @@ -8,94 +8,55 @@ import Foundation -protocol APIRequest { - var components: URLComponents { get } - var queryItems: [URLQueryItem] { get } - var urlRequest: URLRequest { get } +private func makeUnsplashComponent(path: String, queryItems: [URLQueryItem] = []) -> URLComponents { + var component = URLComponents() + component.scheme = "https" + component.host = "api.unsplash.com" + component.path = path + component.queryItems = queryItems + return component } -extension APIRequest { - var urlRequest: URLRequest { - guard let urlString = components.url?.absoluteString.removingPercentEncoding, - let url = URL(string: urlString) else { - preconditionFailure("We should have a valid URL \(components.url?.absoluteString.removingPercentEncoding)") - } - var request = URLRequest(url: url) - request.setValue(accessKey, forHTTPHeaderField: "Authorization") - return request - } -} -private extension APIRequest { - var accessKey: String { - return "Client-ID 40c9db0853526d8cf7d3338a9b6a14722de5ae8adb7efb83e5ea7954d4809dce" - } +protocol APIRequest { + var components: URLComponents { get } } final class ImageAPIRequest: APIRequest { let searchParameters: SearchParameters - var queryItems: [URLQueryItem] { - return setQueryItems() - } - + var components: URLComponents { - var component = URLComponents() - component.scheme = "https" - component.host = "api.unsplash.com" - component.path = searchParameters.searchType.rawValue - component.queryItems = self.queryItems - return component - } - - init(search: SearchParameters) { - self.searchParameters = search - } - - private func setQueryItems() -> [URLQueryItem] { var queryItems: [URLQueryItem] = [] let page: String = searchParameters.page == 0 ? "1" : searchParameters.page.description queryItems.append(URLQueryItem(name: "page", value: page)) if !searchParameters.query.isEmpty { queryItems.append(URLQueryItem(name: "query", value: searchParameters.query)) } - return queryItems + return makeUnsplashComponent(path: searchParameters.searchType.rawValue, queryItems: queryItems) + } + + init(search: SearchParameters) { + self.searchParameters = search } } //Unsplash API demands that we trigger a download count when downloading a picture, this request is created to handle that // https://unsplash.com/documentation#track-a-photo-download final class DownloadAPIRequest: APIRequest { - var components: URLComponents { - var component = URLComponents() - component.scheme = "https" - component.host = "api.unsplash.com" - component.path = "/photos/\(imageId)/download" - component.queryItems = self.queryItems - return component - } - - private let imageId: String - var queryItems: [URLQueryItem] = [] - + let components: URLComponents + init(imageID: String) { - self.imageId = imageID + self.components = makeUnsplashComponent(path: "/photos/\(imageID)/download") } } //Each Image parsed from the API comes with the urls for downloading the images, we use this request to get the proper URL for downloading for each image final class LoadAPIRequest: APIRequest { - var components: URLComponents { - var component = URLComponents() - component.scheme = "https" - component.host = "images.unsplash.com" - component.path = "\(imageURL)" - component.queryItems = self.queryItems - return component - } - var queryItems: [URLQueryItem] = [] - private let imageURL: String - + let components: URLComponents + init(imageURL: String) { - self.imageURL = imageURL.replacingOccurrences(of: "https://images.unsplash.com", with: "") + var components = URLComponents(string: imageURL) + components?.queryItems = [] + self.components = components ?? URLComponents() } } diff --git a/UnsplashApiApp/Network Layer/URLSessionExtension+Load.swift b/UnsplashApiApp/Network Layer/NetworkSession.swift similarity index 52% rename from UnsplashApiApp/Network Layer/URLSessionExtension+Load.swift rename to UnsplashApiApp/Network Layer/NetworkSession.swift index 079ae39..f1544eb 100644 --- a/UnsplashApiApp/Network Layer/URLSessionExtension+Load.swift +++ b/UnsplashApiApp/Network Layer/NetworkSession.swift @@ -13,11 +13,27 @@ protocol Session { func download(_ resource: Resource, completion: @escaping (Data?, Error?) -> ()) } -class NetworkSession: URLSession, Session { +class NetworkSession: Session { + private let urlSession: URLSession = .shared + private let apiKey: String + + init(apiKey: String) { + self.apiKey = apiKey + } + + func urlRequest(from apiRequest: APIRequest) -> URLRequest { + guard let url = apiRequest.components.url else { + preconditionFailure("We should have a valid URL \(apiRequest.components.url?.absoluteString ?? "nil")") + } + var request = URLRequest(url: url) + request.setValue("Client-ID \(self.apiKey)", forHTTPHeaderField: "Authorization") + return request + } func load(_ resource: Resource, completion: @escaping (A?) -> ()) { - print("👾 resource URL \(resource.apiRequest.urlRequest.url?.absoluteString)") - URLSession.shared.dataTask(with: resource.apiRequest.urlRequest) { data, _, error in + let urlReq = urlRequest(from: resource.apiRequest) + print("👾 resource URL \(urlReq.url?.absoluteString ?? "nil")") + urlSession.dataTask(with: urlReq) { data, _, error in if let error = error { print("error while fetching data \(error)") completion(nil) @@ -27,7 +43,7 @@ class NetworkSession: URLSession, Session { } func download(_ resource: Resource, completion: @escaping (Data?, Error?) -> ()) { - URLSession.shared.dataTask(with: resource.apiRequest.urlRequest) { data, _, error in + urlSession.dataTask(with: urlRequest(from: resource.apiRequest)) { data, _, error in if let error = error { print("error while fetching data \(error)") completion(nil, error) diff --git a/UnsplashApiApp/View Controllers/ImageListViewController.swift b/UnsplashApiApp/View Controllers/ImageListViewController.swift index 40d6424..52d1496 100644 --- a/UnsplashApiApp/View Controllers/ImageListViewController.swift +++ b/UnsplashApiApp/View Controllers/ImageListViewController.swift @@ -90,7 +90,7 @@ extension ImageListViewController: UICollectionViewDelegate, UICollectionViewDat } func scrollViewDidScroll(_ scrollView: UIScrollView) { - guard scrollView.isNearBottomEdge(padding: imageCellStyle.insets.bottom) && !viewModel.isFetchingresults else { + guard scrollView.isNearBottomEdge(padding: imageCellStyle.insets.bottom) && !viewModel.isFetchingResults else { return } fetchNextPage() diff --git a/UnsplashApiApp/View Model/ImageListViewModel.swift b/UnsplashApiApp/View Model/ImageListViewModel.swift index 6ecd71b..3ad82ff 100644 --- a/UnsplashApiApp/View Model/ImageListViewModel.swift +++ b/UnsplashApiApp/View Model/ImageListViewModel.swift @@ -22,12 +22,12 @@ final class ImageListViewModel { return response.results.count } - var isFetchingresults: Bool { + var isFetchingResults: Bool { return loading } func image(at index: Int) -> ImageViewState { - return response.results[index].viewState() + return ImageViewState(image: response.results[index]) } func fetchNextPage(_ searchParameters: SearchParameters) { diff --git a/UnsplashApiApp/Views/ImageCollectionViewCell.swift b/UnsplashApiApp/Views/ImageCollectionViewCell.swift index 1b68b06..6fb2184 100644 --- a/UnsplashApiApp/Views/ImageCollectionViewCell.swift +++ b/UnsplashApiApp/Views/ImageCollectionViewCell.swift @@ -50,7 +50,7 @@ class ImageCollectionViewCell: UICollectionViewCell { } func update(with image: ImageViewState) { - imageView.imageFromServerURL(image.imageSmall, placeHolder: #imageLiteral(resourceName: "placeholder-square")) + imageView.setImage(fromURL: image.imageSmall, placeHolder: #imageLiteral(resourceName: "placeholder-square")) backgroundColor = image.colors.imageColor descriptionLabel.text = image.description hoverView.backgroundColor = image.colors.imageColor diff --git a/UnsplashApiApp/Views/ImageFullScreenView.swift b/UnsplashApiApp/Views/ImageFullScreenView.swift index 503094c..0b46c2d 100644 --- a/UnsplashApiApp/Views/ImageFullScreenView.swift +++ b/UnsplashApiApp/Views/ImageFullScreenView.swift @@ -85,7 +85,7 @@ class ImageFullScreenView: UIView { func bind(_ image: ImageViewState) { self.image = image - imageView.imageFromServerURL(image.imageRegular, placeHolder: #imageLiteral(resourceName: "placeholder-square")) + imageView.setImage(fromURL: image.imageRegular, placeHolder: #imageLiteral(resourceName: "placeholder-square")) backgroundColor = image.colors.textColor backgroundView.backgroundColor = image.colors.imageColor containerView.backgroundColor = image.colors.containerColor diff --git a/UnsplashApiApp/Views/SearchBarView.swift b/UnsplashApiApp/Views/SearchBarView.swift index ac0538f..ea3c4f1 100644 --- a/UnsplashApiApp/Views/SearchBarView.swift +++ b/UnsplashApiApp/Views/SearchBarView.swift @@ -19,11 +19,11 @@ class SearchBarView: UIView { private var searchBar: UISearchBar = { let bar = UISearchBar() bar.barStyle = .default - bar.barTintColor = Color.darkGray - bar.tintColor = Color.darkGray + bar.barTintColor = Colors.darkGray + bar.tintColor = Colors.darkGray bar.backgroundImage = UIImage() - bar.textField?.backgroundColor = Color.lightGray - bar.textField?.textColor = Color.darkGray + bar.textField?.backgroundColor = Colors.lightGray + bar.textField?.textColor = Colors.darkGray return bar }() @@ -60,7 +60,7 @@ class SearchBarView: UIView { for category in categories { let button = UIButton() button.setTitle(category.name, for: .normal) - button.setTitleColor(Color.darkGray, for: .normal) + button.setTitleColor(Colors.darkGray, for: .normal) button.addTarget(self, action: #selector(categoryButtonTapped), for: .touchUpInside) stackView.addArrangedSubview(button) } @@ -113,7 +113,7 @@ extension SearchBarView: UISearchBarDelegate { private extension UIView { func setShadowForView() { - self.layer.shadowColor = Color.darkGray.cgColor + self.layer.shadowColor = Colors.darkGray.cgColor self.layer.shadowOpacity = 1 self.layer.shadowOffset = .zero self.layer.shadowRadius = 10 @@ -122,7 +122,7 @@ private extension UIView { } func setBorder() { - self.layer.borderColor = Color.lightGray.cgColor + self.layer.borderColor = Colors.lightGray.cgColor self.layer.borderWidth = 0.2 } } @@ -134,11 +134,11 @@ private extension UISearchBar { } private extension String { - func estimateHeightForText(width: CGFloat, font: UIFont? = Fonts.regular) -> CGFloat { + func estimateHeightForText(width: CGFloat, font: UIFont = Fonts.regular) -> CGFloat { let height: CGFloat = 50 let size = CGSize(width: width, height: height) let options = NSStringDrawingOptions.usesFontLeading.union(.usesLineFragmentOrigin) - let attributes = [NSAttributedString.Key.font: font] + let attributes: [NSAttributedString.Key: Any] = [.font: font] return NSString(string: self).boundingRect(with: size, options: options, attributes: attributes, context: nil).height } diff --git a/UnsplashApiAppTests/Helpers/UIColorExtensionsTests.swift b/UnsplashApiAppTests/Helpers/UIColorExtensionsTests.swift index 9ed6667..a08e38f 100644 --- a/UnsplashApiAppTests/Helpers/UIColorExtensionsTests.swift +++ b/UnsplashApiAppTests/Helpers/UIColorExtensionsTests.swift @@ -11,16 +11,16 @@ import XCTest class UIColorExtensionsTests: XCTestCase { - func testIsLIghtReturnsTrueWhenLightColor() { + func testIsLightReturnsTrueWhenLightColor() { let image = Image.testData(color: "#d68787") - let viewModel = image.viewState() + let viewModel = ImageViewState(image: image) XCTAssertTrue(UIColor(hexString: "#d68787")!.isLight()) XCTAssertTrue(viewModel.colors.textColor == UIColor.gray) } - func testIsLIghtReturnsFalseWhenDarkColor() { + func testIsLightReturnsFalseWhenDarkColor() { let image = Image.testData(color: "#040124") - let viewModel = image.viewState() + let viewModel = ImageViewState(image: image) XCTAssertFalse(UIColor(hexString: "#040124")!.isLight()) XCTAssertTrue(viewModel.colors.textColor == UIColor.white) } diff --git a/UnsplashApiAppTests/ImageList/ImageListViewModelTests.swift b/UnsplashApiAppTests/ImageList/ImageListViewModelTests.swift index bc3078e..9207957 100644 --- a/UnsplashApiAppTests/ImageList/ImageListViewModelTests.swift +++ b/UnsplashApiAppTests/ImageList/ImageListViewModelTests.swift @@ -18,16 +18,19 @@ class ImageListViewModelTests: XCTestCase { } private func fetchForSearch(search: SearchParameters, file: StaticString = #file, line: UInt = #line) { - let session = URLSessionSpy() + let session = SessionSpy() Dependencies.enviroment.session = session let delegate = ImageListViewModelDelegateSpy() let viewModel = ImageListViewModel() - let urlString = ImageAPIRequest(search: search).urlRequest.url?.absoluteString + let queryParams = ImageAPIRequest(search: search).components.queryItems viewModel.delegate = delegate - viewModel.fetch(search) + viewModel.fetchNewQuery(search) session.assertEqual(.load) - session.assertParameterEqual(urlString!) + // order of queryParams should not matter, so let's convert them to Sets + session.assertParameterEqual(Set(queryParams!), transform: { + Set($0 as? [URLQueryItem] ?? []) + }) } } diff --git a/UnsplashApiAppTests/Network Layer/APIRequestTests.swift b/UnsplashApiAppTests/Network Layer/APIRequestTests.swift index 41b9d20..3011dae 100644 --- a/UnsplashApiAppTests/Network Layer/APIRequestTests.swift +++ b/UnsplashApiAppTests/Network Layer/APIRequestTests.swift @@ -11,22 +11,34 @@ import XCTest class APIRequestTests: XCTestCase { + // You can't base your unit test on comparing URLs because the order in query parameters order is generally irrelevant + // So both "api.foo.com?a=1&b=2" and "api.foo.com?a=2&b=1" are the same resource despite their URL not being equal + private func assertComponents(_ urlComps: URLComponents, path: String, queryItems: KeyValuePairs, + file: StaticString = #file, line: UInt = #line) { + XCTAssertEqual(urlComps.scheme, "https", file: file, line: line) + XCTAssertEqual(urlComps.host, "api.unsplash.com", file: file, line: line) + XCTAssertEqual(urlComps.path, path, file: file, line: line) + + let expectedItems: [URLQueryItem]? = queryItems.map(URLQueryItem.init) + XCTAssertEqual(urlComps.queryItems.map(Set.init), expectedItems.map(Set.init), file: file, line: line) + } + func testURLRequestIsCorrectForCuratedSearchType() { let searchParameters = SearchParameters.testData(searchType: .curated) let apiRequest = ImageAPIRequest(search: searchParameters) - XCTAssertEqual(apiRequest.urlRequest.url?.description, "https://api.unsplash.com/curated?page=0") + assertComponents(apiRequest.components, path: "/curated", queryItems: ["page": "0"]) } func testURLRequestIsCorrectForQuery() { let searchParameters = SearchParameters.testData(query: "office") let apiRequest = ImageAPIRequest(search: searchParameters) - XCTAssertEqual(apiRequest.urlRequest.url?.description, "https://api.unsplash.com/curated?query=office&page=0") + assertComponents(apiRequest.components, path: "/curated", queryItems: ["query": "office", "page": "0"]) } func testURLRquestIsCorrectForPage() { let searchParameters = SearchParameters.testData(page: 5) let apiRequest = ImageAPIRequest(search: searchParameters) - XCTAssertEqual(apiRequest.urlRequest.url?.description, "https://api.unsplash.com/curated?page=5") + assertComponents(apiRequest.components, path: "/curated", queryItems: ["page": "5"]) } func testURLRequestIsCorrectForLoading() { diff --git a/UnsplashApiAppTests/TestDoubles/URLSessionTestSession.swift b/UnsplashApiAppTests/TestDoubles/SessionSpy.swift similarity index 58% rename from UnsplashApiAppTests/TestDoubles/URLSessionTestSession.swift rename to UnsplashApiAppTests/TestDoubles/SessionSpy.swift index 5e6cec4..5cf9349 100644 --- a/UnsplashApiAppTests/TestDoubles/URLSessionTestSession.swift +++ b/UnsplashApiAppTests/TestDoubles/SessionSpy.swift @@ -9,26 +9,27 @@ import Foundation @testable import UnsplashApiApp -class URLSessionSpy: URLSession, Session, TestSpy { +class SessionSpy: Session, TestSpy { enum Method { case load case download } - - var recordedMethods: [URLSessionSpy.Method] = [] + + private var buildURLRequest: (APIRequest) -> URLRequest = NetworkSession(apiKey: "testkey").urlRequest(from:) + var recordedMethods: [SessionSpy.Method] = [] var recordedParameters: [AnyHashable] = [] func load(_ resource: Resource, completion: @escaping (A?) -> ()) { record(.load) - if let urlString = resource.apiRequest.urlRequest.url?.absoluteString { - recordParameters(urlString) + if let params = resource.apiRequest.components.queryItems { + recordParameters(params) } } func download(_ resource: Resource, completion: @escaping (Data?, Error?) -> ()) { record(.download) - if let urlString = resource.apiRequest.urlRequest.url?.absoluteString { - recordParameters(urlString) + if let params = resource.apiRequest.components.queryItems { + recordParameters(params) } } } diff --git a/UnsplashApiAppTests/TestDoubles/TestSpy.swift b/UnsplashApiAppTests/TestDoubles/TestSpy.swift index 7e52e35..72a9062 100644 --- a/UnsplashApiAppTests/TestDoubles/TestSpy.swift +++ b/UnsplashApiAppTests/TestDoubles/TestSpy.swift @@ -83,10 +83,11 @@ extension TestSpy { } } - func assertParameterEqual(_ parameter: AnyHashable, + func assertParameterEqual(_ parameter: T, + transform: (AnyHashable) -> T, file: StaticString = #file, line: UInt = #line) { - guard recordedParameters == [parameter] else { + guard recordedParameters.map(transform) == [parameter] else { XCTFail("Spy was expecting \(parameter) but found: \(recordedParameters) ", file: file, line: line)