Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a method that contains a bunch of NSDictionaries (in fact, the only reason I have that method is to create those NSDictionaries). While I don't believe there are enough NSDictionaries initialized to visibly slow the code down as of yet, that is a good possibility in the future. I am currently just creating all the NSDictionaries separately, but is there a better way to do it while keeping its readability?

Here is my code:

- (void)items {

    NSDictionary *item1 = @{
    @"label"     : @"item1",
    @"icon"      : [UIImage imageNamed:@"item1-Icon"],
    @"identifier": @"item1"};

    NSDictionary *item2 = @{
    @"label"     : @"item2",
    @"icon"      : [UIImage imageNamed:@"item2-Icon"],
    @"identifier": @"item2"};

    NSDictionary *item3 = @{
    @"label"     : @"item3",
    @"icon"      : [UIImage imageNamed:@"item3-Icon"],
    @"identifier": @"item3"};

    NSDictionary *item4 = @{
    @"label"     : @"item4",
    @"icon"      : [UIImage imageNamed:@"item4-Icon"],
    @"identifier": @"item4"};

    items = [ @[item1, item2, item3, item4] mutableCopy];

}
share|improve this question
    
Maybe this is sort of artificial (as perhaps you've simplified the code to show it here), but you have an ivar named items and a method named items. I would normally consider that a getter. Here, though, you're setting the items ivar, but not returning it (this method is void). If you want lazy initialization, then I'd make it -(NSMutableArray*) items;, and if the content is not dynamic, then check whether the ivar items is initialized yet, and if so, don't do it again. –  Nate Apr 20 '13 at 22:55
add comment

2 Answers

«Two or more, use a for»
— Edsger W. Dijkstra

Okay, I am not using a for-loop, but an enumeration

NSArray *itemArray = @[
    @[@"item1",@"item1-Icon",@"item1"],
    @[@"item2",@"item2-Icon",@"item2"],
    @[@"item3",@"item3-Icon",@"item3"],
    @[@"item4",@"item4-Icon",@"item4"]
];

NSMutableArray *items = [NSMutableArray array];
[itemArray enumerateObjectsUsingBlock:^(NSArray *itemProperties, NSUInteger idx, BOOL *stop) {
    [items addObject: @{ @"label":  itemProperties[0],
                         @"icon":  [UIImage imageNamed:itemProperties[1]],
                         @"identifier":  itemProperties[2]
      }];
}];
share|improve this answer
    
This declaration of itemArray is still prone to cut and paste errors, especially as the list grows. Better to use a loop as in the other answer. –  Nate Apr 20 '13 at 22:46
    
in the end we dont know where the names come from. if they are as regular as in the example, they can be created as in the other answer. it they must be read for some source it wont work. –  vikingosegundo Apr 20 '13 at 23:53
    
You can only review the code you're given, and the code provided used a repeating pattern. If you're going to recommend a solution that's more brittle, requires more code to grow the array (relative to the other answer, that just involves changing the loop's end index), and is more verbose, in order to accomodate a different use case, that's not presented in the question, then you should document that in your answer. Otherwise, the answer is encouraging code cut-n-pasting. –  Nate Apr 21 '13 at 1:26
add comment

To avoid code redundancy, its better to put this within a loop like the one below:

for (int i = 0; i < 4; i++) {
    NSDictionary *item = @{
    @"label"     : [NSString stringWithFormat:@"item%d",i+1],
    @"icon"      : [UIImage imageNamed:[NSString stringWithFormat:@"item%d-Icon",i+1]],
    @"identifier": [NSString stringWithFormat:@"item%d",i+1]};
    [items addObject:item];
}
share|improve this answer
    
+1 for the loop, but in general, I'd advise against naming loop variables i when something more meaningful is available. itemIdx or index seems better to me. More important, though, is if the index is looping from 1 to 4, then loop from 1 to 4. Don't loop from 0 to 3, and then have to add 1 to the index every time you use it. –  Nate Apr 20 '13 at 22:48
    
Hi @Nate, I am defiantly agree with u, that loop have to be 1 to 4. But this answer saws only concept of add different NSDictionary objects to NSMutableArray. Naming convention and 1 to 4 things are besides of that, Thanks –  iBhavik Apr 22 '13 at 8:45
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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