Custom numberpad on an iPad

Question

I am a self-taught programmer who has been programming for a long time, but mainly just for personal projects. I would like to improve my skills and am open to any suggestions that you may have, but am particularly interested in your thoughts about:

• The number of methods in this class: Too few/too many?
• Method/variable names: Descriptive enough?/too descriptive?
• Comments: Too few/too many?
• Code formatting: Any suggestions?

Full project is hosted on GitHub.

Numberpad.m

/******************************************************************************
 * v. 0.9.1  15 NOV 2012
 * Filename  Numberpad.m
 * Project:  NumberPad
 * Purpose:  Class to display a custom numberpad on an iPad and properly handle
 *           the text input.
 * Author:   Louis Nafziger
 *
 * Copyright 2012 Louis Nafziger
 ******************************************************************************
 *
 * This file is part of NumberPad.
 *
 * NumberPad is free software: you can redistribute it and/or modify
 * it under the terms of the GNU Lesser General Public License as published by
 * the Free Software Foundation, either version 3 of the License, or
 * (at your option) any later version.
 *
 * NumberPad is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU Lesser General Public License for more details.
 *
 * You should have received a copy of the GNU LesserGeneral Public License
 * along with NumberPad.  If not, see <http://www.gnu.org/licenses/>. *
 *
 *****************************************************************************/

#import "Numberpad.h"

#pragma mark - Private methods

@interface Numberpad ()

@property (nonatomic, weak) id<UITextInput> targetTextInput;

@end

#pragma mark - Numberpad Implementation

@implementation Numberpad

@synthesize targetTextInput;

#pragma mark - Singleton method

+ (Numberpad *)defaultNumberpad {
    static Numberpad *defaultNumberpad = nil;
    static dispatch_once_t onceToken;

    dispatch_once(&onceToken, ^{
        defaultNumberpad = [[Numberpad alloc] init];
    });
    return defaultNumberpad;
}

#pragma mark - view lifecycle

- (void)viewDidLoad {
    [super viewDidLoad];

    // Keep track of the textView/Field that we are editing
    [[NSNotificationCenter defaultCenter] addObserver:self
                                             selector:@selector(editingDidBegin:)
                                                 name:UITextFieldTextDidBeginEditingNotification
                                               object:nil];
    [[NSNotificationCenter defaultCenter] addObserver:self
                                             selector:@selector(editingDidBegin:)
                                                 name:UITextViewTextDidBeginEditingNotification
                                               object:nil];
    [[NSNotificationCenter defaultCenter] addObserver:self
                                             selector:@selector(editingDidEnd:)
                                                 name:UITextFieldTextDidEndEditingNotification
                                               object:nil];
    [[NSNotificationCenter defaultCenter] addObserver:self
                                             selector:@selector(editingDidEnd:)
                                                 name:UITextViewTextDidEndEditingNotification
                                               object:nil];
}

- (void)viewDidUnload {
    [[NSNotificationCenter defaultCenter] removeObserver:self
                                                    name:UITextFieldTextDidBeginEditingNotification
                                                  object:nil];
    [[NSNotificationCenter defaultCenter] removeObserver:self
                                                    name:UITextViewTextDidBeginEditingNotification
                                                  object:nil];
    [[NSNotificationCenter defaultCenter] removeObserver:self
                                                    name:UITextFieldTextDidEndEditingNotification
                                                  object:nil];
    [[NSNotificationCenter defaultCenter] removeObserver:self
                                                    name:UITextViewTextDidEndEditingNotification
                                                  object:nil];
    self.targetTextInput = nil;

    [super viewDidUnload];
}

#pragma mark - editingDidBegin/End

// Editing just began, store a reference to the object that just became the firstResponder
- (void)editingDidBegin:(NSNotification *)notification {
    if (![notification.object conformsToProtocol:@protocol(UITextInput)]) {
        self.targetTextInput = nil;
        return;
    }

    self.targetTextInput = notification.object;
}

// Editing just ended.
- (void)editingDidEnd:(NSNotification *)notification {
    self.targetTextInput = nil;
}

#pragma mark - Keypad IBAction's

// A number (0-9) was just pressed on the number pad
- (IBAction)numberpadNumberPressed:(UIButton *)sender {
    if (self.targetTextInput == nil) {
        return;
    }

    NSString *numberPressed  = sender.titleLabel.text;
    if ([numberPressed length] == 0) {
        return;
    }

    UITextRange *selectedTextRange = self.targetTextInput.selectedTextRange;
    if (selectedTextRange == nil) {
        return;
    }

    [self textInput:self.targetTextInput replaceTextAtTextRange:selectedTextRange withString:numberPressed];
}

// The delete button was just pressed on the number pad
- (IBAction)numberpadDeletePressed:(UIButton *)sender {
    if (self.targetTextInput == nil) {
        return;
    }

    UITextRange *selectedTextRange = self.targetTextInput.selectedTextRange;
    if (selectedTextRange == nil) {
        return;
    }

    // Calculate the selected text to delete
    UITextPosition  *startPosition  = [self.targetTextInput positionFromPosition:selectedTextRange.start offset:-1];
    if (startPosition == nil) {
        return;
    }
    UITextPosition  *endPosition    = selectedTextRange.end;
    if (endPosition == nil) {
        return;
    }
    UITextRange     *rangeToDelete  = [self.targetTextInput textRangeFromPosition:startPosition
                                                                       toPosition:endPosition];

    [self textInput:self.targetTextInput replaceTextAtTextRange:rangeToDelete withString:@""];
}

// The clear button was just pressed on the number pad
- (IBAction)numberpadClearPressed:(UIButton *)sender {
    if (self.targetTextInput == nil) {
        return;
    }

    UITextRange *allTextRange = [self.targetTextInput textRangeFromPosition:self.targetTextInput.beginningOfDocument
                                                                 toPosition:self.targetTextInput.endOfDocument];

    [self textInput:self.targetTextInput replaceTextAtTextRange:allTextRange withString:@""];
}

// The done button was just pressed on the number pad
- (IBAction)numberpadDonePressed:(UIButton *)sender {
    if (self.targetTextInput == nil) {
        return;
    }

    // Call the delegate methods and resign the first responder if appropriate
    if ([self.targetTextInput isKindOfClass:[UITextView class]]) {
        UITextView *textView = (UITextView *)self.targetTextInput;
        if ([textView.delegate respondsToSelector:@selector(textViewShouldEndEditing:)]) {
            if ([textView.delegate textViewShouldEndEditing:textView])
            {
                [textView resignFirstResponder];
            }
        }
    } else if ([self.targetTextInput isKindOfClass:[UITextField class]]) {
        UITextField *textField = (UITextField *)self.targetTextInput;
        if ([textField.delegate respondsToSelector:@selector(textFieldShouldEndEditing:)]) {
            if ([textField.delegate textFieldShouldEndEditing:textField])
            {
                [textField resignFirstResponder];
            }
        }
    }
}

#pragma mark - text replacement routines

// Check delegate methods to see if we should change the characters in range
- (BOOL)textInput:(id <UITextInput>)textInput shouldChangeCharactersInRange:(NSRange)range withString:(NSString *)string
{
    if ([textInput isKindOfClass:[UITextField class]]) {
        UITextField *textField = (UITextField *)textInput;
        if ([textField.delegate respondsToSelector:@selector(textField:shouldChangeCharactersInRange:replacementString:)]) {
            if (![textField.delegate textField:textField
                 shouldChangeCharactersInRange:range
                             replacementString:string]) {
                return NO;
            }
        }
    } else if ([textInput isKindOfClass:[UITextView class]]) {
        UITextView *textView = (UITextView *)textInput;
        if ([textView.delegate respondsToSelector:@selector(textView:shouldChangeTextInRange:replacementText:)]) {
            if (![textView.delegate textView:textView
                     shouldChangeTextInRange:range
                             replacementText:string]) {
                return NO;
            }
        }
    }
    return YES;
}

// Replace the text of the textInput in textRange with string if the delegate approves
- (void)textInput:(id <UITextInput>)textInput replaceTextAtTextRange:(UITextRange *)textRange withString:(NSString *)string {
    if (textInput == nil) {
        return;
    }
    if (textRange == nil) {
        return;
    }

    // Calculate the NSRange for the textInput text in the UITextRange textRange:
    int startPos                    = [textInput offsetFromPosition:textInput.beginningOfDocument
                                                         toPosition:textRange.start];
    int length                      = [textInput offsetFromPosition:textRange.start
                                                         toPosition:textRange.end];
    NSRange selectedRange           = NSMakeRange(startPos, length);

    if ([self textInput:textInput shouldChangeCharactersInRange:selectedRange withString:string]) {
        // Make the replacement:
        [textInput replaceRange:textRange withText:string];
    }
}

@end

Answer 1

I'm not that much experienced in iOS development, but I can give a great deal about code formatting. These are my suggestions, which are mostly my own opinion and in no way the only way of doing things.

I'll start by saying that in your situation (working on your own), formatting your code properly is mostly so you can read your code easily, and find your way through if you leave the project for a certain amount of time, and come back to it later. So, you should really do it the way you like, and in a way you're sure you'll always understand.

That being said, here is what I think, answering to the points you gave in your question.

The number of methods in this class:

It seems about right. There doesn't seems to be code duplicates, and I mostly see methods as a way of avoiding this situation.

Method/variable names:

They're explicit, and as I said earlier, as long as you understand them, it's fine.

One thing though, you declare the property targetTextInput in your .m file. Someone stop me if I'm wrong, but I think you only get for this the fact that the property becomes private. To do so, I would personally declare it in the @implementation directive :

@implementation Numberpad {

    id<UITextInput> targetTextInput;
}

The variable is still private, and I think it's more readable. Again, if someone else could confirm that one, I really think the outcome is the same.

Comments:

The code is well commented, most importantly all your methods have a comment explaining what they do. I wouldn't care for a couple more comments inside the methods, but they don't seem that complex here. Methods involving intense use of while, if, switch, for, etc. would require more comments to understand them.

Code formatting:

It's clear and well-structured, nothing strikes at sees.

One thing I really don't like though is the way you align all the "=" signs. I don't think it helps readability, so I find pretty useless doing so. You also do it with some declarations:

UITextPosition  *endPosition    = selectedTextRange.end;
UITextRange     *rangeToDelete  = [self.targetTextInput textRangeFromPosition:startPosition
                                                                   toPosition:endPosition];

I don't find it very useful...

I'd also say there's a bit of a lack of consistency in your formatting. For example (a minor thing though), you may use

if (my condition) {

    // my actions
}

and

if (my condition)
{
    // my actions
}

The position of the left brace is not really important, but if you really want a perfectly-formatted code, I'd consider picking one and use only this one throughout your code. Apple's code uses the first one, and I like it too. But again, personal choice.

Also, when you call methods in your ifs, you sometimes use an inline format, and sometimes you do it that way:

if (![textField.delegate textField:textField
     shouldChangeCharactersInRange:range
                 replacementString:string]) {
            return NO;
        }

I think an inline presentation is better, and I would drop the braces if your if's action is a single line, giving this result :

if (![textField.delegate textField:textField shouldChangeCharactersInRange:range replacementString:string]) return NO;

One may argue on that one, saying it's a too long line and it my break, but if you're not developing on an iPad, I think it'll fit.

Any other thoughts?

When you declare a property, it's nice to synthesize it that way :

@synthesize targetTextInput = _targetTextInput;

and access the property with _targetTextInput instead of self.targetTextInput.This "underscore convention" is encouraged by Apple, and even more, since Xcode 4.5, you don't need to synthesize your properties anymore (Xcode does it for you), and you can access them directly using _targetTextInput. Apple has integrated the underscore convention to properties declaration. It takes a few lines out from your code, so that's always nice.

When you check if an object is nil, there are two ways to do it :

if (self.targetTextInput == nil) {}
if (!self.targetTextInput) {}

Not much to say here, choose whichever you like, but you seem to have a bunch of those in your code.

Not everybody agrees on the use of pragma, as (I think ?) "/* */" can do the same (puts a link in the jump bar), but you can go further with it:

pragma -

will create a simple separator,

pragma My Label

will create a label, and

pragma - My Label

will create both (as you used it).

Answer 2

I agree with @rdurand about the code formatting. Aligning the right sides of assignment statements is too hard to maintain:

UITextPosition  *startPosition  = [self.targetTextInput positionFromPosition:selectedTextRange.start offset:-1];
...
UITextPosition  *endPosition    = selectedTextRange.end;
...
UITextRange     *rangeToDelete  = [self.targetTextInput textRangeFromPosition:startPosition
                                                                   toPosition:endPosition];

From Code Complete, 2nd Edition by Steve McConnell, p758:

Do not align right sides of assignment statements

[...]

With the benefit of 10 years’ hindsight, I have found that, while this indentation style might look attractive, it becomes a headache to maintain the alignment of the equals signs as variable names change and code is run through tools that substitute tabs for spaces and spaces for tabs. It is also hard to maintain as lines are moved among different parts of the program that have different levels of indentation.

Answer 3

if (![textField.delegate textField:textField
     shouldChangeCharactersInRange:range
                 replacementString:string]) {
    return NO;
}

This could be replaced by simply this:

return [textField.delegate textField:textField
       shouldChangeCharactersInRange:range
                   replacementString:string];

And the same can be done for textView.

---

- (void)viewDidUnload

viewDidUnload has been deprecated. We need to remove ourselves as observers in dealloc.