Parsing Websites and transferring Data between Objects

Question

I have a DiningHall class that has the 3 properties: openingTime, closingTime, and nextMealOpeningTime, and I need to set these NSDate properties from the data from this website.

Each DiningHall object corresponds to a particular meal. For example, there is a separate "De Neve" dining hall object for breakfast, lunch, and dinner.

As an example, if I have a De Neve hall object corresponding to "Lunch", and the times were

breakfast (9-11) lunch (12-2) dinner (5-8)

then openingTime is 12pm, closingTime is 2pm, and nextMealOpeningTime is 5pm.

The general algorithm is:

given a particular meal:

for each hall

    in data, find row corresponding to hall

    find column corresponding to current meal

    parse text, convert toNSDate

    pass 3 NSDate's to DiningHall as array

---

I'm using an open source DOM parser, and below is the code for extracting the text once I obtain the correct row. Specific concerns are dealing with "Closed" meals and a better way to pass data to DiningHall class, but any optimizations will help.

- (NSArray*) getHourDataForRow:(TFHppleElement*)row Meal:(NSString*)meal {

    NSArray *meals = [row childrenWithTagName:@"td"];
    NSArray *mealNames = [NSArray arrayWithObjects:@"breakfast", @"lunch", @"dinner", nil];
    int index = [mealNames indexOfObject:meal]+1;
   TFHppleElement *cell = meals[index];
NSArray *strong = [cell childrenWithTagName:@"strong"];

NSMutableArray *retArr = [NSMutableArray array];

NSString *opening = [strong[0] firstChildWithTagName:@"text"].content;
NSDate *openingAsDate = [self dateFromString:opening];
if (openingAsDate){
    [retArr addObject:openingAsDate];
    NSString *closing = [strong[1] firstChildWithTagName:@"text"].content;
    [retArr addObject:[self dateFromString:closing]];
}
else{
    [retArr addObject:[NSNull null]];
    [retArr addObject:[NSNull null]];
}
    if (index +1 <= 3) { 

        TFHppleElement *nextCell = meals[index+1];
        NSArray *strong1 = [nextCell childrenWithTagName:@"strong"];
        NSString* nextOpening = [strong1[0] firstChildWithTagName:@"text"].content; //strong[0] is opening time, don't need closing time from next cell
        [retArr addObject:[self nextOpening.content]];
    }
    else // nextMealOpeningTime is always nil for dinner
        [retArr addObject:[NSNull null]];

    return retArr;

 }



- (NSDate*) dateFromString:(NSString*)time {

    if ([time isEqualToString:@"CLOSED"])
        return nil;

        NSArray *digits = [time componentsSeparatedByCharactersInSet:[[NSCharacterSet decimalDigitCharacterSet] invertedSet]];

        int hour = [digits[0] intValue];
        hour+= (([time rangeOfString:@"pm"].location == NSNotFound) ? 0:12) ; //add 12 for pm
        int minutes = [digits[1] intValue];
        //format nsdate
        NSCalendar *calendar = [[NSCalendar alloc] initWithCalendarIdentifier:NSGregorianCalendar];
        NSDateComponents *components = [calendar components:( NSYearCalendarUnit | NSMonthCalendarUnit | NSWeekCalendarUnit | NSDayCalendarUnit | NSHourCalendarUnit ) fromDate:[NSDate date]];
        [components setHour:hour];
        [components setMinute:minutes];
        return [calendar dateFromComponents:components];

}

In DiningHall class

- (void) setHoursFromData:(NSArray*)data {

    if (data[0] != [NSNull null] && data[1] != [NSNull null]){
    _openingTime = data[0];
    _closingTime = data[1];

    }

    if (data[2] != [NSNull null])
    _nextOpeningTime = data[2];


}

Answer 1

I think I can talk about a few naming issues and also some of the logic in your DiningHall class. I don't know if you will come back and check for an additional answer to your question, but maybe it will be helpful to someone else.

Here is the method as you have it written:

- (void) setHoursFromData:(NSArray*)data {

    if (data[0] != [NSNull null] && data[1] != [NSNull null]){
    _ openingTime = data[0];
    _closingTime = data[1];

    }

    if (data[2] != [NSNull null])
    _nextOpeningTime = data[2];


}

nhgrif already pointed out that you have irregular indentation and spacing, and that you should always use curly braces around if statements, but those things bear repeating as they greatly improve the readability of code.

That out of the way, lets talk about the method name setHoursFromData:(NSArray*)data. First, unless this is a property and you are creating a manual setter for it, it is improper naming to call the method "set...". I can see that you are setting multiple ivars inside here so I can understand why you might have chosen that name, but it is still confusing because it makes me think you are setting a property. Second thing is that from the name, it is not at all clear what the Data is or how it will be used to set the hours. It would make more sense to briefly describe what the Data is. I am no expert but I might call it something like configureHoursForOpenCloseTimes. Sure it is longer but it makes the code a lot more self documenting.

The same advice goes for getHourDataForRow, it looks like a property getter. I think this would be fine as hourDataForRow.

Now to move inside the method. When you simply access data[0] and data[1] the way you are doing, it makes it unclear exactly what is contained in that value. This makes me think that using an NSArray in this instance is not the best choice for readability. Maybe you should be passing in an object that contains the properties for openingTime, closingTime, and nextOpeningTime. You are passing them all in together, and they each are responsible for a different piece of information. This would also get rid of the possibly strange usage of NSNull here. (At least I think it seems strange?). After doing this the code would look something like:

if (times.openingTime && times.closingTime) {
    _openingTime = times.openingTime;
    _closingTime = times.closingTime;
}

I think that is much more readable. However, once you convert it to an object, there may be better ways to approach the problem as a whole.

I also don't like that nothing happens in the event that the if statements are not true. If they are not true, what happens? How is this represented on the screen? Does it say "invalid" or something? Maybe an error message should be shown? There's nothing preventing all three of the values to be true, either. So are there three spots, and when the data is null, does it just show a blank spot? None of this is apparent from the code. It makes me wonder if the old values for _openingTime and _closingTime are cleared out, or if it just shows the wrong time if the data is null.

There are more issues, but that is enough for one review.

Answer 2

For now, I will avoid commenting too much on - (NSArray*) getHourDataForRow:(TFHppleElement*)row Meal:(NSString*)meal as I think the real answer here is to look into NSXMLParser and see if that will work for you. Other than that, for now I'll simply comment that your spacing is inconsistent and you should work in cleaning that up to make the code more readable.

---

I will, however, comment on dateFromString:

- (NSDate*) dateFromString:(NSString*)time {

    if ([time isEqualToString:@"CLOSED"])
        return nil;

        NSArray *digits = [time componentsSeparatedByCharactersInSet:[[NSCharacterSet decimalDigitCharacterSet] invertedSet]];

        int hour = [digits[0] intValue];
        hour+= (([time rangeOfString:@"pm"].location == NSNotFound) ? 0:12) ; //add 12 for pm
        int minutes = [digits[1] intValue];
        //format nsdate
        NSCalendar *calendar = [[NSCalendar alloc] initWithCalendarIdentifier:NSGregorianCalendar];
        NSDateComponents *components = [calendar components:( NSYearCalendarUnit | NSMonthCalendarUnit | NSWeekCalendarUnit | NSDayCalendarUnit | NSHourCalendarUnit ) fromDate:[NSDate date]];
        [components setHour:hour];
        [components setMinute:minutes];
        return [calendar dateFromComponents:components];

}

First of all, you left brackets off of your if statement. This in combination with the awkward indentation leaves the code quite confusing. Despite being optional, curly braces should never be omitted.

The rest of the method however.... can be completely replaced. There's a class called NSDateFormatter which is made specifically for converting NSDate objects to NSString objects and vice versa.

First, we need this link. This lets us know what our date format string should look like. We have hours without a leading zero and in 12-hour format, minutes, we have minutes, and we have an am/pm symbol.

So our format string will look like this:

@"h:mma"

Our AM symbol is @"am" and our PM symbol is @"pm". Now we have all the information we need to create our NSDateFormatter object:

- (NSDate*) dateFromString:(NSString*)time {
    if ([time isEqualToString:@"CLOSED"]) {
        return nil;
    } else {
        NSDateFormatter *df = [[NSDateFormatter alloc] init];
        [df setDateFormat:@"h:mma"];
        [df setAMSymbol:@"am"];
        [df setPMSymbol:@"pm"];

        return [df dateFromString:time];
    }
}