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?
• Code formatting: Any suggestions?
• Any other thoughts?

Full project is hosted on Github.

/******************************************************************************
* v. 0.9.1  15 NOV 2012
* Purpose:  Class to display a custom numberpad on an iPad and properly handle
*           the text input.
* Author:   Louis Nafziger
*
******************************************************************************
*
* This file is part of NumberPad.
*
* NumberPad is free software: you can redistribute it and/or modify
* 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
*
*****************************************************************************/

#pragma mark - Private methods

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

@end

@synthesize targetTextInput;

#pragma mark - Singleton method

static dispatch_once_t onceToken;

dispatch_once(&onceToken, ^{
});
}

#pragma mark - view lifecycle

// Keep track of the textView/Field that we are editing
selector:@selector(editingDidBegin:)
object:nil];
selector:@selector(editingDidBegin:)
object:nil];
selector:@selector(editingDidEnd:)
object:nil];
selector:@selector(editingDidEnd:)
object:nil];
}

object:nil];
object:nil];
object:nil];
object:nil];
self.targetTextInput = nil;

}

#pragma mark - editingDidBegin/End

// Editing just began, store a reference to the object that just became the firstResponder
self.targetTextInput = nil;
return;
}

}

// Editing just ended.
self.targetTextInput = nil;
}

// A number (0-9) was just pressed on the number pad
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
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
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
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

-

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.

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

-
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

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.

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

-
Yes, this is an old version that was written before viewDidUnload was deprecated. The current version available here has been updated since. Good suggestion on the first part though, thanks! –  lnafziger Jul 29 '14 at 1:00