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'm an active user on SO, however I just found this site and this is my first question here. 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?
  • Any other thoughts?

Thank you for your time!

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
share|improve this question
add comment

2 Answers 2

up vote 6 down vote accepted

Hi and welcome on CodeReview ! As you can see, I also recently discovered this StackExchange site. I'm not that much experienced in iOS development, but I give a great deal about code formatting. These are my suggestions, it's mostly my own opinion and in no way the only way of doing things.

First of all, 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 (lol), I think it'll fit.

  • Any other thoughts?

Yes ! lol

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, so..

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).

Conclusion

So, this is just my own opinion, anyone feel free to disagree on any point I gave. It's what I like to read, and how I like my code to look like.

I hope this helped !

share|improve this answer
    
Thank you for your detailed analysis! I have a few comments about what you have said (I'll add one comment for each subject to make them easier to follow. :)) –  lnafziger Nov 17 '12 at 4:28
    
The difference between putting targetTextInput in the implementation block and putting it in the .m file is two-fold. 1. The way that you suggest creates an iVar which is different than adding a declared property. It creates no setters/getters and forces you to use the iVar directly. 2. Putting it in the .m file hides the implementation details of the class from external users of the class. When there is something that does not need to be exposed, it gives more flexibility down the road when it comes time to make changes. –  lnafziger Nov 17 '12 at 4:32
    
Aligning the = signs: While I do think that it makes code easier to read (you can easily scan down a list of variables without having to "zig-zag"), you bring up good points about maintainability. I'll try that moving forward, thanks! –  lnafziger Nov 17 '12 at 4:34
    
if { on one line -vs- two lines: Yeah, I have changed my style on this since originally writing the class and had mixed styles, which I thought that I had made all match before posting the question. Those two slipped through, thanks. –  lnafziger Nov 17 '12 at 4:36
    
Inline -vs- multiple line method calls: This one I really think improves the readability on long lines because it is very clear where each argument starts, and with the very long method names that we use on iOS, it often does wrap to multiple lines for me. Is there standardization on the inline format from most programmers? (If so, I guess I'll just have to deal with it, lol.) –  lnafziger Nov 17 '12 at 4:39
show 12 more comments

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.

share|improve this answer
    
Quoting this book ? amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/… –  rdurand Nov 16 '12 at 16:48
    
@rdurand: Oh, no, sorry, see the edit, please. Thanks for the comment! –  palacsint Nov 16 '12 at 17:02
    
Seems like quite a read ! Looks instructive. –  rdurand Nov 16 '12 at 17:07
    
+1, thanks for the reference! Any other suggestions? –  lnafziger Nov 17 '12 at 4:49
    
@lnafziger: That was my only one. I'm not familiar with iOS/Objective C at all. –  palacsint Nov 17 '12 at 16:53
show 1 more comment

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.