Is this poor C# programming?

[Guest]

Hi

I have a Windows Form that I have 3 textboxes and some buttons. Below is
the code that I have implemented it reads a pile of files from a folder and
then reads each of the files. If the McAfee Dat file version is old (older
than 15 revisions) then it will output the machine name, Scan Engine, and dat
file version (all of this is each of the text files that I load)

Is the way that I coded it poor C# OOP? I mean I have a registry class that
I have borrowed and implemented "using" so that I have access to the class.
Is it a bad practice to have so much code after a button like I do in my Run
button??

BTW I am using C# Express 2005 Beta 1....

#region Using directives

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Windows.Forms;
using Utility.ModifyRegistry; // Class borrowed from the Internet
using System.IO;

#endregion

namespace McAfeeVersionCheck
{
partial class MainForm : Form
{

// Create an instance of the the registry class. Needs to be here
// because of the Save button (which saves the stuff to Registry)
(SCOPE)
ModifyRegistry myRegistry = new ModifyRegistry();

public MainForm()
{
InitializeComponent();
}

private void buttonExit_Click(object sender, EventArgs e)
{
Application.Exit(); // Close Program
}

private void MainForm_Load(object sender, EventArgs e)
{
// Create an instance of the class.
//ModifyRegistry myRegistry = new ModifyRegistry();

// Creates strings and stores a value for each
// Registry reads from HKLM\SOFTWARE\<ProgramName>
string strMcAfeeVersion = myRegistry.Read("McAfeeVersion");
string strScanEngine = myRegistry.Read("ScanEngine");
string strDatFile = myRegistry.Read("DatFile");

if ((strMcAfeeVersion != null && strScanEngine != null &&
strDatFile != null) && (strMcAfeeVersion != "" || strScanEngine != "" ||
strDatFile != ""))
{
// Place the values on the form from the registry
textBoxMcAfeeVersion.Text = strMcAfeeVersion;
textBoxScanEngine.Text = strScanEngine;
textBoxDatVersion.Text = strDatFile;
buttonRun.Enabled = true;
}
else //null or empty
{
MessageBox.Show("This must be the first time that you have
used this program. Please set the values in the text boxes and click the
Save Button");
}
}

private void buttonSave_Click(object sender, EventArgs e)
{

if ((textBoxMcAfeeVersion.Text == null || textBoxScanEngine.Text
== null || textBoxDatVersion.Text == null) || (textBoxMcAfeeVersion.Text ==
"" || textBoxScanEngine.Text == "" || textBoxDatVersion.Text == ""))
{
MessageBox.Show("Please fill out all of the text boxes
before trying to Save");
}
else
{
myRegistry.Write("McAfeeVersion", textBoxMcAfeeVersion.Text);
myRegistry.Write("ScanEngine", textBoxScanEngine.Text);
myRegistry.Write("DatFile", textBoxDatVersion.Text);
}
}

private void buttonRun_Click(object sender, EventArgs e)
{
if ((textBoxMcAfeeVersion.Text == null ||
textBoxScanEngine.Text == null || textBoxDatVersion.Text == null) ||
(textBoxMcAfeeVersion.Text == "" || textBoxScanEngine.Text == "" ||
textBoxDatVersion.Text == ""))
{
MessageBox.Show("Please fill out all of the text boxes
before trying to Run");
}
else
{
DirectoryInfo Dir = new DirectoryInfo(@"c:\SVirus_ver");
FileInfo[] txtFiles = Dir.GetFiles();

// Print out the names of the files in the current directory.
foreach (FileInfo filesTemp in txtFiles)
//MessageBox.Show(filesTemp.Name);
{
try
{
// Create an instance of StreamReader to read from
a file.
// The using statement also closes the StreamReader.
using (StreamReader sr = new
StreamReader(Path.Combine(@"c:\SVirus_ver",filesTemp.Name)))
{
String line;
// Read and display lines from the file until
the end of
// the file is reached.
while ((line = sr.ReadLine()) != null)
{

// Remove spaces
string RemovedSpacesLine = line.Replace(",
",",");

// Split the line into an string array
string[] LineFields =
RemovedSpacesLine.Split(',');

// Dat File, some are 8, some are 4. Trim
the long ones.
if (LineFields[9].Length > 4)
LineFields[9] =
LineFields[9].Substring(4, (LineFields[9].Length - 4));

// Convert the Array Member to an int
int DatVersion =
Convert.ToInt32(LineFields[9]);

// Create the limit for the dat files.
int DatFileLowLimit =
(Convert.ToInt32(textBoxDatVersion.Text) - 15);

if (DatVersion < DatFileLowLimit)
{
// Computer Name
MessageBox.Show(LineFields[3]);

// Software Version
MessageBox.Show(LineFields[8]);

// Scan Engine
MessageBox.Show(LineFields[11]);

// Dat File
MessageBox.Show(LineFields[9]);
}

}
} // End of Using

} // End of Try
catch (Exception except)
{
// Let the user know what went wrong.
//MessageBox.Show("The file could not be read:");
//MessageBox.Show(except.Message);
}
} // End of ForEach
} // End of If Else
} // End of Button Click
} // End of Class
} // End NameSpace

 

[carl.manaster]

Hi, JM,

To me, the deep nesting indicates too much complexity in one function.
That kind of complexity makes it hard to debug and hard to reuse code
elements. So I would refactor it. Step one might look like this:

private void ShowStuff(string[] LineFields)
{
// Computer Name
MessageBox.Show(LineFields[3]);
// Software Version
MessageBox.Show(LineFields[8]);
// Scan Engine
MessageBox.Show(LineFields[11]);
// Dat File
MessageBox.Show(LineFields[9]);
}

which doesn't help very much on its own, but it's a start. You might
go even deeper:

private string ComputerName(string[] LineFields)
{
return LineFields[3];
}
private string SoftwareVersion(string[] LineFields)
{
return LineFields[8];
}
private string ScanEngine(string[] LineFields)
{
return LineFields[11];
}
private string DatFile(string[] LineFields)
{
return LineFields[9];
}
private void ShowStuff(string[] LineFields)
{
MessageBox.Show(ComputerName(LineFields));
MessageBox.Show(SoftwareVersion(LineFields));
MessageBox.Show(ScanEngine(LineFields));
MessageBox.Show(DatFile(LineFields));
}

eliminating the need for some comments while preserving clarity.

This starts to suggest a class is waiting to be born, something that
would hold LineFields and be able to report from it, for example, the
ComputerName and SoftwareVersion; you could feed the "line" variable
into it in its constructor, and let it prune out the spaces and split
along the commas. You might find you have a use for that class
somewhere else; it wouldn't be so tightly tied to this particular Form.

Another approach to this might be to declare the indexes into the
LineFields arrays as constants:
const int ComputerName = 3;
const int SoftwareVersion = 8;
etc.

This would still work quite well with the separate class.

HTH,
--Carl

 

[SP]

Hi

I have a Windows Form that I have 3 textboxes and some buttons. Below is
the code that I have implemented it reads a pile of files from a folder
and
then reads each of the files. If the McAfee Dat file version is old
(older
than 15 revisions) then it will output the machine name, Scan Engine, and
dat
file version (all of this is each of the text files that I load)

Is the way that I coded it poor C# OOP? I mean I have a registry class
that
I have borrowed and implemented "using" so that I have access to the
class.
Is it a bad practice to have so much code after a button like I do in my
Run
button??

BTW I am using C# Express 2005 Beta 1....

#region Using directives

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Windows.Forms;
using Utility.ModifyRegistry; // Class borrowed from the Internet
using System.IO;

#endregion

namespace McAfeeVersionCheck
{
partial class MainForm : Form
{

// Create an instance of the the registry class. Needs to be here
// because of the Save button (which saves the stuff to Registry)
(SCOPE)
ModifyRegistry myRegistry = new ModifyRegistry();

public MainForm()
{
InitializeComponent();
}

private void buttonExit_Click(object sender, EventArgs e)
{
Application.Exit(); // Close Program
}

private void MainForm_Load(object sender, EventArgs e)
{
// Create an instance of the class.
//ModifyRegistry myRegistry = new ModifyRegistry();

// Creates strings and stores a value for each
// Registry reads from HKLM\SOFTWARE\<ProgramName>
string strMcAfeeVersion = myRegistry.Read("McAfeeVersion");
string strScanEngine = myRegistry.Read("ScanEngine");
string strDatFile = myRegistry.Read("DatFile");

if ((strMcAfeeVersion != null && strScanEngine != null &&
strDatFile != null) && (strMcAfeeVersion != "" || strScanEngine != "" ||
strDatFile != ""))

Why not have this testing as part of the class, e.g.

if(! myRegistry.hasNullOrEmptyValues)

This would also eliminate the temporary variables...

{
// Place the values on the form from the registry
textBoxMcAfeeVersion.Text = strMcAfeeVersion;

with...
textBoxMcAfeeVersion.Text =
myRegistry.Read("McAfeeVersion");

textBoxScanEngine.Text = strScanEngine;
textBoxDatVersion.Text = strDatFile;
buttonRun.Enabled = true;
}
else //null or empty
{
MessageBox.Show("This must be the first time that you have
used this program. Please set the values in the text boxes and click the
Save Button");
}
}

private void buttonSave_Click(object sender, EventArgs e)
{

If you have an observer on the button then there would be no need to test
when they click because it would
be disabled until they have filled in the textboxes. I also think that
Text.Length != 0 should cover what you need in the observer.

if ((textBoxMcAfeeVersion.Text == null ||
textBoxScanEngine.Text
== null || textBoxDatVersion.Text == null) || (textBoxMcAfeeVersion.Text
==
"" || textBoxScanEngine.Text == "" || textBoxDatVersion.Text == ""))
{
MessageBox.Show("Please fill out all of the text boxes
before trying to Save");
}
else
{
myRegistry.Write("McAfeeVersion",
textBoxMcAfeeVersion.Text);
myRegistry.Write("ScanEngine", textBoxScanEngine.Text);
myRegistry.Write("DatFile", textBoxDatVersion.Text);
}
}

private void buttonRun_Click(object sender, EventArgs e)
{

same here with the observer

if ((textBoxMcAfeeVersion.Text == null ||
textBoxScanEngine.Text == null || textBoxDatVersion.Text == null) ||
(textBoxMcAfeeVersion.Text == "" || textBoxScanEngine.Text == "" ||
textBoxDatVersion.Text == ""))
{
MessageBox.Show("Please fill out all of the text boxes
before trying to Run");
}
else
{
DirectoryInfo Dir = new DirectoryInfo(@"c:\SVirus_ver");
FileInfo[] txtFiles = Dir.GetFiles();

// Print out the names of the files in the current
directory.
foreach (FileInfo filesTemp in txtFiles)
//MessageBox.Show(filesTemp.Name);
{
try
{
// Create an instance of StreamReader to read from
a file.
// The using statement also closes the
StreamReader.
using (StreamReader sr = new
StreamReader(Path.Combine(@"c:\SVirus_ver",filesTemp.Name)))
{
String line;
// Read and display lines from the file until
the end of
// the file is reached.
while ((line = sr.ReadLine()) != null)
{

// Remove spaces
string RemovedSpacesLine = line.Replace(",
",",");

// Split the line into an string array
string[] LineFields =
RemovedSpacesLine.Split(',');

// Dat File, some are 8, some are 4. Trim
the long ones.
if (LineFields[9].Length > 4)
LineFields[9] =
LineFields[9].Substring(4, (LineFields[9].Length - 4));

// Convert the Array Member to an int
int DatVersion =
Convert.ToInt32(LineFields[9]);

// Create the limit for the dat files.
int DatFileLowLimit =
(Convert.ToInt32(textBoxDatVersion.Text) - 15);

if (DatVersion < DatFileLowLimit)
{
// Computer Name
MessageBox.Show(LineFields[3]);

// Software Version
MessageBox.Show(LineFields[8]);

// Scan Engine
MessageBox.Show(LineFields[11]);

// Dat File
MessageBox.Show(LineFields[9]);
}

}
} // End of Using

} // End of Try
catch (Exception except)
{
// Let the user know what went wrong.
//MessageBox.Show("The file could not be read:");
//MessageBox.Show(except.Message);
}
} // End of ForEach
} // End of If Else
} // End of Button Click
} // End of Class
} // End NameSpace

Final comment - far too many comments!!! A comment is often a sign to
extract a method and a whole bunch of comments is probably asking for a
class.

SP

 

[Nick Malik [Microsoft]]

Final comment - far too many comments!!! A comment is often a sign to
extract a method and a whole bunch of comments is probably asking for a
class.

SP

To SP: I disagree. A comment that illustrates a complicated snafu in the
code may be a sign that the code should be refactored. OTOH, a list of
comments that appear derived from psuedocode may simply be a sign of an
organized mind.

To JM: Typically, comments are most useful when they explain "why" the code
is what it is, or "what purpose" a section of code has. Comments on every
line is probably not an efficient use of your time. That said, there is
NOTHING WRONG with it.

--
--- Nick Malik [Microsoft]
MCSD, CFPS, Certified Scrummaster
http://blogs.msdn.com/nickmalik

Disclaimer: Opinions expressed in this forum are my own, and not
representative of my employer.
I do not answer questions on behalf of my employer. I'm just a
programmer helping programmers.
--

 

[Jon Skeet [C# MVP]]

To JM: Typically, comments are most useful when they explain "why" the code
is what it is, or "what purpose" a section of code has. Comments on every
line is probably not an efficient use of your time. That said, there is
NOTHING WRONG with it.

I disagree - when comments are there when they don't need to be, they
make the really *useful* comments less easily visible, and they
distract from the code.

The rules I use are:

1) Comment every member, except private fields which have direct
property equivalents - and then make doubly sure you comment the
properties.

2) Add comments within the code if it's not immediately clear what that
code is doing, especially if it would otherwise look like it could be
changed or removed to simplify things.