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 this Objective-C code for use in an iOS app that makes a 'bar' on the screen, with a red exclamation mark that flashes in it at random places.

Here's what it looks like:

enter image description here

Header file:

#import <UIKit/UIKit.h>

@interface ESFlashingErrorBar : UIView
@property (nonatomic, strong) NSArray *points;
@property (nonatomic, strong) NSString *character;
@property (nonatomic, strong) UIColor *color;
@end

Implementation:

#import "ESFlashingErrorBar.h"
#import "ESThemeManager.h"
@implementation ESFlashingErrorBar
#define kNumOfPoints    7
int lastFlash;
- (id)initWithFrame:(CGRect)frame
{
    self = [super initWithFrame:frame];
    if (self)
    {
        self.backgroundColor = [UIColor clearColor];

        CGFloat width = frame.size.width;

        CGFloat perItemWidth = width/kNumOfPoints;

        int i = 0;

        NSMutableArray *mA = @[].mutableCopy;

        while (i < kNumOfPoints)
        {
            UILabel *exPoint = [[UILabel alloc] initWithFrame:CGRectMake(perItemWidth * i, 0, perItemWidth, self.frame.size.height)];
            exPoint.backgroundColor = [UIColor clearColor];
            exPoint.textColor = [UIColor redColor];
            exPoint.textAlignment = NSTextAlignmentCenter;
            exPoint.text = @"!";
            exPoint.font = [UIFont fontWithName:exPoint.font.fontName size:27];
            exPoint.alpha = 0.1;
            [self addSubview:exPoint];
            [mA addObject:exPoint];

            i++;
        }

        self.points = mA;
    }
    [NSTimer scheduledTimerWithTimeInterval:1 target:self selector:@selector(flashPoint) userInfo:nil repeats:YES];
    return self;
}
- (void)setColor:(UIColor *)color
{
    for (UILabel *l in self.points) {
        l.textColor = color;
    }
}
- (void)setCharacter:(NSString *)character
{
    for (UILabel *l in self.points) {
        l.text = character;
    }
}
- (void)flashPoint
{
    int randomNumber = lastFlash;

    while (randomNumber == lastFlash) {
        randomNumber = arc4random() % kNumOfPoints;
    }

    lastFlash = randomNumber;

    UILabel *l = self.points[randomNumber];

    [UIView animateWithDuration:0.4 delay:0 options:kNilOptions animations:^
    {
        l.alpha = 1;
    }
    completion:^(BOOL finished) {
        [UIView animateWithDuration:0.4 animations:^{
            l.alpha = 0.1;
        }];
    }];
}
- (void)drawRect:(CGRect)rect
{
    UIBezierPath *path = [UIBezierPath bezierPath];
    [path moveToPoint:CGPointMake(0, 0)];
    [path addLineToPoint:CGPointMake(self.frame.size.width, 0)];
    [[ESThemeManager theme].navLineColor setStroke];
    [path stroke];
}
@end

ESThemeManager is a thing that manages the colors shown in the app. It returns an ESTheme object which contains a bunch of properties of colors.

I'm wondering if there's a way to make this code simpler - in particular, I don't like how I store a bunch of labels in an array, then loop through it every time I want to change something. Is this best practice?

share|improve this question

3 Answers 3

up vote 12 down vote accepted

I'm just a beginner myself but I feel like I can point out a few things in this code.

First, I would add some white space at the top of the file here:

#import "ESFlashingErrorBar.h"
#import "ESThemeManager.h"
@implementation ESFlashingErrorBar
#define kNumOfPoints    7
int lastFlash;

Instead I would do this for increased readability:

#import "ESFlashingErrorBar.h"
#import "ESThemeManager.h"

@implementation ESFlashingErrorBar

#define kNumOfPoints    7
int lastFlash;

That is a minor niggle but it's the first thing I noticed.

Next, I would not use #defines but rather do constants. If you are going to do defines, I would make the variable all capitalized, but from my understanding defines are harder to debug than using a constant like so:

static const int kNumPoints = 7

I don't know why you are defining lastFlash outside of the methods of the class. I guess just for simplicity? But it is still confusing what it is doing there and why it starts out without being assigned a value. Maybe add a comment to explain its purpose?

Many of your variable names need some help. It is not immediately clear what perItemWidth means. Nor is it clear what mA is or what exPoint means.

It is not good practice to use the dot accessors inside the initialization method, such as this:

 self.backgroundColor = [UIColor clearColor];
 self.points = mA;

Instead access the variable directly like _points. The reason for this is that the instance of the class may not be fully initialized and this can lead to unexpected behavior. However, now that I think about it, if self.backgroundColor is part of the superclass, then it is in fact acceptable to access it with the dot accessors, because when you call self = [super initWithFrame:frame];, the superclass is in fact fully initialized at that point and therefore safe.

Personally I would break out everything involving the while loop into another method, rather than doing that work in the initialization. I'm not totally sure that this is best practice, but I do think it would make the code easier to understand.

Bonus points for overriding the set: methods. It appears that this simplifies the code compared to what it would take to do this otherwise. However, when you do this, it may not be clear to someone else using the code that calling .color or .character is going to modify all of the points. Maybe just comments in the header file to indicate that this is what will happen? I might be totally wrong here though, so take that with a grain of salt.

Speaking of comments, there are absolutely none in the code. I know that the overuse of comments can be a bad thing, but I think a few comments about the overall flow of what is happening in the class would be helpful for someone else trying to understand it, especially inside of the flashPoint method.

As for making the code simpler overall, I can't think of any ways to do this. Maybe someone else will be able to provide feedback on that.

share|improve this answer
    
Are #defines really harder to debug? I can't see why this might be the case... –  Schism Jul 28 at 19:38
1  
#define YES NO ... happy debugging. Now, that's not the case here, but #define is just find and replace. And they can lead to some weird debugging issues. Apple agrees too, which is why they've eliminated #defines in swift. –  nhgrif Jul 28 at 21:33
    
I agree with the comments both on the #define and the oddity of lastFlash existing outside the class. I'd move it as a static variable within the flashPoint method. –  nhgrif Jul 28 at 21:34
@property (nonatomic, strong) NSArray *points;
@property (nonatomic, strong) NSString *character;
@property (nonatomic, strong) UIColor *color;

These three properties are all problematic.


The first is only problematic because it should be private. Move it into the class extension, in the .m file:

@interface ESFlashingErrorBar()
@property (nonatomic, strong) NSArray *points;
@end

The second and third are primarily problematic because if you try to access them to find out what the current color is, it will always return nil--the setters don't set the backing variable.

To fix this, you can add the following line to the setter method:

_character = character;

or

_color = color;

Alternatively, you can completely eliminate the back variable. This is fine to do, because we already have seven references to the backing variable. We just need to fix the setters to return that value, which can be done as such:

- (UIColor *)color {
    return [[self.points firstObject] textColor];
}

There's also this problem:

@property (nonatomic,strong) NSString *character;

What? Is it a string or a character? It appears the intended use is only individual characters, but nothing actually prevents the user from entering a very long string.

If you intend to allow the user to enter any string, the property should probably be changed to text, which matches other UIKit objects.

If however the intention is only a single character, we should change this property to a char rather than an NSString. The tricky part here is then converting from a char to NSString and back again.

@property (nonatomic,assign) char character;

First, the forin loop in the setter can (and should) remain the same. All we have to do is convert the char to an NSString, which can be done as such:

NSString *text = [NSString stringWithFormat:@"%c", character];

Now then, let's get the character back out.

- (char)character {
    NSString *text = [self.points firstObject];
    if ([text length] > 0) {
        return [text characterAtIndex:0];
    } else {
        return NULL;
    }
}
share|improve this answer

Let's talk about this snippet:

if (self) {
    self.backgroundColor = [UIColor clearColor];

    CGFloat width = frame.size.width;

    CGFloat perItemWidth = width/kNumOfPoints;

    int i = 0;

    NSMutableArray *mA = @[].mutableCopy;

    while (i < kNumOfPoints) {
        UILabel *exPoint = [[UILabel alloc] initWithFrame:CGRectMake(
                               perItemWidth * i, 
                               0, 
                               perItemWidth, 
                               self.frame.size.height
                           )];
        exPoint.backgroundColor = [UIColor clearColor];
        exPoint.textColor = [UIColor redColor];
        exPoint.textAlignment = NSTextAlignmentCenter;
        exPoint.text = @"!";
        exPoint.font = [UIFont fontWithName:exPoint.font.fontName size:27];
        exPoint.alpha = 0.1;
        [self addSubview:exPoint];
        [mA addObject:exPoint];

        i++;
    }

    self.points = mA;
}
[NSTimer scheduledTimerWithTimeInterval:1 
                                 target:self 
                               selector:@selector(flashPoint) 
                               userInfo:nil 
                                repeats:YES];

First of all, this NSTimer is problematic.

  1. It's in init but not in if (self). Why not?
  2. You don't store a reference to the repeating timer you've created. This is okay for non-repeating timers, but a huge no-no for repeating timers. You have no way of stopping this timer. AND, timers hold a strong reference to the object which the selector sent in the selector argument belongs to, so I'm quite certain this object never deallocates.
  3. You're starting this timer in init. This should probably be started elsewhere... think startAnimating and stopAnimating (like UIActivityIndicatorView with a hidesWhenStopped property).

NSMutableArray *mA = @[].mutableCopy;

This is personal preference, but this syntax is really silly and if I were maintaining your code, I'd immediately change this to [NSMutableArray array];


Everything a while loop can do can also be done by a do...while and a for loop. All three of these loops can do everything the other two can do. How we decide between the loops is based entirely on which loop's syntax makes our code most clear. Chances are, if you're using i, you probably need a for loop for the most clear syntax.

And your loop is a perfect candidate for a for loop. It even saves two lines of code! We should definitely be using a for loop here.

And because this is in init, I'd recommend extrapolating the bulk of the loop out into a function (which you could inline) which does all of the set up of the view and then returns the view for you to add to the subview and the array.

share|improve this answer

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.