ViewController.h
First, this name isn't really descriptive at all, is it? We need to know more about this view controller. In fact, standard naming convention is to suffix all view controller subclasses with ViewController
anyway, so this is basically an unnamed view controller.
Now, basically everything declared in your .h
should be in the .m
file. Of the 6 property declarations, 5 of them are IBOutlet
properties, and there's never really an excuse for a view controller to expose its subviews publicly. A view controller should be in complete controller of its view. If an outside class needs to do something that would effect one of these views, create a public method, and let that method internally handle the effects on those views.
As for the 4 IBAction
method declarations, you don't even need to declare these. The only reason to put a method in an @interface
section is to expose it to another class. And IBAction
methods should never be exposed to another class for the same point as exposing IBOutlets
to outside classes. If you need to, create a public method and have that method call the IBAction
method internally.
Finally, the MBFQuotes
object. Does this need public visibility? Are you modifying it from outside classes? Looks like probably not, and as such, we can get rid of the class declaration (@class MBFQuotes
) and move the @property
declaration in the the @interface
section of the .m
, which comes after the #import
of that classes file, and eliminates the need for declaring the class.
ViewController.m
Seeing as we're not actually doing anything in didReceiveMemoryWarning
, we can simply eliminate that entire method from this file. All we're doing is calling the super
method, and simply leaving the entire method out handles that. I understand that didReceiveMemoryWarning
's stub is generated automatically by Xcode when you create a new UIViewController
subclass, but you should still take it out unless you intend to do something with it (and in most cases, you won't need this method).
Our four IBAction
methods look quite similar, don't they? Why don't we combine them into one method.
First, we need an enum
for some readability, so at the top of this file, right after the #import
statements, you'll want this code:
typedef NS_ENUM(NSInteger, QuoteTypes) {
GeneralQuotes = 100,
LifeQuotes = 101,
PoliticalQuotes = 102,
SmartQuotes = 103
};
Now, let's use the .tag
property of UIButton
. You can set this on the storyboard or programmatically. If you set it on the storyboard, set the tag for each button to match the appropriate number in the enum
we just created. Programmatically, you can set them as such:
self.generalButton.tag = GeneralQuotes;
self.lifeButton.tag = LifeQuotes;
self.politicalButton.tag = PoliticalQuotes;
self.smartButton.tag = SmartQuotes;
Now then, get rid of the 4 IBAction
methods (be sure to unlink the buttons in storyboard), and create on method to handle all 4 buttons, link all of the buttons to that method. The method should look something like this:
- (IBAction)quoteButtonPushed:(id)sender {
switch(sender.tag) {
case GeneralQuotes:
self.quoteLabel.text = [self.quotes randomGeneral];
self.quoteLabel.backgroundColor = [UIColor redColor];
break;
case LifeQuotes:
self.quoteLabel.text = [self.quotes randomLife];
self.quoteLabel.backgroundColor = [UIColor blueColor];
break;
case PoliticalQuotes:
self.quoteLabel.text = [self.quotes randomPolitical];
self.quoteLabel.backgroundColor = [UIColor blackColor];
break;
case SmartQuotes:
self.quoteLabel.text = [self.quotes randomSmart];
self.quoteLabel.backgroundColor = [UIColor orangeColor];
break;
default: break;
}
}
And please notice that I've used self.quotes
here rather than _quotes
as you used. If you're declaring a @property
, you should be accessing that property through the accessor except in init
, dealloc
, and the accessor methods, should you choose to write custom accessor methods. If you don't want to access the property through the accessors, then maybe you don't need a property and instead just need a regular instance variable.
MBFQuotes.h / MBFQuotes.m
As it stands, this is really incomplete as a class, and doesn't really justify being a class at all. It could easily be simply a set of C-style functions. Which way do you want to go? Right now, you're in between.
If we're happy with the methods as they are and don't want to add anything, then let's change this to some C-style functions:
// MBFQuotes.h
#import <Foundation/Foundation.h>
NSString * randomGeneral();
NSString * randomLife();
NSString * randomPolitical();
NSString * randomSmart();
Notice the slightly different style and the elimination of @interface
and @end
. This is no longer a class, but instead a header file with some C-style function declarations.
EDIT: All right, so you can't actually have const
NSArray objects as far as I can tell. The answer has been edited to the next best solution:
// MBFQuotes.m
#import "MBFQuotes.h"
NSString * randomGeneral() {
static NSArray *generalQuotes = @[@"g1", @"g2", @"g3", @"g4"];
int randomGeneral = arc4random_uniform([generalQuotes count]);
return [generalQuotes objectAtIndex:randomGeneral];
}
NSString * randomLife() {
static NSArray *lifeQuotes = @[@"l1", @"l2"];
int randomLife = arc4random_uniform([lifeQuotes count]);
return [lifeQuotes objectAtIndex:randomLife];
}
NSString * randomPolitical() {
static NSArray *politicsQuotes = @[@"p1", @"p2"];
int randomPolitical = arc4random_uniform([politicsQuotes count]);
return [politicsQuotes objectAtIndex:randomPolitical];
}
NSString * randomSmart() {
static NSArray *smartQuotes = @[@"s1", @"s2"];
int randomSmart = arc4random_uniform([smartQuotes count]);
return [smartQuotes objectAtIndex:randomSmart];
}
Notice moving the array declaration out of the functions/methods and declaring them as constants. This makes the program a lot more efficient.
Anyway, if you don't want to go this route, there is certainly room to turn this into a class. For starters, you'd still want to move the arrays out of the methods, but now you'll want them as private properties. For this to be truly classworthy though, you'd want to include methods to be able to add and remove quotes, perhaps give the class a file-path and parse quotes out of a text file.
Realistically, since this is a class and we're instantiating it, we shouldn't have different categories of quotes. Realistically, we should instantiate a different object of this class for each quote category.
UNLESS... we were to add a method to pick a random quote from ALL categories (single method, random category, random quote).
Finally, I think for the project to be really complete, instead of the MBHQuotes
class simply holding an array of strings, completeness would say there should be a Quote class, which consists of a few properties. The main property is the quote itself. The other properties give some information about the quote, such as the author, the date of the quote's origin, the category of quote it fits into, etc.