4
\$\begingroup\$

I just began learning programming and I'm learning Objective-C. I'm taking an online course and in that course we were doing a magic crystal ball app. Then I decided to use what I learned and make my own app.

Viewcontroller .h

#import <UIKit/UIKit.h>

@class MBFQuotes;

@interface MBFViewController : UIViewController

@property (strong, nonatomic) MBFQuotes *quotes;

@property (strong, nonatomic) IBOutlet UILabel *quoteLabel;


@property (strong, nonatomic) IBOutlet UIButton *generalButton;
@property (strong, nonatomic) IBOutlet UIButton *lifeButton;
@property (strong, nonatomic) IBOutlet UIButton *politicsButton;
@property (strong, nonatomic) IBOutlet UIButton *smartButton;

//Action

- (IBAction)generalButton:(id)sender;
- (IBAction)lifeButton:(id)sender;
- (IBAction)politicsButton:(id)sender;
- (IBAction)smartButton:(id)sender;

@end

Viewcontroller .m

#import "MBFViewController.h"
#import "MBFQuotes.h"

@interface MBFViewController ()

@end

@implementation MBFViewController

- (void)viewDidLoad
{
[super viewDidLoad];
self.quotes = [[MBFQuotes alloc] init];
}

- (void)didReceiveMemoryWarning
{
[super didReceiveMemoryWarning];
// Dispose of any resources that can be recreated.
}

- (IBAction)generalButton:(id)sender {
self.quoteLabel.text = [_quotes randomGeneral];
self.quoteLabel.backgroundColor = [UIColor redColor];

}   

- (IBAction)lifeButton:(id)sender {
self.quoteLabel.text = [_quotes randomLife];
self.quoteLabel.backgroundColor = [UIColor blueColor];

}

- (IBAction)politicsButton:(id)sender {
self.quoteLabel.text = [_quotes randomPolitics];
self.quoteLabel.backgroundColor = [UIColor blackColor];

}

- (IBAction)smartButton:(id)sender {
self.quoteLabel.text = [_quotes randomSmart];
self.quoteLabel.backgroundColor = [UIColor orangeColor];

}
@end   

Class Quotes .h

#import <Foundation/Foundation.h>

@interface MBFQuotes : NSObject

-(NSString *)randomGeneral;
-(NSString *) randomLife;
-(NSString *) randomPolitics;
-(NSString *) randomSmart;
@end

Class Quotes.m

#import "MBFQuotes.h"

@implementation MBFQuotes

-(NSString *) randomGeneral {

 NSArray *generalQuotes = @[@"g1", @"g2", @"g3", @"g4"];

int randomGeneral = arc4random_uniform([generalQuotes count]);

return [generalQuotes objectAtIndex:randomGeneral];
}

-(NSString *) randomLife {

NSArray *lifeQuotes = @[@"l1", @"l2"];

int randomLife = arc4random_uniform([lifeQuotes count]);

 return [lifeQuotes objectAtIndex:randomLife];
}

-(NSString *) randomPolitics {

NSArray *politicsQuotes = @[@"p1", @"p2"];

int randomPolitics = arc4random_uniform([politicsQuotes count]);

return [politicsQuotes objectAtIndex:randomPolitics];
}

-(NSString *) randomSmart {

 NSArray *smartQuotes = @[@"s1", @"s2"];

int randomSmart = arc4random_uniform([smartQuotes count]);

return [smartQuotes objectAtIndex:randomSmart];
 }

 @end

The app works perfectly, but I just want to see if there is better or easier way to do it, or is there there a better design.

\$\endgroup\$
1
  • \$\begingroup\$ Regarding your P.S. statement I removed, select all of the code and press CTRL-K. \$\endgroup\$
    – syb0rg
    Commented Apr 7, 2014 at 14:12

1 Answer 1

3
\$\begingroup\$

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.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.