Message estimator

I'm new to C# and OOP and am struggling to move away from the procedural way of doing things. I understand the tutorials I have looked at but find it more difficult when actually doing something a little more involved.

The code works out the number of presses and time taken for the supplied message.

```
public partial class Form1 : Form
{

public Form1()
{
InitializeComponent();
}

private void Form1_Load(object sender, EventArgs e)
{

}

private void calculateButton_Click(object sender, EventArgs e)
{
try
{
// basic validation
if (string.IsNullOrWhiteSpace(messageTextBox.Text))
{
MessageBox.Show("A message must be entered.", "Error Detail", MessageBoxButtons.OK, MessageBoxIcon.Error);
messageTextBox.Focus();
return;
}

Message message = new Message();
// exclude all punctuation and non alphanumeric characters
message.KeypadMessage = Regex.Replace(messageTextBox.Text, "[^0-9A-Za-z]", String.Empty);

String keypad = "0,1,2:abc,3:def,4:ghi,5:jkl,6:mno,7:pqrs,8:tuv,9:wxyz";

CharacterCollection coll = new CharacterCollection();
List chars = coll.createCollection(keypad);

KeyCharacter precedingChar = null;

for (int i = 0; i createCollection(String delmitedKeypad)
{
List list = new List();

String[] KeyCharacters = delmitedKeypad.Split(',');

for (int i = 0; i 1)
{
// number and letter key
for (int j = 0; j characterCollection, char character)
{
KeyCharacter keyChar;
if (char.IsDigit(character))
{
// number
int number;
if (!Int32.TryParse(character.ToString(), out number))
{
throw new InvalidOperationException();
}

keyChar = (KeyChara

Give your form a meaningful class name instead of Form1

Remove the Form1_Load method, and be sure to remove the event handler in the GUI designer from your main form.

use foreach iterator instead of for, I also took the liberty to use ternary operators instead of giant if-else bocks.

bool isFirst = true;

foreach(var item in message.KeypadMessage)
{
    KeyCharacter keyChar = coll.getKeyCharacter(chars, item);

    message.addToMetrics(message, item, keyChar,
        isFirst ? "Initial" : 
        keyChar.Number == precedingChar.Number ? "SameKeyAsPreceding" :
        String.Empty);

    precedingChar = keyChar;

    if(isFirst) 
        isFirst = false;
}

I would suggest not blanket catching all Exceptions. Catch exceptions you know are going to be thrown explicitly, you should also take preventative measures to make sure exceptions are not thrown. Also instead of completely closing down the application when this calculation fails, perhaps just show an error message on screen and continue.

For future reference, instead of making the KeyCharacter class you could take advantage of the Tuple class. Not to say that you shouldn't have made this class, but Tuple can generally replace these simple data structure like classes.

In CharacterCollection the public method name createCollection should be PascalCase

and String[] KeyCharacterNumbersAndLetters is a method level variable and thus should be camelCase

KeyCharacter keyCharacter = new KeyCharacter();
keyCharacter.NumberOnly = false;
keyCharacter.Number = Convert.ToInt32(KeyCharacterNumbersAndLetters[0]);
keyCharacter.Letter = KeyCharacterNumbersAndLetters[1][j];
keyCharacter.NumberOfPresses = j + 1;
list.Add(keyCharacter);

can be written as one line

list.Add(new KeyCharacter(){
    NumberOnly = false,
    Number = Convert.ToInt32(KeyCharacterNumbersAndLetters[0]),
    Letter = KeyCharacterNumbersAndLetters[1][j],
    NumberOfPresses = j + 1
});

As I am not a MVP expert, if you want some information on MVP check out Microsoft's page on it.

Edit: Expanding on Exceptions

Instead of throwing an InvalidOperationException, adopt the model used in TryParse. Return success or failure of the method as a boolean, and sent out the data you want. This way you don't have to throw an exception at all.

public Boolean TryGetKeyCharacter(List characterCollection, char character, out KeyCharacter keyChar)
{
    if (char.IsDigit(character))
    {
        int number;
        if (!Int32.TryParse(character.ToString(), out number))
        {
            keyChar = null;
            return false;
        }
        keyChar = (KeyCharacter)characterCollection.Where(c => c.Number == number).FirstOrDefault();
    }
    else
    {
        char chr = char.ToLower(character);
        keyChar = (KeyCharacter)characterCollection.Where(c => c.Letter == chr).FirstOrDefault();
    }
    return true;
}

As for catching all exceptions, if you coded your program right, you shouldn't be getting ANY exceptions, not even out of bounds ones, you should design your logic to never check indices that are out of bounds.

StackExchange Code Review Q#51898, answer score: 4