patterncsharpMinor
Simple IPv4 and port validation for chat client
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.
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
Here the new methods
This method now immediately states what it is doing: it's validating the text in
Repetition
Now look at the second method. You'll see that
We can almost use
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
An alternative, which you may not have known about, is
Simplifying
If you ever have an
You should simplify this to:
In your case, you can do:
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 statementsIf 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 < 65535Code 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 < 65535Context
StackExchange Code Review Q#58119, answer score: 4
Revisions (0)
No revisions yet.