HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Custom numberpad on an iPad

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
ipadcustomnumberpad

Problem

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

#import "Numberpad.h"

#pragma mark - Private methods

@interface Numberpad ()

@property (nonatomic, weak) id targetTextInput;

@end

#pragma mark - Numberpad Implementation

@implementation Numberpad

@synthesize targetTextInput;

#pragma mark - Singleton method

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

Solution

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

Code Snippets

@implementation Numberpad {

    id<UITextInput> targetTextInput;
}
UITextPosition  *endPosition    = selectedTextRange.end;
UITextRange     *rangeToDelete  = [self.targetTextInput textRangeFromPosition:startPosition
                                                                   toPosition:endPosition];
if (my condition) {

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

Context

StackExchange Code Review Q#18676, answer score: 7

Revisions (0)

No revisions yet.