Add swiftlint #1

mfclarke · 2018-01-22T14:23:48Z

This PR adds swiftlint (install with brew) and fixes as much as I could without doing bigger refactoring.

Hopefully without being annoying, here's some issues I noticed along the way:

If data comes back from the API in a format we don't expect, the app will likely crash. The code is force unwrapping deserialised API response model vars all over the place. This is a failure of AlamofireObjectMapper where you have to make all vars optional. We moved away from this on PlayPlex to the built in swift 4 Codable protocol. This is much nicer, you can use immutable non-optionals. Your init is throwable, so if something doesn't decode from json as you'd expect an error is thrown for you to handle. You can then just filter that model out etc.
Some force unwraps I couldn't clean up because I don't know what default to return for an Rx Observable. In ReactiveSwift you can for example return an empty SignalProducer (cold signal) which completes immediately without sending values.
There's captured self in a lot of places, and this will result in a retain cycles (memory leaks). I didn't have time to clean these up. See below though.

And a few notes, from a swift guy to an android guy 😄 :

Anytime you see a ! (force unwrap) or an as! (force cast) that's a crash waiting to happen. Always favour non-optionals where possible, or ? to safely unwrap. There's a bunch of operators to help you here:
- Optional Chaining
- Nil Coalescing (half way down)
- if let / guard let
- Probably other stuff I'm forgetting :)
Some ! are legit. Ie where you know there will always be a value but it's simpler to do it this way than fight the compiler. Or when using storyboards with @IBOutlet etc. Also where you actually do want the app to crash if something is nil or wrong type.
Watch out for captured self. Everytime you reference self in an anonymous closure it captures. You need to weakify self to break this. See ARC (about 2/3 way down)
Swift is really opinionated when it comes to naming and casing. See Swift API Design Guidelines. So the conventions are pretty different between swift and java (ie with enums).

mfclarke · 2018-01-22T14:24:40Z

ReactiveArchitecture.xcodeproj/project.pbxproj

+				"${BUILT_PRODUCTS_DIR}/RxSwift/RxSwift.framework",
+				"${BUILT_PRODUCTS_DIR}/RxBlocking/RxBlocking.framework",
+				"${BUILT_PRODUCTS_DIR}/RxTest/RxTest.framework",
+				"${BUILT_PRODUCTS_DIR}/SwiftHamcrest/Hamcrest.framework",


It's conventional to commit the result of pod install even if you don't commit the Pods folder

mfclarke · 2018-01-22T14:25:04Z

ReactiveArchitecture/Classes/core/AppDelegate/AppDelegate.swift

-    func applicationWillResignActive(_ application: UIApplication) {
-        // Sent when the application is about to move from active to inactive state. This can occur for certain types of temporary interruptions (such as an incoming phone call or SMS message) or when the user quits the application and it begins the transition to the background state.
-        // Use this method to pause ongoing tasks, disable timers, and invalidate graphics rendering callbacks. Games should use this method to pause the game.
-    }


Line length and general boilercrap cleanup

👍 - on the android side I knew the analysis too.ls to enforce this. Not the case on iOS swift...yet.

mfclarke · 2018-01-22T14:25:30Z

ReactiveArchitecture/Classes/core/error/AppError.swift

-public enum AppError : Error {
-    case RuntimeError(String)
+public enum AppError: Error {
+    case runtimeError(String)


Swift naming guidelines

mfclarke · 2018-01-22T14:25:45Z

ReactiveArchitecture/Classes/core/extension/ConnectableObservableExtension.swift

    func autoconnect() -> Observable<E> {
        return Observable.create { observer in
-            return self.do(onSubscribe: {
+            self.do(onSubscribe: {


Prefer implicit return of explicit

I bet Aziz Ansar would have loved explicit LOL!

I had no idea swift did this. However as a learning tool I'm probably going to leave it. Less WTF moments.

mfclarke · 2018-01-22T14:26:36Z

ReactiveArchitecture/Classes/core/extension/Observable+Optional.swift

+}
+
+extension Observable where Element: OptionalType {
+    func ignoreNil() -> Observable<Element.Wrapped> {


Added this for easier strong type mapping to subclasses during an observable flow. Optional map to class -> ignore nils.

Nice...not 100% sure what it does but the less manual casting the better

FYI, RxSwiftExt (a collection of RxSwift extensions from the RxSwift contributors) has an option similar to this already called .unwrap()

mfclarke · 2018-01-22T14:27:06Z

ReactiveArchitecture/Classes/nowplaying/controller/ServiceControllerImpl.swift


            return Observable.just(NowPlayingInfoImpl.init(movieInfoList: movieInfoList,
-                                                           pageNumber: serviceResonse.page!,
-                                                           totalPageNumber: serviceResonse.total_pages!))


Swift naming guidelines

mfclarke · 2018-01-22T14:27:33Z

ReactiveArchitecture/Classes/nowplaying/model/result/Result.swift

-    case FAILURE
+    case inFlight
+    case success
+    case failure


Swift naming guidelines

I'm trying to find where they talk about ENUMS. From ADA To C to Java...always been caps. Easier for readability that the code speaks about enums vs other types.

Just trusting and verifying :-)

mfclarke · 2018-01-22T14:28:15Z

ReactiveArchitecture/Classes/nowplaying/service/Results.swift

+    var popularity: Double!
+    var voteCount: Int!
+    var video: Bool!
+    var voteAverage: Double!


Force unwraps here rather than scattered throughout the code. The real solution to this is to convert to Codable but had no time

I might try that on another PR. For now this is good for educational tool

mfclarke · 2018-01-22T14:28:25Z

ReactiveArchitecture/Classes/nowplaying/service/Results.swift

-        vote_count <- map["vote_count"]
+        voteCount <- map["vote_count"]
        video <- map["video"]
-        vote_average <- map["vote_average"]


Swift naming guidelines

😲 well look at that.

mfclarke · 2018-01-22T14:29:32Z

ReactiveArchitecture/Classes/nowplaying/viewcontroller/NowPlayingTableViewController.swift

     Add passed in list to table view with animation for each entry to add
     Parameters: listToAdd - list to add
    */
-    func addAll(listToAdd: Array<MovieViewInfo>!) -> Void {


-> Void is implicit. This (and a lot of spacing and implicit return stuff) was fixed with 1 pass of swiftlint autocorrect 💯

In the beginning I didn't have the void on my functions, then started putting it in. No idea why..

👍

mfclarke · 2018-01-22T14:30:24Z

ReactiveArchitecture/Classes/nowplaying/viewcontroller/NowPlayingTableViewController.swift

-        if (movieViewInfo is MovieViewInfoImpl) {
+        if movieViewInfo is MovieViewInfoImpl {
+            // swiftlint:disable:next force_cast
            let movieCell: MovieCell = tableView.dequeueReusableCell(withIdentifier: "MovieCellIdentifier", for: indexPath) as! MovieCell


I personally don't mind force casting in this case. A crash here is basically the only sane thing to do as you won't know how the app will behave with a cell of the wrong type

mfclarke · 2018-01-22T14:31:04Z

ReactiveArchitecture/Classes/nowplaying/viewcontroller/NowPlayingViewController.swift

            nowPlayingViewModel?.processUiEvent(uiEvent: scrollEvent)
        } else {
-            if (uiModel.adapterCommandType == AdapterCommandType.ADD_DATA) {
+            if uiModel.adapterCommandType == .addData {


Didn't clean this up everywhere but I generally prefer removing the prefix where it's implicit

so if I start typing "." on Xcode it'll expand all the enum types? I guess I'll find out...
👍

mfclarke · 2018-01-22T14:31:31Z

ReactiveArchitectureTests/nowplaying/controller/ServiceControllerImplTest.swift

        assertThat(movieInfo.getPictureUrl(), matchesPattern(IMAGE_PATH + "/tnmL0g604PDRJwGJ5fsUSYKFo9.jpg", options: .caseInsensitive))
        assertThat(movieInfo.getRating(), equalTo(7.2))
+        // TODO: This fails outside of US timezones
        assertThat(movieInfo.getReleaseDate().description, equalTo("2017-03-15 04:00:00 +0000"))


I believe you can set an override timezone to NSCalendar or soemthing but didn't have a chance to do it

👍 leaving in TODO

HIFILEO · 2018-01-25T14:05:04Z

ReactiveArchitecture/Classes/core/AppDelegate/AppDelegate.swift


        let fileLogger: DDFileLogger = DDFileLogger() // File Logger
-        fileLogger.rollingFrequency = TimeInterval(60*60*24)  // 24 hours
+        fileLogger.rollingFrequency = TimeInterval(60 * 60 * 24)  // 24 hours


HIFILEO · 2018-01-25T14:05:21Z

ReactiveArchitecture/Classes/core/dip/AppDelegateModule.swift

            //App Singleton
            //
-            container.register(.singleton) { ServiceApiImpl(baseUrl: "https://api.themoviedb.org/3/movie") as ServiceApi}
+            container.register(.singleton) { ServiceApiImpl(baseUrl: "https://api.themoviedb.org/3/movie") as ServiceApi }


HIFILEO · 2018-01-25T14:05:30Z

ReactiveArchitecture/Classes/core/dip/AppDelegateModule.swift

            // VC Singleton
            //
-            container.register {serviceController in NowPlayingViewModel(serviceController: serviceController) as NowPlayingViewModel}
+            container.register { serviceController in NowPlayingViewModel(serviceController: serviceController) as NowPlayingViewModel }


HIFILEO · 2018-01-25T14:05:56Z

ReactiveArchitecture/Classes/core/error/APIError.swift


-    /// Init an error for a local client-side error. Usually this would be the NSError reported by NSURLSession, e.g. network connection not available.
+    /// Init an error for a local client-side error. Usually this would be the NSError reported by NSURLSession,
+    /// e.g. network connection not available.


HIFILEO · 2018-01-25T14:06:07Z

ReactiveArchitecture/Classes/core/extension/ArrayExtension.swift


    func indexOf(element: Element) -> Int {
-        guard let index = index(where: { $0 as AnyObject === element as AnyObject}) else { return -1 }
+        guard let index = index(where: { $0 as AnyObject === element as AnyObject }) else { return -1 }


HIFILEO · 2018-01-25T14:22:22Z

ReactiveArchitecture/Classes/nowplaying/controller/ServiceControllerImpl.swift

-            .flatMap{ (serviceResponse: ServiceResponse) -> Observable<NowPlayingInfo> in
-                return TranslateNowPlayingSubscriptionFunc.init(imageUrlPath: self.imageUrlPath).apply(serviceResonse: serviceResponse)
+            .flatMap { (serviceResponse: ServiceResponse) -> Observable<NowPlayingInfo> in
+                TranslateNowPlayingSubscriptionFunc.init(imageUrlPath: self.imageUrlPath).apply(serviceResonse: serviceResponse)


I'll probably add the return back in for learning. put some kind of note in the readme...why they are there lol

HIFILEO · 2018-01-25T14:25:22Z

ReactiveArchitecture/Classes/nowplaying/interactor/NowPlayingInteractor .swift

 import RxSwift

-protocol NowPlayingInteractor : class {
+protocol NowPlayingInteractor: class {


HIFILEO · 2018-01-25T14:25:35Z

ReactiveArchitecture/Classes/nowplaying/interactor/NowPlayingInteractorImpl.swift

-                    .catchError{ (error: Error) -> Observable<ScrollResult> in
-                        return Observable.just(ScrollResult.failure(pageNumber: scrollAction.getPageNumber(), error: error))
+                    .catchError { (error: Error) -> Observable<ScrollResult> in
+                        Observable.just(ScrollResult.failure(pageNumber: scrollAction.getPageNumber(), error: error))


same here...

HIFILEO · 2018-01-25T14:26:06Z

ReactiveArchitecture/Classes/nowplaying/interactor/NowPlayingInteractorImpl.swift

+            .flatMap { action -> Observable<ScrollAction> in
                DDLogInfo("Thread name: " + Thread.current.description + " Translate Actions into ScrollActions.")
-                return Observable.just(action as! ScrollAction);
+                return Observable.just(action)


Nice to have the extension and just do this.

HIFILEO · 2018-01-25T14:33:04Z

ReactiveArchitecture/Classes/nowplaying/model/MovieInfoImpl.swift

     */
    init(pictureUrl: String, title: String, releaseDate: Date, rating: Double) {
-        self.pictureUrl = pictureUrl;
+        self.pictureUrl = pictureUrl


kaosdg · 2018-01-25T15:43:58Z

.swiftlint.yml

+excluded: # paths to ignore during linting. overridden by `included`.
+  - Pods
+  - .git
+  - ReactiveArchitectureTests


on the components side, we allow swiftlint to lint our test code, but with custom rule sets.
Test code is still code that needs to follow standards

you're strict lol. Good to know though. I 'll leave for now.

HIFILEO · 2018-01-25T15:53:25Z

ReactiveArchitecture/Classes/nowplaying/viewcontroller/NowPlayingTableViewController.swift


            return movieCell
        } else {
+            // swiftlint:disable:next force_cast


HIFILEO · 2018-01-25T15:54:54Z

ReactiveArchitecture/Classes/nowplaying/viewcontroller/NowPlayingViewController.swift

        //
-        if (scrollDisposable != nil) {
-            return;
+        guard self.scrollDisposable == nil else {


loving this is a keyword
👍

HIFILEO · 2018-01-25T16:10:56Z

ReactiveArchitecture/Classes/nowplaying/viewmodel/NowPlayingViewModel.swift

                return (self.nowPlayingInteractor?.processAction(actions: actions))!
            })
-            .scan(initialUiModel) { (uiModel: UiModel!, result: Result!) in
+            .map { $0 as? ScrollResult }


So this I'll have to reject. Although it's interesting I'll explain why.

As far as the nil...I know RxJava 2 I can't push nulls. for RxSwift I'll leave this in with a comment.

There will be a case where we have more than one type of Result. You'll need to switch off the class type just like you do in drawing the cell view. I'll pull this and make the changes.

HIFILEO · 2018-02-06T17:36:16Z

Max - thanks for the PR. I took most of your suggestions and incorporated them in via another branch. I took 80% of the suggestions. The rest I just marked with notes so people using this example understand the underlying weaknesses. Please close this PR.

mfclarke added 5 commits January 19, 2018 15:02

Swiftlint and autocorrect

8c2811d

Fixing a bunch of swiftlint stuff

46a98e2

Trade force unwraps scattered in code for force unwraps in models

81f1425

Fix more force unwraps

7158325

Add note to fix test

09c63a1

mfclarke commented Jan 22, 2018

View reviewed changes

Last force unwrap fix

ce43aae

HIFILEO reviewed Jan 25, 2018

View reviewed changes

kaosdg reviewed Jan 25, 2018

View reviewed changes

HIFILEO reviewed Jan 25, 2018

View reviewed changes

Add swiftlint #1

Are you sure you want to change the base?

Add swiftlint #1

Uh oh!

Conversation

mfclarke commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfclarke commented Jan 22, 2018 •

edited

Loading