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

Simple IPv4 and port validation for chat client

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

Problem

This is part of my chat client's code. It works fine but it looks messy. Any ideas?

//IP Validation
private void textBox3_Leave(object sender, EventArgs e)
{
    IPAddress ip;
    bool ValidateIP = IPAddress.TryParse(textBox3.Text, out ip);
    if (ValidateIP)
    {
        button2.Enabled = true;
        button3.Enabled = true;
    }
    else
    {
        //Disables buttons and locks into text box
        textBox3.Text = "Invalid IPv4 address!!";
        textBox3.Select();
        button2.Enabled = false;
        button3.Enabled = false;
    }
}

//Port Validation
private void textBox4_Leave(object sender, EventArgs e)
{
    bool ValidatePort;
    int porttest = 0;
    try
    {
        porttest = Convert.ToInt32(textBox4.Text);
    }
    catch (Exception)
    {           
    }

    if (porttest > 1 && porttest < 65535)
    {
        ValidatePort = true;
    }
    else
    {
        ValidatePort = false;
    }

    if (ValidatePort)
    {
        button2.Enabled = true;
        button3.Enabled = true;
    }
    else
    {
        //Disables buttons and locks into text box
        textBox4.Text = "Invalid Port";
        textBox4.Select();
        button2.Enabled = false;
        button3.Enabled = false;
    }
}

Solution

Naming

By convention, variables local to a method should be camelCased. You seem to have a mixture of PascalCased and alllowercase, making it hard to see at a glance what a variable is. All lower case has the additional disadvantage of being hard to read.

Your boolean check variables also seem to be named in the form 'VerbNoun'. This is rather confusing because it's usually how methods are named. It sounds like they're supposed to do something. ValidatePort for example sounds like something that validates a port. A name like portValid or perhaps isPortValid more clearly states what the variables are.

Extracting methods

Looking at both of your methods, they have the same structure: Check if something is valid, if it is then change the state of the form one way, if it isn't then change the state of the form a different way. Each of those are basically completely separate pieces of functionality, so I'd suggest separating them out. This would turn textBox3_Leave, for example, into:

private void textBox3_Leave(object sender, EventArgs e)
{
   bool ipValid = ValidateIP(textbox3.Text);
   if(ipValid)
       EnableControls();
    else
       DisableControls();
}


Here the new methods ValidateIP, EnableControls and DisableControls would be private methods containing the bits of code they replace.

This method now immediately states what it is doing: it's validating the text in textbox3as an IP, if it's valid then enable the controls, otherwise disable them. Potentially you could get rid of ipValid and just put that ValidateIP call directly into the if statement, it's up to you what you think reads better. You may also be able to improve on those names a bit (enable which controls, for example).

Repetition

Now look at the second method. You'll see that EnableControls can be used here too. This means that just by making the first method a clearer statement of what it's doing, we've discovered a valuable abstraction, and we no longer have to repeat that enabling controls means enabling button2 and button3.

We can almost use DisableControls too, but there's a slight difference- the messages. So to fix that, DisableControls should take the message as a string parameter:

private void DisableControls(string message);


Integer parsing

It's generally considered bad to use exceptions in normal control flow. That's what you're doing here when you try to check whether the text in textbox4 is a valid integer or not.

An alternative, which you may not have known about, is Int32.TryParse. This method is designed for this exact situation- where you want to check whether a string represents an integer, but don't want to throw an exception if it isn't. This allows you to get rid of that try...catch and clean that bit of code up a bit.

Simplifying if statements

If you ever have an if statement which reads:

if(condition)
{
    value = true;
}
else
{
    value = false;
}


You should simplify this to:

value = condition;


In your case, you can do:

ValidatePort = porttest > 1 && porttest < 65535

Code Snippets

private void textBox3_Leave(object sender, EventArgs e)
{
   bool ipValid = ValidateIP(textbox3.Text);
   if(ipValid)
       EnableControls();
    else
       DisableControls();
}
private void DisableControls(string message);
if(condition)
{
    value = true;
}
else
{
    value = false;
}
value = condition;
ValidatePort = porttest > 1 && porttest < 65535

Context

StackExchange Code Review Q#58119, answer score: 4

Revisions (0)

No revisions yet.