5
\$\begingroup\$

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];


}
\$\endgroup\$
  • \$\begingroup\$ Have you tried using NSXMLParser? \$\endgroup\$ – nhgrif Jul 24 '14 at 22:27
  • \$\begingroup\$ What specific concerns from my current approach would it solve? \$\endgroup\$ – Mahir Jul 25 '14 at 1:44
  • \$\begingroup\$ I'm not familiar with the 3rd party parser you're using, so I can't help you much with it. There's an existing Apple class for parsing XML. I know you're trying to parse HTML, but I think XML parsing might work here. NSXMLParser is definitely efficient, and might present the data in a way that makes plenty of sense. \$\endgroup\$ – nhgrif Jul 25 '14 at 3:09
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ So I have modified my code a little since I posted the question: I decided to store all the hours at once within an Hours object \$\endgroup\$ – Mahir Aug 7 '14 at 6:23
  • \$\begingroup\$ The Hours object has a dictionary of halls as a property, of type HourHall, which is another custom class. Each hall has a dictionary of meal objects, of type HourMeal. The HourMeal has as properties an opening and closing time \$\endgroup\$ – Mahir Aug 7 '14 at 6:25
  • \$\begingroup\$ When I want a particular set of hours for a given hall and meal, I pass the parameters to the hour object and it returns an array with the desired opening/closing times. \$\endgroup\$ – Mahir Aug 7 '14 at 6:27
  • \$\begingroup\$ Would it be better if I instead passed the HourMeal object? \$\endgroup\$ – Mahir Aug 7 '14 at 6:28
  • 1
    \$\begingroup\$ Calling data[0], data[1], and data[2] without first checking the array's count is also just begging for a out of bounds exception. \$\endgroup\$ – nhgrif Aug 7 '14 at 11:19
4
\$\begingroup\$

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];
    }
}
\$\endgroup\$
  • \$\begingroup\$ Sometimes the strings have an extra space or a dash in them, for example, "5:00pm " or "- 4:23am". Do I need to eliminate these from the string before I can use the NSDateFormatter? \$\endgroup\$ – Mahir Jul 25 '14 at 1:42
  • \$\begingroup\$ for example, [time stringByReplacingOccurrencesOfString:@" " withString:@""] \$\endgroup\$ – Mahir Jul 25 '14 at 1:48
  • \$\begingroup\$ Yes. But if you parse the strings correctly, these shouldn't be here actually. \$\endgroup\$ – nhgrif Jul 25 '14 at 3:08
  • \$\begingroup\$ So I implemented the code, and I found a problem: in another part of my app, I compare the current time with the time I create from the string like this NSComparisonResult compareOpen = [[NSDate date] compare : _openingTime]; \$\endgroup\$ – Mahir Jul 26 '14 at 9:07
  • \$\begingroup\$ This no longer works because creating an nsdate with the NSDateFormatter does not set the day \$\endgroup\$ – Mahir Jul 26 '14 at 9:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.