GDC: grade my code

Discussion in 'Mac Programming' started by MrFusion, Dec 6, 2009.

  1. MrFusion macrumors 6502a

    Joined:
    Jun 8, 2005
    Location:
    West-Europe
    #1
    Hi everyone

    Since installing Snow Leopard a few weeks ago, I have been messing around with GCD.
    In this function, I am passing a sets of files to be read. The function returns immediately to avoid blocking the GUI.
    Each file is read in a synchronous queue loop, whereby the amount of progress is reported back to the delegate (i.e. the GUI) after each file has been dealt with. Maybe I should include here an option to cancel.
    The order in which the files are processed can not be predicted, but every file is processed before reporting to the GUI that everything has finished.

    Do you think I got everything right in this code? Is it correct, or did I fall into some (hidden) traps of concurrency?

    Code:
    -(void) rawDataFromFiles:(NSArray *)files error:(NSError *)outError {
    	dispatch_queue_t a_queue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_LOW, 0);
    	
    	dispatch_async(a_queue,^{ //don't wait for me
    		//NSMutableArray *results = [NSMutableArray arrayWithCapacity:[files count]];
    		//__block NSMutableArray *results = [[NSMutableArray alloc] init]; //__block prevents retain++ of results in block -> crash
    		NSMutableArray *results = [[NSMutableArray alloc] initWithCapacity:[files count]];
    		__block int progress = 0;
    		void (^my_loop)(size_t) = ^(size_t i){ //what to do within loop
    #warning todo: replace next line by file reading code 			
    			[results addObject:[NSNumber numberWithInt:i]]; //todo read data 
    			dispatch_sync(dispatch_get_main_queue(),^{ //report progress, wait for me, run on main thread
    				[delegate importProgression:progress++ userInfo:nil];
    			});
    		}; 
    
    		dispatch_apply([files count], a_queue, my_loop); //wait for me 
    		
    		dispatch_async(dispatch_get_main_queue(),^{ //finishing up, don't wait for me, run on main thread
    			[delegate importDidFinish:[NSDictionary dictionaryWithObject:results forKey:@"results"]];
    		});
    		
    		[results release];
    	});	
            outError = nil;
    }
    
     
  2. Catfish_Man macrumors 68030

    Catfish_Man

    Joined:
    Sep 13, 2001
    Location:
    Portland, OR
    #2
    Overall, looks fine. However, I do have an alternate approach that you might want to consider:

    Code:
    -(void) rawDataFromFiles:(NSArray *)files handlingResultOnQueue:(dispatch_queue_t)resultQueue withBlock:((^)(NSURL *,NSData *, NSError *))handler {	
    	dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0),^{ //don't wait for me
    		dispatch_group_t resultGroup = dispatch_group_create();
    		void (^my_loop)(NSURL *, NSUInteger, BOOL *) = ^(NSURL *file, NSUInteger idx, BOOL *stop){ //what to do within loop
    			NSError *err = nil;
    #warning todo: replace next line by file reading code 			
    			NSData *aResult = [something fileDataAtURL:file error:&err];
    			dispatch_group_async(resultGroup, resultQueue,^{ //report back with the data we just read
    				handler(file, aResult, err); //probably object lifetime issues, I haven't thought this through
    			});
    		}; 
    
    		[files enumerateObjectsWithOptions:NSEnumerationConcurrent usingBlock:my_loop]; //equivalent to dispatch_apply
    		
    		//wait until we're done processing results. Not totally clear this is necessary.
    		//removing it would change the semantics from "wait until results are processed before saying we're done" to
    		//"wait until all results are in the queue for reporting before saying we're done"
    		dispatch_group_wait(resultGroup); 
    		
    		dispatch_async(resultQueue,^{ //finishing up, don't wait for me
    			[delegate importDidFinish:[NSDictionary dictionaryWithObject:results forKey:@"results"]];
    		});
    	});
    }
    
    This has a number of potential advantages:
    *By processing results on a queue of the API clients' choice, you can make it easier to stay off the main queue (good for UI latency)
    *By handing the data off to the result handler immediately, you can overlap reading and processing
    *The NSError usage is... rather nonstandard, which is unfortunate, but it does nicely let you handle errors in a very fine-grained way.
     
  3. MrFusion thread starter macrumors 6502a

    Joined:
    Jun 8, 2005
    Location:
    West-Europe
    #3
    That is so much better than what I had. Thanks!

    You are absolutely right.

    Oh, I figured I let the user know which files could not be read. What is the best way to do this?


    Edit:
    //wait until we're done processing results. Not totally clear this is necessary.

    The data should be completely processed before it can be shown to the user. Since this takes some time, the UI starts beachballing. With GCD, this will hopefully not be the case.
     

Share This Page