App for allowing the user to type in chemistry formulas using a custom keyboard

Question

Essentially, I am making an app that allows the user to type in chemistry formulas using a custom keyboard.

The keyboard will have several keys, for example, "Na", "H", and "O". Pressing them in different combinations will result in different formulas. For example, Na + O will result in NaO. Pressing any key consecutively will increase the subscript of that given element. For example, H + H + O will result in H₂O.

The code below determines how the textfield combines the different elements to make the formulas. I'm wondering if it's too confusing. Also, I'm not sure if I'm correctly managing the memory.

consecutiveElementCount = 0;
formula = [[NSMutableString  stringWithString:@""] retain]; // released in dealloc

^^^ declared in the init method.

- (void)addElement:(NSString *)currentElement {


    if (![currentElement isEqualToString:lastElementPressed])  {

        // when new element is pressed 

        [formula appendString:currentElement];
        lastElementPressed = currentElement;
        consecutiveElementCount = 1;


    }


    else if ([currentElement isEqualToString:lastElementPressed]) { 

        // when element is pressed consecutively

        if (consecutiveElementCount == 1) {

            // since there is no '1' subscript, nothing needs to be deleted

            [formula appendString:@"₂"]; 
            subscriptLength = 1;


        }


        if (consecutiveElementCount > 1) {

            // deletes last subscript and replaces it with new subscript


            NSArray *subscriptList = [[[NSArray alloc] initWithObjects:@"₂", @"₃", @"₄", @"₅", @"₆", @"₇", @"₈", @"₉", @"₁₀", @"₁₁", @"₁₂", nil] autorelease]; 



            NSRange range = NSMakeRange (([formula length] - subscriptLength), subscriptLength);

            [formula deleteCharactersInRange:range];

            NSString *tempSubscript = [[subscriptList objectAtIndex:consecutiveElementCount - 1] autorelease];
            [formula appendString:tempSubscript];

            subscriptLength = [tempSubscript length];

        }


     // don't increase subscript after 12
        if (consecutiveElementCount < 11)
            consecutiveElementCount ++;

    }


    NSLog(@"%@", formula);
}

Answer 1

The following variation:

• initializes and uses subscriptLength more consistently.
• minimizes redundancy and special cases
• eliminates distracting whitespace.

for

- (void)addElement:(NSString *)currentElement {
    if (![currentElement isEqualToString:lastElementPressed])  {
        // when new element is pressed 
        [formula appendString:currentElement];
        lastElementPressed = currentElement;
        consecutiveElementCount = 1;
        subscriptLength = 0;
    }
    // TO DO: might want to replace 12 with a named constant like MAX_SUBSCRIPT
    // for greater readability and flexibility
    else if (consecutiveElementCount < 12)
        // when element is pressed consecutively, up to 12 times

        // since there is no '1' subscript, nothing needs to be deleted
        // for count == 1
        if (subscriptLength > 0) {
            // delete current subscript
            NSRange range = NSMakeRange (([formula length] - subscriptLength), subscriptLength);
            [formula deleteCharactersInRange:range];
        }
        // TO DO: might want to allocate subscriptList only once EVER
        // and keep it around for re-use, rather than reallocate it every time?
        NSArray *subscriptList = [[[NSArray alloc] initWithObjects:@"₂", @"₃", @"₄", @"₅", @"₆", @"₇", @"₈", @"₉", @"₁₀", @"₁₁", @"₁₂", nil] autorelease]; 
        // append subscript
        NSString *tempSubscript = [[subscriptList objectAtIndex:consecutiveElementCount-1] autorelease];
        [formula appendString:tempSubscript];
        subscriptLength = [tempSubscript length];
        consecutiveElementCount++;
    }
    // else { /* just ignore identical key-presses after 12 of them */ }
    NSLog(@"%@", formula);
}

Answer 2

You can use some if else statement, where u use several independent ifs. This will lead to a slightly faster code, because in the case, where the previous if is try, no further testing is needed. Personally I also find it more readable for short and medium sized code block.

if (![currentElement isEqualToString:lastElementPressed])  {
    //…
} else { 
    if (consecutiveElementCount == 1) { 
         //…
    } else if (consecutiveElementCount > 1) {
        //…
    } 
    if (consecutiveElementCount < 11) {
        //…
    }
}

But beside that, I don't think, that you are using to many if-statements. But you should use proper indention, as it will build visual block of your code and helps you, to understand the structure.

There is a rule of thumb: If a method/function does not fit on your screen, it becomes to long, and you should use well-named helper functions to break it into several logical blocks.

Answer 3

A couple of observations:

1. It looks to me that the if part of your code else if ([currentElement isEqualToString:lastElementPressed]) is unnecessary; you already checked the negation of this condition in the top-level if statement.

2. Although not necessary, you might want to put else in front of your if statement if (consecutiveElementCount > 1). In fact, is there ever a chance that the consecutiveElementCount variable will be less than 1? If not, then just turn it into an else without a condition.

I kind of prefer to organize this code as shown in the code example below. Hopefully I haven't mangled it; I've combined some of your if statements into a switch statement.

After reading your code, I decided that there are basically 3 steps that it performs:

1. Detect whether the element pressed is a new element or is the same as the last element pressed.

2. Perform work based on the number of times that the element has been pressed.

3. Increment the counter to record the number of times the current element has been pressed

Your original code kind of mixes #1 and #2 together. In the code below, I've separated them a bit and flattened things out with a switch statement. I think it's easier to read and understand what's happening. Similar-purpose code is grouped closer together.

- (void)addElement:(NSString *)currentElement {

    // determine if the element pressed is different than the last element pressed
    if (![currentElement isEqualToString:lastElementPressed])  {
        lastElementPressed = currentElement;
        // set count to zero to indicate that this element has not been pressed previously (consecutively)
        consecutiveElementCount = 0;
    }

    // perform logic based on the number of times the element has been pressed
    switch(consecutiveElementCount)
    {
        case 0: // first time the element is pressed
            // when new element is pressed 

            [formula appendString:currentElement];
            break;
        case 1: // second time the element is pressed
            // since there is no '1' subscript, nothing needs to be deleted

            [formula appendString:@"2"]; 
            subscriptLength = 1;
            break;
        default: // third or more times that the element is pressed
            // deletes last subscript and replaces it with new subscript


            NSArray *subscriptList = [[[NSArray alloc] initWithObjects:@"2", @"3", @"4", @"5", @"6", @"7", @"8", @"9", @"10", @"11", @"12", nil] autorelease]; 



            NSRange range = NSMakeRange (([formula length] - subscriptLength), subscriptLength);

            [formula deleteCharactersInRange:range];

            NSString *tempSubscript = [[subscriptList objectAtIndex:consecutiveElementCount - 1] autorelease];
            [formula appendString:tempSubscript];

            subscriptLength = [tempSubscript length];
            break;
    }

    // increment the counter to record the number of times this element has been pressed,
    // but don't increase subscript after 12
    if (consecutiveElementCount < 11)
        consecutiveElementCount ++;

}