A flashing ! exclamation ! point ! bar

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:



Header file:

#import 

@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() %

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.

StackExchange Code Review Q#58192, answer score: 12