Message estimator

Question

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<KeyCharacter> chars = coll.createCollection(keypad);

            KeyCharacter precedingChar = null;

            for (int i = 0; i < message.KeypadMessage.Length; i++)
            {
                KeyCharacter keyChar;

                // get character 
                keyChar = coll.getKeyCharacter(chars, message.KeypadMessage[i]);

                if (i == 0)
                {
                    // intial character
                    message.addToMetrics(message, message.KeypadMessage[i], keyChar, "Initial");
                }
                else
                {
                    // determine whether character is on the same key as the preceding one
                    if (keyChar.Number == precedingChar.Number)
                    { 
                        // character on same key
                        message.addToMetrics(message, message.KeypadMessage[i], keyChar, "SameKeyAsPreceding");
                    }
                    else
                    {
                        // character not on same key
                        message.addToMetrics(message, message.KeypadMessage[i], keyChar, String.Empty);                        
                    }
                }

                // hold current character for comparison
                precedingChar = keyChar;
            }

            // display output
            noOfKeyPressesLabel.Text = message.NoOfKeyPresses.ToString();
            approxTimeLabel.Text = message.ApproxTime.ToString();
        }
        catch (Exception ex)
        {                   
            MessageBox.Show("Unhandled exception.");

            // TODO : log error using logger

            this.Close();
        }
    }
}


class KeyCharacter
{
    public bool NumberOnly { get; set; }
    public int Number { get; set; } // for when preceding letter of message is on same key
    public char Letter { get; set; }
    public int NumberOfPresses { get; set; }
}


class CharacterCollection
{

    public List<KeyCharacter> createCollection(String delmitedKeypad)
    {
        List<KeyCharacter> list = new List<KeyCharacter>();

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

        for (int i = 0; i < KeyCharacters.Length; i++)
        {
            String[] KeyCharacterNumbersAndLetters = KeyCharacters[i].Split(':');

            if (KeyCharacterNumbersAndLetters.Length > 1)
            {
                // number and letter key
                for (int j = 0; j < KeyCharacterNumbersAndLetters[1].Length; j++)
                {
                    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);
                }
            }
            else
            {
                // number only key
                KeyCharacter keyCharacter = new KeyCharacter();
                keyCharacter.NumberOnly = true;
                keyCharacter.Number = Convert.ToInt32(KeyCharacterNumbersAndLetters[0]);
                keyCharacter.NumberOfPresses = 1;
                list.Add(keyCharacter);
            }                
        }

        return list;
    }


    public KeyCharacter getKeyCharacter(List<KeyCharacter> characterCollection, char character)
    {
        KeyCharacter keyChar;
        if (char.IsDigit(character))
        {
            // number
            int number;
            if (!Int32.TryParse(character.ToString(), out number))
            {
                throw new InvalidOperationException();
            }

            keyChar = (KeyCharacter)characterCollection.Where(c => c.Number == number).FirstOrDefault();
        }
        else
        {
            // letter
            char chr = char.ToLower(character);
            keyChar = (KeyCharacter)characterCollection.Where(c => c.Letter == chr).FirstOrDefault();
        }
        return keyChar;
    }

}


class Message
{
    public const double letterKeyPressDelay = 0.3;
    public const double numberKeyHoldTime = 1;
    public const double sameKeyAsPrecedingDelay = 0.7;

    public string KeypadMessage { get; set; }
    public int NoOfKeyPresses { get; set; }
    public double ApproxTime { get; set; }


    public Message addToMetrics(Message message, char messageChar, KeyCharacter keyChar, string type)
    {
        message.NoOfKeyPresses += keyChar.NumberOfPresses;

        if (char.IsDigit(messageChar))
        {
            // number   
            // there is a delay for a number because it requires holding, even when it is the initial press 
            message.ApproxTime += keyChar.NumberOfPresses * numberKeyHoldTime; // number of presses for a number will be 1
            if (type == "SameKeyAsPreceding")
            {
                // extra delay due to character being on same key
                message.ApproxTime += sameKeyAsPrecedingDelay; 
            }
        }
        else
        {
            // letter 
            if (type == "Initial" || type == "SameKeyAsPreceding")
            {
                // no delay for intial press if the character is a letter
                // subtract 1 as the very first key press initiates the message OR subtract 1 as the first key press is the one that has the extra delay due to being on the same key 
                message.ApproxTime += (keyChar.NumberOfPresses - 1) * letterKeyPressDelay; 
            }
            else
            {
                // the first key press after the last character has the standard delay 
                message.ApproxTime += (keyChar.NumberOfPresses) * letterKeyPressDelay; 
            }

            if (type == "SameKeyAsPreceding")
            {
                // extra delay due to character being on same key
                message.ApproxTime += sameKeyAsPrecedingDelay; 
            }

            // uppercase letter requires 1 extra keypress
            if (Char.IsUpper(messageChar))
            {
                message.NoOfKeyPresses += 1;
                // add standard key press delay 
                message.ApproxTime += letterKeyPressDelay;
            }
        }

        return message;
    }        
}

I would be very grateful for any pointers about what I'm doing wrong (probably lots) with regards to code correctness and SOLID principles.

I also know that, for testability, the code shouldn't be in the click event, but I'm not sure how to make the code conform to something like the MVP passive view pattern.

## EDIT ##

I've attempted to use MVP and have updated the code as below. Firstly have I done it right and where can I apply IoC?

public partial class KeyPressCalc : Form, IMessage
{

    private MessagePresenter presenter;

    public string KeypadMessage
    {
        get { return messageTextBox.Text; }
        set { messageTextBox.Text = value; }
    }

    public int NoOfKeyPresses
    {
        // #### IS IT OK TO DO THIS ?????? ####
        get { return Convert.ToInt32(noOfKeyPressesLabel.Text); }
        set { noOfKeyPressesLabel.Text = value.ToString(); }
    }

    public double ApproxTime
    {
        get { return Convert.ToDouble(approxTimeLabel.Text); }
        set { approxTimeLabel.Text = value.ToString(); }
    }

    // event logic defined in presenter
    public event EventHandler<EventArgs> Calculate;


    public KeyPressCalc()
    {
        InitializeComponent();

        // pass this form to presenter
        presenter = new MessagePresenter(this);      
    }    


    private void calculateButton_Click(object sender, EventArgs e)
    { 
            // basic validation
            if (string.IsNullOrWhiteSpace(this.KeypadMessage))
            {
                MessageBox.Show("A message must be entered.", "Error Detail", MessageBoxButtons.OK, MessageBoxIcon.Error);
                messageTextBox.Focus();
                return;
            }
            else if (this.KeypadMessage.Length > 500)
            {
                MessageBox.Show("The message must be a maximum of 500 characters.", "Error Detail", MessageBoxButtons.OK, MessageBoxIcon.Error);
                messageTextBox.Focus();
                return;
            }

            // fire event
            if (Calculate != null)
            {
                Calculate(this, EventArgs.Empty);
            }
    }
}

class MessagePresenter
{
    private readonly IMessage view;  

    // store passed view for use in presenter
    public MessagePresenter(IMessage view) 
    {
        this.view = view;
        Initialize();
    }


    private void Initialize() 
    {
        // hook up view event
        view.Calculate += Calculate;
    }


    private void Calculate(object sender, EventArgs e) 
    {
        try
        {
            Message message = new Message();
            // exclude all punctuation and non alphanumeric characters
            message.KeypadMessage = Regex.Replace(view.KeypadMessage, "[^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<KeyCharacter> chars = coll.CreateCollection(keypad);

            KeyCharacter precedingChar = null;

            bool isFirst = true;

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

                // determine whether character is the intial one or is on the same key as the preceding one
                message.AddToMetrics(message, item, keyChar,
                    isFirst ? "Initial" :
                    keyChar.Number == precedingChar.Number ? "SameKeyAsPreceding" :
                    String.Empty);

                // hold current character for comparison
                precedingChar = keyChar;

                if (isFirst)
                    isFirst = false;
            }

            // set view properties to display ouput
            view.NoOfKeyPresses = message.NoOfKeyPresses;
            view.ApproxTime = message.ApproxTime;

        }
        catch (Exception ex)
        {
            MessageBox.Show("Unhandled exception. Please try again.");

            // TODO : log error using logger

            //this.Close();
        }
    }
}


class Message
{
    public const double letterKeyPressDelay = 0.3;
    public const double numberKeyHoldTime = 1;
    public const double sameKeyAsPrecedingDelay = 0.7;

    public string KeypadMessage { get; set; }
    public int NoOfKeyPresses { get; set; }
    public double ApproxTime { get; set; }


    public Message AddToMetrics(Message message, char messageChar, KeyCharacter keyChar, string type)
    {
        message.NoOfKeyPresses += keyChar.NumberOfPresses;

        if (char.IsDigit(messageChar))
        {
            // number   
            // there is a delay for a number because it requires holding, even when it is the initial press 
            message.ApproxTime += keyChar.NumberOfPresses * numberKeyHoldTime; // number of presses for a number will be 1
            if (type == "SameKeyAsPreceding")
            {
                // extra delay due to character being on same key
                message.ApproxTime += sameKeyAsPrecedingDelay; 
            }
        }
        else
        {
            // letter 
            if (type == "Initial" || type == "SameKeyAsPreceding")
            {
                // no delay for intial press if the character is a letter
                // subtract 1 as the very first key press initiates the message OR subtract 1 as the first key press is the one that has the extra delay due to being on the same key 
                message.ApproxTime += (keyChar.NumberOfPresses - 1) * letterKeyPressDelay; 
            }
            else
            {
                // the first key press after the last character has the standard delay 
                message.ApproxTime += (keyChar.NumberOfPresses) * letterKeyPressDelay; 
            }

            if (type == "SameKeyAsPreceding")
            {
                // extra delay due to character being on same key
                message.ApproxTime += sameKeyAsPrecedingDelay; 
            }

            // uppercase letter requires 1 extra keypress
            if (Char.IsUpper(messageChar))
            {
                message.NoOfKeyPresses += 1;
                // add standard key press delay 
                message.ApproxTime += letterKeyPressDelay;
            }
        }

        return message;
    }        
}


interface IMessage
{
    string KeypadMessage { get; set; }
    int NoOfKeyPresses { get; set; }
    double ApproxTime { get; set; }

    event EventHandler<EventArgs> Calculate;        
}


class CharacterCollection
{

    public List<KeyCharacter> CreateCollection(String delmitedKeypad)
    {
        List<KeyCharacter> list = new List<KeyCharacter>();

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

        for (int i = 0; i < keyCharacters.Length; i++)
        {
            String[] keyCharacterNumbersAndLetters = keyCharacters[i].Split(':');

            if (keyCharacterNumbersAndLetters.Length > 1)
            {
                // number and letter key
                for (int j = 0; j < keyCharacterNumbersAndLetters[1].Length; j++)
                {
                    list.Add(new KeyCharacter()
                    {
                        NumberOnly = false,
                        Number = Convert.ToInt32(keyCharacterNumbersAndLetters[0]),
                        Letter = keyCharacterNumbersAndLetters[1][j],
                        NumberOfPresses = j + 1
                    });
                }
            }
            else
            {
                list.Add(new KeyCharacter()
                {
                    NumberOnly = true,
                    Number = Convert.ToInt32(keyCharacterNumbersAndLetters[0]),                        
                    NumberOfPresses = 1
                });
            }                
        }

        return list;
    }


    public KeyCharacter GetKeyCharacter(List<KeyCharacter> characterCollection, char character)
    {
        KeyCharacter keyChar;
        if (char.IsDigit(character))
        {
            // number
            int number;
            if (!Int32.TryParse(character.ToString(), out number))
            {
                throw new InvalidOperationException();
            }

            keyChar = (KeyCharacter)characterCollection.Where(c => c.Number == number).FirstOrDefault();
        }
        else
        {
            // letter
            char chr = char.ToLower(character);
            keyChar = (KeyCharacter)characterCollection.Where(c => c.Letter == chr).FirstOrDefault();
        }
        return keyChar;
    }

}


class KeyCharacter
{
    public bool NumberOnly { get; set; }
    public int Number { get; set; } // for when preceding letter of message is on same key
    public char Letter { get; set; }
    public int NumberOfPresses { get; set; }
}

Answer 1

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

Answer 2

In terms of WinForms and OO/SOLID etc, the best thing you can do is extract your logic out into something like a service class and remove the logic from your 'Form' codebehind.

This will allow you to re-use it if you wanted to say, perform the same action but from perhaps a webservice or console application for example.

You can then de-couple your service/business logic from your form using IoC (Inversion of control) using and IoC container, and DI (Dependency injection).

This will then allow your form to be less tightly coupled and more testable, as you can write tests which mock or substitute out dependencies during testing.

One pattern I'd suggest looking at (since I used in in webforms recently) is something like MVP (Model View Presenter) - http://msdn.microsoft.com/en-us/library/ff649571.aspx as it allows you to de-couple your classes in a relatively standard manner.

EDIT In response to code ammendment:

// #### IS IT OK TO DO THIS ?????? ####

• Yes, simple conversions which would be done each time are quite appropriate inside view properties, especially if the conversion is specific to the type of controls in your view.

In terms of your MVP implementation I've not been through with the finest comb, but it looks pretty good, you've actually already performed and IoC action by allowing your presenter to operate on the view interface, and not on a concrete view object. What this allows, is your presenter to operate on ANY type which implements IMessage without knowing anything about what it's actually changing. E.g you could implement IMessage in a webform, and the presenter would work with that, without needing to know anything further.

I think there may be a mistake with your Calculate event handler in the presenter. This I believe ties you into a specific view type. What I would do, is bind a wrapper function in your winform, which calls presenter.Calculate, something like this:

WinForm

private void calculateButton_Click(object sender, EventArgs e) {
    presenter.Calculate();
}

Presenter

public void Calculate() {
    // bla
}

The aim is to make the presenter totally de-coupled from your view. If you have any references to anything winform specific in the presenter, it's not fully decoupled. You haven't done this, but a common mistake is to includes references to controls in the presenter, the presenter should operate on non view specific types, so largely primitives and your own custom DTO types, operating on a string in the presenter and alter a textbox.Text in your view, don't operate on a anything like TextBox in the presenter as you can't ensure it would be suitable for all views.