473,395 Members | 1,613 Online
Bytes | Software Development & Data Engineering Community
Post Job

Home Posts Topics Members FAQ

Join Bytes to post your question to a community of 473,395 software developers and data experts.

Is this poor C# programming?

JM
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",filesTe mp.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
Nov 16 '05 #1
4 2216
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

Nov 16 '05 #2
SP
"JM" <JM@discussions.microsoft.com> wrote in message
news:B4**********************************@microsof t.com...
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",filesTe mp.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
Nov 16 '05 #3
>
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.
--
Nov 16 '05 #4
Nick Malik [Microsoft] <ni*******@hotmail.nospam.com> wrote:

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

--
Jon Skeet - <sk***@pobox.com>
http://www.pobox.com/~skeet
If replying to the group, please do not mail me too
Nov 16 '05 #5

This thread has been closed and replies have been disabled. Please start a new discussion.

Similar topics

30
by: Michael B Allen | last post by:
I have a general purpose library that has a lot of checks for bad input parameters like: void * linkedlist_get(struct linkedlist *l, unsigned int idx) { if (l == NULL) { errno = EINVAL;...
4
by: Bill Thorne | last post by:
We have a COM object that has been wrappered for use in .NET and which can make calls which can take a while to execute. These are mainframe integration calls that might perform a lot of data...
20
by: John Mark Howell | last post by:
I had a customer call about some C# code they had put together that was handling some large arrays. The performance was rather poor. The C# code runs in about 22 seconds and the equivalent...
11
by: John Fly | last post by:
I'm working on a large project(from scratch). The program is essentially a data file processor, the overall view is this: A data file is read in, validated and stored in a memory structure...
4
by: Jim Devenish | last post by:
I have converted an Access back-end to SQL Server back-end but am having some problems. The Access to Access application has been running well for some years. I have successfully copied all the...
49
by: utab | last post by:
Why is for(int i=0; i < 100; ++i) poor?
40
by: JeffP | last post by:
Is this a holiday week? There are way too many unanswered posts! I dont' care if the person asked a dumb or un-comprehensible question, they deserve a response even if it is to clarify or...
12
by: pittendrigh | last post by:
Let's say we're trying to keep blog and forum spammers out of our site--we're not trying to protect fort knox. 1) Step one is a one-time-only step. We create six different css files that define...
4
by: joa2212 | last post by:
Hello everybody, I'm posting this message because I'm quiet frustrated. We just bought a software from a small software vendor. In the beginning he hosted our application on a small server at...
0
by: Charles Arthur | last post by:
How do i turn on java script on a villaon, callus and itel keypad mobile phone
0
by: ryjfgjl | last post by:
In our work, we often receive Excel tables with data in the same format. If we want to analyze these data, it can be difficult to analyze them because the data is spread across multiple Excel files...
0
BarryA
by: BarryA | last post by:
What are the essential steps and strategies outlined in the Data Structures and Algorithms (DSA) roadmap for aspiring data scientists? How can individuals effectively utilize this roadmap to progress...
1
by: nemocccc | last post by:
hello, everyone, I want to develop a software for my android phone for daily needs, any suggestions?
1
by: Sonnysonu | last post by:
This is the data of csv file 1 2 3 1 2 3 1 2 3 1 2 3 2 3 2 3 3 the lengths should be different i have to store the data by column-wise with in the specific length. suppose the i have to...
0
by: Hystou | last post by:
There are some requirements for setting up RAID: 1. The motherboard and BIOS support RAID configuration. 2. The motherboard has 2 or more available SATA protocol SSD/HDD slots (including MSATA, M.2...
0
by: Hystou | last post by:
Most computers default to English, but sometimes we require a different language, especially when relocating. Forgot to request a specific language before your computer shipped? No problem! You can...
0
Oralloy
by: Oralloy | last post by:
Hello folks, I am unable to find appropriate documentation on the type promotion of bit-fields when using the generalised comparison operator "<=>". The problem is that using the GNU compilers,...
0
agi2029
by: agi2029 | last post by:
Let's talk about the concept of autonomous AI software engineers and no-code agents. These AIs are designed to manage the entire lifecycle of a software development project—planning, coding, testing,...

By using Bytes.com and it's services, you agree to our Privacy Policy and Terms of Use.

To disable or enable advertisements and analytics tracking please visit the manage ads & tracking page.