Convert TRDailyForecast to DailyForecast

[tomkowz]

No description provided.

[tomkowz]

Again something crashed, not crashing locally :/

[jakecraige]

How about capitalizing json so it's JSON?

[tomkowz]

I can capitalize it but have no opinion. I usually try to keep parameters lowercased but here is json which is always capitalized as shortcut, so, okay, let's do it.

[tomkowz]

@jakecraige I will also change JSON type to Dictionary<String, AnyObject> to keep it more Swifty.

[tomkowz]

@jakecraige one question about github, are you notified by github when I push new commit to this my pull request? Asking because don't know if I should mention you when I push some changes according to review.

[jakecraige]

What do you think about turning this into a failable initializer and doing an if-let as? for the JSON values so we can remove the explicit unwraps?

[jakecraige]

That might also allow you to get rid of the extra stuff you had to add to the tests to get it passing.

[tomkowz]

If we decide to change it into failable initializer it will require changes in the app logic. TRWeatherUpdate always enumerate from 1 to 4 and creates 3 DailyForecasts. If we start returning nil there instead of object additional safety checks will be required. I am okay for now to change this explicit unwrap into if-let but I would like to keep initializer not failable because I don't know the app logic on controllers level where RAC is.

EDIT: I know for what DailyForecast objects are used now.

Let's discuss it.

1. What if something goes wrong and view DailyForecast cannot be created. Do we want to display then 2 or just 1 DailyForecasts ?
2. Can we assume that DailyForecast is always created and there is no case when something goes wrong? Then we can use only if-let and initializer will be always returning some objects.

[tomkowz]

I do not like how failable initializers (not) work with classes. The code gets messy so far. You have to fill all stored properties before returning nil. Example for code "improvement".

@objc class DailyForecast: NSObject {
    let date: NSDate
    let conditionsDescription: String
    let highTemperature: Temperature
    let lowTemperature: Temperature

    init?(JSON: Dictionary<String, AnyObject>) {
        if let time = JSON["time"] as? NSTimeInterval {
            self.date = NSDate(timeIntervalSince1970: time)
        } else {
            self.date = NSDate()
        }

        if let conditionsDescription = JSON["icon"] as? String {
            self.conditionsDescription = conditionsDescription
        } else {
            self.conditionsDescription = ""
        }

        if let temperatureMax = JSON["temperatureMax"] as? Int {
            self.highTemperature = Temperature(fahrenheitValue: temperatureMax)
        } else {
            self.highTemperature = Temperature(fahrenheitValue: 0)
        }

        if let temperatureMin = JSON["temperatureMin"] as? Int {
            self.lowTemperature = Temperature(fahrenheitValue: temperatureMin)
        } else {
            self.lowTemperature = Temperature(fahrenheitValue: 0)
        }

        super.init()
        if self.dynamicType.validate(JSON) == false { return nil }
    }

    class func validate(JSON: Dictionary<String, AnyObject>) -> Bool {
        // json validation goes here
        // check if all necessary values are inside
        return false
    }
}

What do you think about it @jakecraige? Can we keep it as is for now and think about it more later when we will be able e.g. to convert DailyForecast to be struct instead of class (when all classes that uses DailyForecast are written in Swift)? Then we can implement failable intializer even simpler without filling stored properties. It works different for structs.

[jakecraige]

Great points.

What if something goes wrong and view DailyForecast cannot be created. Do we want to display then 2 or just 1 DailyForecast?

That's what I'm thinking. After some playing around to get one to not show up I have it looking like this which seems reasonable.

Can we assume that DailyForecast is always created and there is no case when something goes wrong? Then we can use only if-let and initializer will be always returning some objects.

I don't think we can. I haven't seen a case where it doesn't work yet but it's always a possibility since we're relying on a third party API for the data. With that in mind we should definitely have some sort of fallback. I'm thinking the easiest way to handle this would be to use the null-object pattern. When it fails we can provide a null-object and it will render empty like in the screenshot I posted earlier.

I was originally considering making the values optional but that's not really great because there's no case where this object is valid if it's missing any of them. We can't display it. With that in mind I'm thinking there should be a NullDailyForecast class that returns nil values for everything. This way we don't have to add lots of conditional logic or mess with any RAC stuff.

After some playing around I got a diff that looks like this, still needs the failable initializer stuff:

diff --git a/Tropos/Models/DailyForecast.swift b/Tropos/Models/DailyForecast.swift
index 0075d48..99c5f25 100644
--- a/Tropos/Models/DailyForecast.swift
+++ b/Tropos/Models/DailyForecast.swift
@@ -13,3 +13,10 @@ import Foundation
         self.lowTemperature = Temperature(fahrenheitValue: JSON["temperatureMin"] as! Int)
     }
 }
+
+@objc class NullDailyForecast: NSObject {
+    let date: NSDate? = .None
+    let conditionsDescription: String? = .None
+    let highTemperature: Temperature? = .None
+    let lowTemperature: Temperature? = .None
+}
diff --git a/Tropos/Models/TRWeatherUpdate.m b/Tropos/Models/TRWeatherUpdate.m
index 56ce074..dadecd5 100644
--- a/Tropos/Models/TRWeatherUpdate.m
+++ b/Tropos/Models/TRWeatherUpdate.m
@@ -54,6 +54,10 @@

     for (NSUInteger index = 1; index < 4; index++) {
         DailyForecast *dailyForecast = [[DailyForecast alloc] initWithJSON:currentConditionsJSON[@"daily"][@"data"][index]];
+        if (index == 3) { dailyForecast = nil; } // ONLY FOR TESTING
+        if (dailyForecast == nil) {
+            dailyForecast = (DailyForecast *)[NullDailyForecast new];
+        }
         [dailyForecasts addObject:dailyForecast];
     }

diff --git a/Tropos/ViewModels/TRDailyForecastViewModel.m b/Tropos/ViewModels/TRDailyForecastViewModel.m
index bb068a7..2bbd4db 100644
--- a/Tropos/ViewModels/TRDailyForecastViewModel.m
+++ b/Tropos/ViewModels/TRDailyForecastViewModel.m
@@ -30,19 +30,31 @@
     return [dateFormatter stringFromDate:self.dailyForecast.date];
 }

-- (UIImage *)conditionsImage
+- (nullable UIImage *)conditionsImage
 {
-    return [UIImage imageNamed:self.dailyForecast.conditionsDescription];
+    NSString *conditionsDescription = [NSString stringWithFormat:@"%@", self.dailyForecast.conditionsDescription];
+    return [UIImage imageNamed:conditionsDescription];
 }

-- (NSString *)highTemperature
+- (nullable NSString *)highTemperature
 {
-    return [[TRTemperatureFormatter new] stringFromTemperature:self.dailyForecast.highTemperature];
+    return [self stringFromTemperature:self.dailyForecast.highTemperature];
 }

-- (NSString *)lowTemperature
+- (nullable NSString *)lowTemperature
 {
-    return [[TRTemperatureFormatter new] stringFromTemperature:self.dailyForecast.lowTemperature];
+    return [self stringFromTemperature:self.dailyForecast.lowTemperature];
+}
+
+#pragma mark - Private
+
+- (NSString *)stringFromTemperature:(Temperature *)temperature
+{
+    if (temperature == nil) {
+        return @"";
+    } else {
+        return [[TRTemperatureFormatter new] stringFromTemperature:self.dailyForecast.lowTemperature];
+    }
 }

 @end

Thoughts?

[tomkowz]

Hey @jakecraige, any updates? :)

[tomkowz]

Hey @jakecraige and @gfontenot - is that possible to discuss this topic more in real time so we can decide about preferred solution faster? Some Slack, Skype, Hangouts, Messages or something?

[gfontenot]

Sorry we're not more responsive. I appreciate you putting so much effort into this, and feel bad that we're blocking you. We don't actually work on this Mon - Thurs (we're on client work), so during the week we're limited to jumping in on our free time. We should be more responsive on Friday, which is when we have time to do non-client work.

[tomkowz]

Ahh, okay I see. Let's wait then for you guys.

[tomkowz]

Any updates? :)

[jakecraige]

@tomkowz Typically we try to comment on each individual review and I will get notified of that. I don't for pushes though. If you are replying to a comment about it, no need, if you aren't then you can ping me in a new comment.

Also, I remembered we have a whole guide on code review that explains some things the things I've re-typed before here at thoughtbot/guides.

[tomkowz]

thanks for this guide!

[gfontenot]

Can we put spaces before the pointers? NSDictionary *, NSNumber *, etc.

[tomkowz]

I would like not doing this. TRWeatherUpdateCacheSpec will be removed in next pull request and replaced with WeatherUpdateCacheSpec written in Swift.

[gfontenot]

Gotcha. I'd rather that we maintain a clean code base wherever possible. That means, in this case, keeping the existing code style for new code even if it's going to be replaced soon.

[hyperspacemark]

I would like not doing this.

And I'd prefer that we didn't merge code that doesn't conform to the style guide, even if it will be removed eventually.

[jakecraige]

^ said style guide here: https://github.com/thoughtbot/guides/tree/master/style/objective_c

I don't think it directly mentions the pointers but the sample has them.

[tomkowz]

Okay, thanks for the guide. I'll improve the code then.

[tomkowz]

IMO this should looks like this:

NSDictionary* (^weatherConditionsWithTemperature) (NSNumber *) = ^NSDictionary* (NSNumber *temp) {

Do you agree? I don't think that asterisk should be placed next to the opening bracket. (Only NSNumber corrected).

I see the same pattern in untouched code:

CLPlacemark* (^stubbedPlacemark) () = ^CLPlacemark* {

[gfontenot]

Hey, sorry, there should always be a space before the pointer, never one after. If we didn't do that elsewhere it slipped through code review. I'd write that line as

NSDictionary *(^weatherConditionsWithTemperature) (NSNumber *) = ^NSDictionary *(NSNumber *temp) {

[tomkowz]

Okay, improved.

[gfontenot]

Is it possible to mark this as final? Or is that disallowed by the @objc declaration?

[tomkowz]

Yes, it is. Marked.

[gfontenot]

Great, I legitimately wasn't sure. We should do this going forward. Making sure we're not subclassing will make the transition to structs easier later.

[tomkowz]

Sure, will do, this will also make app faster by reducing dynamic dispatch.