15
\$\begingroup\$

I am contemplating working with a development firm, and had asked for this piece of sample code. Could you please take a look and let me know if this is a quality piece of work, or if it needs improvement?

// textures pvr

-(void)startWaitAnimation
{
    if(![self numberOfRunningActions] && self.visible)
    {
        [super startWaitAnimation];
        [[CCTextureCache sharedTextureCache] addImageAsync:@"lion_wait2.pvr.ccz" target:self selector:@selector(_startWaitAnimation)];
    }
}

-(void)_startWaitAnimation
{

    CCTexture2D *texture = [[CCTextureCache sharedTextureCache] textureForKey:@"lion_wait2.pvr.ccz"];
    [[CCSpriteFrameCache sharedSpriteFrameCache] addSpriteFramesWithFile:@"lion_wait2.plist" texture:texture];

    NSMutableArray *waitFrames = [NSMutableArray array];

    for(int i = 1; i <= 19; i++) {

        i = ( i%2 == 0 && i!=19 ) ? i+1 : i;
        [waitFrames addObject:[[CCSpriteFrameCache sharedSpriteFrameCache] spriteFrameByName:[NSString stringWithFormat:@"lev_wait2%04d.png", i]]];

    }

    CCAnimation *waitAnimation = [CCAnimation animationWithFrames:waitFrames delay:kWaitAnimationTime*2];

    CCAnimate *tempWaitAction =  [CCAnimate actionWithAnimation:waitAnimation restoreOriginalFrame:YES] ;

    CCRepeat *re = [CCRepeat actionWithAction:tempWaitAction times:2];
    CCSequence *seq = [CCSequence actions:re,[CCCallFunc actionWithTarget:self selector:@selector(releaseWaitFrames)], nil];

    [self runAction:seq];

    //CCRepeatForever *repeat = [CCRepeatForever actionWithAction: [CCAnimate actionWithAnimation:animation restoreOriginalFrame:NO]]
}

-(void)releaseWaitFrames
{
    [[CCSpriteFrameCache sharedSpriteFrameCache] removeSpriteFramesFromFile:@"lion_wait2.plist"];
    [[CCTextureCache sharedTextureCache] removeTextureForKey:@"lion_wait2.pvr.ccz"];
    [self setDisplayFrame:animal];
}


//particles

-(void)firHeaded
{
    CGSize s = [[CCDirector sharedDirector] winSize];

    CCTexture2D *fir_texture = [[CCTextureCache sharedTextureCache] addImage:@"fir_glow.png"];
    self.fir.texture = fir_texture;

    for (CCParticleSystem *p in effects) {
        [p removeFromParentAndCleanup:YES];
    }

    [effects release];

    effects = [[NSMutableArray alloc] init];

    for (int y = 60; y < 900; y+=20) {

        CGFloat x1 = [FirToyView fy1:s.height - y];
        CGFloat x2 = [FirToyView fy2:s.height - y];
        CCParticleSystem  *particle = [self getStars:CGRectMake(x1, y, x2-x1 , 20 )]; 
        [effects addObject:particle];
        [self addChild:particle];
    }

    if(!isTextEnabled)
    {
        textBtn.isEnabled = YES;
        isTextEnabled = YES;
    }

    [[AudioEngine sharedEngine] playEffect:kSoundCool];
    //[[AudioEngine sharedEngine] playEffect:kSoundMenuClick];

}



-(CCParticleSystem*)getStars:(CGRect)rect
{
    float mul = (rect.size.width * rect.size.height) / 11815.0f;

    int maxcount = roundf( mul*10 );

    CCParticleSystem *particle=[[[CCParticleSystemQuad alloc] initWithTotalParticles:96] autorelease];

    CCTexture2D *texture=[[CCTextureCache sharedTextureCache] addImage:@"stars_t.png"];
    particle.texture=texture;
    particle.emissionRate=19.20;
    particle.angle=210.0;
    particle.angleVar=30.0;
    ccBlendFunc blendFunc={GL_SRC_ALPHA,GL_ONE};
    particle.blendFunc=blendFunc;
    particle.duration=-1.00;
    particle.emitterMode=kCCParticleModeGravity;
    ccColor4F startColor={1.00,0.75,0.23,1.00};
    particle.startColor=startColor;
    ccColor4F startColorVar={0.15,0.00,0.00,0.44};
    particle.startColorVar=startColorVar;
    ccColor4F endColor={0.98,1.00,0.82,1.00};
    particle.endColor=endColor;
    ccColor4F endColorVar={0.11,0.12,0.12,0.18};
    particle.endColorVar=endColorVar;
    particle.startSize=15.00;
    particle.startSizeVar=5.00;
    particle.endSize=20.00;
    particle.endSizeVar=5.00;
    particle.gravity=ccp(0.00,0.00);
    particle.radialAccel=0.00;
    particle.radialAccelVar=0.00;
    particle.speed= 0;
    particle.speedVar= 0;
    particle.tangentialAccel= 0;
    particle.tangentialAccelVar= 0;
    particle.totalParticles = maxcount;
    particle.life=0.50;
    particle.lifeVar=3.00;
    particle.startSpin=0.00;
    particle.startSpinVar=0.00;
    particle.endSpin=0.00;
    particle.endSpinVar=0.00;
    particle.position=ccp(rect.origin.x + rect.size.width/2, rect.origin.y + rect.size.height/2);
    particle.posVar=ccp(rect.size.width/2,rect.size.height/2);

    return particle;
}


//animations

-(void)rectoreWorm:(CGPoint)location
{
    touchOffset = 0.0;

    float distance = -rod.position.y;

    NSLog(@"Pos %f - %f",worm.position.y,worm.position.y + touchOffset);

    float mul =  (distance > 0)? 1 + (distance / 768) * 2 : 1 + (-distance / 768) *2;

    NSLog(@"mul = %f, distance %f ",mul, distance);

    CGPoint pos1 = CGPointMake(rodHomePosition.x, rodHomePosition.y + distance/3 );

    CCSequence *action =  [CCSequence actions:[CCMoveTo actionWithDuration:0.2*mul position:pos1],[CCMoveTo actionWithDuration:0.1*mul position:rodHomePosition], nil];
    [rod runAction:action];

    CCTexture2D *texture = [[CCTextureCache sharedTextureCache] addImage:@"worm.png"];
    CCSpriteFrame *frame = [CCSpriteFrame frameWithTexture:texture rect:worm.textureRect];
    [worm setDisplayFrame:frame];

    //ccDrawPoint(wormHomePosition);


    if(distanse > 240 )
    {
        [delegate changeLevel];
        [[AudioEngine sharedEngine] playEffect:kUpSound];
    }

}


//rotation with finger

-(void)ccTouchMoved:(UITouch *)touch withEvent:(UIEvent *)event
{
    CGPoint firstLocation = [touch previousLocationInView:[touch view]];
    CGPoint location = [touch locationInView:[touch view]];

    CGPoint touchingPoint = [[CCDirector sharedDirector] convertToGL:location];
    CGPoint firstTouchingPoint = [[CCDirector sharedDirector] convertToGL:firstLocation];

    CGPoint firstVector = ccpSub(firstTouchingPoint, wheel.position);
    CGFloat firstRotateAngle = -ccpToAngle(firstVector);
    CGFloat previousTouch = CC_RADIANS_TO_DEGREES(firstRotateAngle);

    CGPoint vector = ccpSub(touchingPoint, wheel.position);
    CGFloat rotateAngle = -ccpToAngle(vector);
    CGFloat currentTouch = CC_RADIANS_TO_DEGREES(rotateAngle);

    CGFloat diff = currentTouch - previousTouch;
    CGFloat rotation = wheel.rotation + currentTouch - previousTouch;

    if(rotation > 180)
    {
        int normal =  (int) ( rotation / 360 );
        rotation -= normal*360;
        rotation -= 360;
    }
    else if( rotation < -180 )
    {
        int normal =  (int) ( rotation / 360 );
        rotation -= normal*360;
        rotation += 360;
    }

    wheel.rotation = rotation;

    NSLog(@"ANGLE %f - %d | diff %f",wheel.rotation,normal,diff);

    float delta = wheelStartAngle - wheel.rotation;
    delta = (delta > 0) ? delta : -delta;

    if(delta > 5 && !isWheelSoundPalyed)
    {
        [[AudioEngine sharedEngine] playEffect:kWhellSound];
        isWheelSoundPalyed = YES;
    }

}

//sqlite3 native

-(DSSurvey*)getSyrvey:(NSInteger)survey_id
{
    const char *sql = "SELECT id, title, date, template FROM surveys WHERE id=? LIMIT 1";
    sqlite3_stmt *statement;

    DSSurvey *survey = [[DSSurvey alloc] init];

    if (sqlite3_prepare_v2(database, sql, -1, &statement, NULL) == SQLITE_OK) {

        int iId = sqlite3_bind_int(statement, 1, survey_id);

        while (sqlite3_step(statement) == SQLITE_ROW) {
            int primaryKey = sqlite3_column_int(statement,0);
            const unsigned char *title = sqlite3_column_text(statement, 1);
            const unsigned char *date = sqlite3_column_text(statement, 2);
            const unsigned char *templ = sqlite3_column_text(statement, 3);

            survey._id = primaryKey;
            survey.title = (title) ? [NSString stringWithUTF8String:title] : @"";
            survey.date = (date) ? [NSString stringWithUTF8String: date] : @"";
            survey._template = (templ) ? [NSString stringWithUTF8String: templ] : @"";

        }
    }

    sqlite3_finalize(statement);

    return (survey._id) ? [survey retain] : nil;
}

//sqlite3 fmdb


-(void)setAnswer:(DMAnswer*)answer forTest:(DMTest*)test withQuestion:(DMQuestion*)question;
{
    [db executeUpdate:@"INSERT OR REPLACE INTO tests_answers (id, course_id, theme_id, question_id, answer_id, is_right ,date) VALUES ( (SELECT id FROM tests_answers WHERE question_id = ?), ?, ?, ?, ?, ?, ?)" , 
     [NSNumber numberWithInt:question.quetionId], 
     [NSNumber numberWithInt: self.currentCourseId],
     [NSNumber numberWithInt:test.themeId] ,
     [NSNumber numberWithInt:question.quetionId],
     [NSNumber numberWithInt:answer.answerId],
     [NSNumber numberWithBool:answer.isRight],
     [NSDate date]];

}

//image processing

- (UIImage *)getRoundedCornerImageWithGlareEffect:(NSInteger)cornerSize borderSize:(NSInteger)borderSize {
    // If the image does not have an alpha layer, add one
    UIImage *image = [self imageWithAlpha];

    NSLog(@"%f,%f bits %lu",image.size.width,image.size.height,CGImageGetBitsPerComponent(image.CGImage));

    // Build a context that's the same dimensions as the new size
    CGContextRef context = CGBitmapContextCreate(NULL,
                                                 image.size.width,
                                                 image.size.height,
                                                 CGImageGetBitsPerComponent(image.CGImage),
                                                 0,
                                                 CGColorSpaceCreateDeviceRGB(),
                                                 kCGImageAlphaPremultipliedFirst);

    // Create a clipping path with rounded corners
    CGContextBeginPath(context);
    [self addRoundedRectToPath:CGRectMake(borderSize, borderSize, image.size.width - borderSize * 2, image.size.height - borderSize * 2)
                       context:context
                     ovalWidth:cornerSize
                    ovalHeight:cornerSize];
    CGContextClosePath(context);
    CGContextClip(context);

    // Draw the image to the context; the clipping path will make anything outside the rounded rect transparent

    UIImage *glare = [UIImage imageNamed:@"glare.png"];

    CGContextDrawImage(context, CGRectMake(0, 0, image.size.width, image.size.height), image.CGImage);
    CGContextDrawImage(context, CGRectMake(0, 0, glare.CGImage, glare.CGImage), glare.CGImage);

    // Create a CGImage from the context
    CGImageRef clippedImage = CGBitmapContextCreateImage(context);
    CGContextRelease(context);

    // Create a UIImage from the CGImage
    UIImage *roundedImage = [UIImage imageWithCGImage:clippedImage];
    CGImageRelease(clippedImage);

    return roundedImage;
}

-(void)getCard:(SEL)callback
{
    //flashView.alpha = 1.0f;

    [[SimpleAudioEngine sharedEngine] playEffect:kSoundCamera];

    [[CCDirector sharedDirector].openGLView addSubview:flashView];

    [self hideMenu];

    [UIView animateWithDuration:0.03

                          delay: 0.0

                        options: UIViewAnimationOptionCurveEaseIn | UIViewAnimationOptionBeginFromCurrentState

                     animations:^{

                         flashView.alpha = 1.0f;

                     }


                     completion:^(BOOL finished){

                         CGSize s = [[CCDirector sharedDirector] winSize];

                         UIImage *img2 =[self screenhot];
                         //UIImage *img3 =[UIImage imageNamed:@"frame.png"];



                         UIGraphicsBeginImageContext(s);
                         [img2 drawInRect:CGRectMake(0,0,s.width,s.height)];
                         [transitView.layer renderInContext:UIGraphicsGetCurrentContext()];
                         //[img3 drawInRect:CGRectMake(0,0,s.width,s.height)];
                         self.snapshoot = UIGraphicsGetImageFromCurrentImageContext();
                         UIGraphicsEndImageContext();

                         [self showMenu];


                         [UIView animateWithDuration:0.5

                                               delay: 0.0

                                             options: UIViewAnimationOptionCurveEaseIn | UIViewAnimationOptionBeginFromCurrentState

                                          animations:^{

                                              flashView.alpha = 0.0f;

                                          }


                                          completion:^(BOOL finished){

                                              [flashView removeFromSuperview];
                                              [self performSelector:callback];

                                          }]; 

                     }];    
}
\$\endgroup\$
15
\$\begingroup\$
for(int i = 1; i <= 19; i++) {

    i = ( i%2 == 0 && i!=19 ) ? i+1 : i;
    [waitFrames addObject:[[CCSpriteFrameCache sharedSpriteFrameCache] 
        spriteFrameByName:[NSString stringWithFormat:@"lev_wait2%04d.png", i]]];

}

Holy Toledo.

First of all, if you must update the iterator within a for loop, let's leave the update statement empty:

for (int i = 1; i <= 19; /*updated within loop*/) {

As written, your loop misleadingly suggests we'll go from 1 to 19, run 19 times exactly, in order, and that's it. Then we sometimes increment in the loop...

But even that's not necessary. Honestly, all your ternary operator seems to be doing is ensuring that i is an odd number. There's a much better way to do this:

for (int i = 1; i <= 19; i += 2) {

Now we're incrementing by two on each loop. The end result is identical (almost). The readability is a million times better. The ternary is completely removed.

(Technically, the end result is better because now we'll add 2 instead of adding 1 and then adding 1 again.)

Now, there's something else important to note. In the body of that loop, all this nesting just hampers the readability. Unnesting these method calls doesn't negatively impact performance.

So the final form of the loop should look something more like this:

for (int i = 1; i <= 19; i += 2) {
    NSString *spriteName = [NSString stringWithFormat:@"lev_wait2%04d.png",i];
    CCSpriteFrameCache *cache = [CCSpriteFrameCache sharedSpriteFrameCache];

    [waitFrames addObject:cache spriteFrameByName:spriteName];
}
\$\endgroup\$
12
\$\begingroup\$

I observe that:

  1. Only one function has any comments
  2. There is a fair amount of commented out code
  3. The variable names aren't very helpful
  4. Somethings are just misspelled: getSyrvey, rectoreWorm
  5. The code seems to mix various things together (drawing/particle system/database access)

There is also this nugget:

  for(int i = 1; i <= 19; i++) {

        i = ( i%2 == 0 && i!=19 ) ? i+1 : i;
        [waitFrames addObject:[[CCSpriteFrameCache sharedSpriteFrameCache] spriteFrameByName:[NSString stringWithFormat:@"lev_wait2%04d.png", i]]];

    }

Have fun figuring out the logic for i in that loop.

Keep in mind its easy to go through code and find things to complain about. I'm probably guilty of much of the same stuff in my iOS code. (I blame objective-C, but that's another matter). But certainly, there are things that could use improvement.

\$\endgroup\$
7
\$\begingroup\$
[UIView animateWithDuration:0.03
                      delay: 0.0
                    options: UIViewAnimationOptionCurveEaseIn | 
                             UIViewAnimationOptionBeginFromCurrentState
                 animations:^{
                     flashView.alpha = 1.0f;
                 } completion:^(BOOL finished){
                     CGSize s = [[CCDirector sharedDirector] winSize];
                     UIImage *img2 =[self screenhot];

                     UIGraphicsBeginImageContext(s);
                     [img2 drawInRect:CGRectMake(0,0,s.width,s.height)];
                     [transitView.layer renderInContext:UIGraphicsGetCurrentContext()];

                     self.snapshoot = UIGraphicsGetImageFromCurrentImageContext();
                     UIGraphicsEndImageContext();
                     [self showMenu];

                     [UIView animateWithDuration:0.5
                                           delay: 0.0
                                         options: UIViewAnimationOptionCurveEaseIn | 
                                                  UIViewAnimationOptionBeginFromCurrentState
                                      animations:^{
                                          flashView.alpha = 0.0f;
                                      } completion:^(BOOL finished){
                                          [flashView removeFromSuperview];
                                          [self performSelector:callback];
                                      }]; 
                 }]; 

This is an absolute mess, which I've already cleaned up slightly for the sake of the readability of my own answer...

The core problem with this is the fact that within a method, we're calling a method which takes two blocks as arguments, and within one of those blocks, we're calling another method which takes two blocks as arguments. We want to indent to keep things readable, but by the time we indent enough for the deepest nested blocks, we type two letters and get word-wrapped or go off the edge! Now I realize that we'd be coding on an edit area much wider than what code review offers, so you'd have more space, but we should still avoid nesting so deeply.

In this case, there's actually a pretty simple solutions. The inner animateWithDuration call has a single purpose, and that is to undo the animation that was done in the first block. Rather than nest this, we can just use the delay argument, unnest, and give the formerly inner block a delay that is equivalent to the other animation's duration.

But we can still keep these nested without being so ugly if we want. We just need to know a little bit more about Objective-C code blocks.

There are two types of code blocks in this snippet.

The first kind is the animate block, which takes no arguments and returns no values.

The second kind is the completion block, which takes a BOOL value (indicating whether or not the animation actually animated) and returns no value.

We can create variables to hold references to block (just as the method this code resides in takes a SEL argument (a reference to a selector).

First, let's create the animation blocks, as they're simple. It looks something like this:

void (^showFlashView)(void) = ^{
    flashView.alpha = 1.0f;
}

void (^hideFlashView)(void) = ^{
    flashView.alpha = 0.0f;
}

Simple, right?

The completion blocks which take a BOOL argument are only slightly more complex:

void (^lastCompletionBlock)(BOOL) = ^(BOOL finished) {
    [flashView removeFromSuperview];
    [self performSelector:callback];
}

We have to create the last one first, because the first one needs the last one:

void (^firstCompletionBlock)(BOOL) = ^(BOOL finished) {
    CGSize s = [[CCDirector sharedDirector] winSize];
    UIImage *img2 = [self screenhot];

    UIGraphicsBeginImageContext(s);
    [img2 drawInRect:CGRectMake(0,0,s.width,s.height)];
    [transitView.layer renderInContext:UIGraphicsGetCurrentContext()];

    self.snapshoot = UIGraphicsGetImageFromCurrentImageContext();
    UIGraphicsEndImageContext();
    [self showMenu];

    [UIView animateWithDuration:0.5
                          delay:0.0
                        options:UIViewAnimationOptionCurveEaseIn |
                                UIViewAnimationOptionBeginFromCurrentState
                     animations:hideFlashView
                     completion:lastCompletionBlock];
}

And now that we've defined our blocks, our actual code looks like this:

[UIView animateWithDuration:0.03
                      delay:0.0
                    options:UIViewAnimationOptionCurveEaseIn | 
                            UIViewAnimationOptionBeginFromCurrentState
                 animations:showFlashView
                 completion:firstCompletionBlock];

The total number of lines of code is very slightly increased, however the readability is drastically improved.

\$\endgroup\$

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.