|
PomaPomaPoma wrote: Maybe you can modify your method to take this into account?
No, it's your project. If you want me to do your work email and we'll discuss rates.
I know the language. I've read a book. - _Madmatt
|
|
|
|
|
Hi All,
I’m teaching myself C# and I’ve written a small application that that I use to retrieve some computer information using WMI.
The application is working and returns the information I want. My question is, because I am just learning, would someone be willing to look at my code and suggest areas where I could improve what I have written?
I’m fairly sure that there are probably better more efficient ways to do what I’ve written.
Thanks
Rob
|
|
|
|
|
If you were to show say 20 to 40 lines of it (inside PRE tags!), you likely get feedback right here.
Luc Pattyn [Forum Guidelines] [Why QA sucks] [My Articles]
I only read code that is properly formatted, adding PRE tags is the easiest way to obtain that. [The QA section does it automatically now, I hope we soon get it on regular forums as well]
|
|
|
|
|
Here is some of the code I used to get some BIOS info and display it.
private void SearchWMI_Data(string hostName)
{
this.listBox1.Items.Clear();
this.queueStack.Clear();
string myString = "";
string target = hostName;
long ramSize = 0;
IPAddress[] myIP = Dns.GetHostAddresses(target);
try
{
if (!this.ValidateHostInput(hostName)) return;
{
ManagementScope scope = new ManagementScope("\\\\" + target + "\\root\\cimv2");
ManagementPath pathBios = new ManagementPath("Win32_Bios");
ManagementPath pathSystem = new ManagementPath("Win32_ComputerSystem");
ObjectGetOptions obj = new ObjectGetOptions(null);
ManagementClass wmiBios = new ManagementClass(scope, pathBios, obj);
ManagementClass wmiSystem = new ManagementClass(scope, pathSystem, obj);
foreach (IPAddress theIP in myIP)
{
queueStack.Enqueue("IP Address: " + theIP.ToString());
}
foreach (ManagementObject bios in wmiBios.GetInstances())
{
queueStack.Enqueue("");
queueStack.Enqueue("=== BIOS Info ===");
try
{
queueStack.Enqueue("Manufacturer: " + bios.Properties["Manufacturer"].Value.ToString().Trim());
}
catch
{
queueStack.Enqueue("Manufacturer: Error reading status");
}
try
{
queueStack.Enqueue("SN: " + bios.Properties["Serialnumber"].Value.ToString().Trim());
}
catch
{
queueStack.Enqueue("SN: Error reading status");
}
}
while (this.queueStack.Count > 0)
{
this.listBox1.Items.Add(this.queueStack.Dequeue());
}
}
txtBoxHostName.Focus();
}
catch
{
this.listBox1.Items.Add("Unable to find the specified computer: " + hostName.ToUpper());
txtBoxHostName.Focus();
}
}
modified on Wednesday, February 3, 2010 12:27 PM
|
|
|
|
|
Hi again,
looks pretty good overall. I have a couple of remarks:
functionality & behavior
1.
Right now your code first gathers all info, then displays it. It might be more user-friendly to intertwine those actions, so you can see the info parts as soon as they are available. More on this later.
structure
2.
You might want to split the functionality into a method gathering information (maybe returning a list of strings), and one displaying the information. So you later on can more easily change one or the other. Disadvantage: you don't see anything until all has been gathered. Possible fix: make a gather method and give it a delegate, so it can report without knowing how the report is actually done (works like a function pointer in C/C++). My Sokoban article is an example, search for "Logger".
coding
3.
your overall coding style is OK.
4.
A queue is a bit of an odd choice, a simple List<string> would behave identically here. Nothing wrong though.
5.
Objects (such as those ManagementClass instances) that have a Dispose() method and are no longer needed should be disposed of. So your code should have a try-finally where the finally tests for null and disposes. This improves the memory footprint, and for some classes is needed for correct operation. Not disposing is (almost) a bug.
6.
it would be more logical to do input validation first, so move ValidateHostInput(hostName) up. Also check the textbox contents early on.
7.
in .NET one normally throws exceptions on bad inputs, rather than just returning. Only in special cases, .NET also provides a non-throwing alternative (see int.Parse and int.TryParse).
8.
I find it strange the line IPAddress[] myIP = Dns.GetHostAddresses(target); was left outside the try block.
9.
Having try-catch without explicitly specifying exception type isn't good practice. I tend to report at two levels: a functional level (which you have), and a technical level, probably providing the whole Exception.ToString() result, which is many lines, but would help the technical people to pinpoint what caused the problem.
You passed; up to the next level!
Luc Pattyn [Forum Guidelines] [Why QA sucks] [My Articles]
I only read code that is properly formatted, adding PRE tags is the easiest way to obtain that. [The QA section does it automatically now, I hope we soon get it on regular forums as well]
|
|
|
|
|
1. Not sure how to approach this.
2. Not entirely sure how to do this, will need to read your article.
3. Thanks
4. This was done for a few reasons,
>1. I was having problems formatting the output and
>2. I had recently finished reading the chapter on stacks….
Guess its time to go back and readdress the <string> again more especially the list<string>
5. I will need to look into the Dispose() method and will add this to the code.
6. I have moved the ValidateHostInput(hostname) to the top as suggested.
7. I do have a method that handles exceptions for bad inputs but I don’t think that this is what you are referring to.
private bool ValidateHostInput(string userInput)
{
if (userInput.Length == 0)
{
MessageBox.Show("You must enter a Host Name \n or IP Address", "ERROR", MessageBoxButtons.OK, MessageBoxIcon.Error);
return false;
}
return true;
}
8. This was just an oversight on my part and has been corrected.
9. Yes, this would be nice to include. Yet another thing I will need to look into…
Thank you very much for great feedback
Only question now is what part to start on first?
|
|
|
|
|
you're welcome.
eeffoc42 wrote: 4. This was done for a few reasons,
>1. I was having problems formatting the output and
Huh? you create a string, enqueue it, dequeue it, and add it to the ListBox.
Creating the string and adding it to the ListBox would result in the same net result, the queue doesn't change anything here.
eeffoc42 wrote: 7. I do have a method that handles exceptions
I'm not particularly fond of MessageBox, I tend to include reporting functionality (progress, status, errors, ...) in my active form, but that is a matter of taste. I only consider MessageBoxes acceptable for the very exceptional stuff, most certainly not for progress! It is not my job to tell the computer to go and continue all the time...
eeffoc42 wrote: 1. Not sure how to approach this
eeffoc42 wrote: 2. Not entirely sure how to do this
Not tested, just indicative:
public List<string> Gather() {
List<string> list=new List<string>();
for(int i=0; i<10; i++) list.Add("string number "+i.ToString());
return list;
}
public void ShowResults(IEnumerable<string> list) {
foreach(string s in list) myListBox.Items.Add(s);
}
and
public void Gather(Action<string> reporter) {
for(int i=0; i<10; i++) {
string s="string number "+i.ToString();
if (reporter!=null) reporter(s);
}
}
public void myReporter(string s) {
myListBox.Items.Add(s);
}
public void GatherAndReportIntertwined() {
Gather(new Action<string>(myReporter));
}
both examples illustrate separation of what you gather and how you present the results.
eeffoc42 wrote: what part to start on first?
that's up to you. Most issues are minor edits, replacing the queue stuff by a delegate-based approach is the only major one. It will shorten and clarify your code a lot, especially if you create and use a simple report method; example:
public void report(string s) {
if (reporter!=null) reporter(s);
}
where reporter now is a class member that gets initialized by this.reporter=reporter; at the start of Gather() .
Luc Pattyn [Forum Guidelines] [Why QA sucks] [My Articles]
I only read code that is properly formatted, adding PRE tags is the easiest way to obtain that. [The QA section does it automatically now, I hope we soon get it on regular forums as well]
|
|
|
|
|
This is Gr8... I think you can post it as an article... for getting proper feed back from others.
Thanks
Md. Marufuzzaman
I will not say I have failed 1000 times; I will say that I have discovered 1000 ways that can cause failure – Thomas Edison.
|
|
|
|
|
|
i am arunprasath . i am working c# developer trainee . just one month before i joined . my problem is i don't know about c# environment. so any can tell me about how can i improve my c# skills.and also how can i prepare c# language
|
|
|
|
|
Several options that I use
1) College Courses - They help but sometimes the professor has a different agenda than teaching you what it is you seek to learn. What I mean is you have to adventure on your own. I found college was more about theory and concepts. Anyway, a basic c# class would get your started with understanding things like foreach, classes, so for forth and so on....
2) Online Video Training
I highly recommend this. I personally use learn visual studio dot net videos[^] They are to the point and very informative. Check it out.
3) Books are always your friend. I know my library has several books on Visual c# 2008 so check those out.
4) And of course there is always this forum to ask your specific questions. It is however best to post some of the code that you are working on so people get the idea that you are trying and are able to understand your logic and offer suggestions.
I hope this helps...
|
|
|
|
|
Since you are new you need to read and understand the rules of this community to get most from it and save yourself from being flamed or ignored.
How to get an answer to your question[^]
Review item #3. "hello every one" is not a descriptive subject.
I know the language. I've read a book. - _Madmatt
|
|
|
|
|
Read a C# book.
Me, I'm dishonest. And a dishonest man you can always trust to be dishonest. Honestly. It's the honest ones you want to watch out for...
|
|
|
|
|
|
hi.
Wamuti: Any man can be an island, but islands to need water around them!
Edmund Burke: No one could make a greater mistake than he who did nothing because he could do only a little.
|
|
|
|
|
I believe I'm going about this the wrong way and was wondering if there is an easier way to do this. It just appears to be a lot of functions to call for what I would assume would be a simple process.
Here is a break down of what I'm trying to accomplish:
see db diagram
I want to simply make an Insert_Update functionality when the save button is pressed on a linq to sql c# winform application. I have the insert working per say but I don't think it is the correct way to do it so I'm wondering if there is a better method than doing all the checks separately and calling db.submit() multiple times.
private void saveToolStripButton_Click(object sender, EventArgs e)
{
CheckAuthor(authorFirstTextBox.Text, authorLastTextBox.Text);
}
public void CheckAuthor(String FirstName, String LastName)
{
using (var db = new momdbDataContext())
{
try
{
StatusLabel.Text = "Adding Item to database";
List<Author> authorList = new List<Author>();
bool exist = db.Authors.Any(a => a.AuthorFirst == FirstName && a.AuthorLast == LastName);
if (exist)
CheckBookExist_ADD();
else
{
Author authorInsert = new Author { AuthorFirst = authorFirstTextBox.Text, AuthorLast = authorLastTextBox.Text };
authorList.Add(authorInsert);
db.Authors.InsertAllOnSubmit(authorList);
db.SubmitChanges();
Debug.WriteLine(db.Log);
CheckBookExist_ADD();
}
}
catch (Exception oops)
{
MessageBox.Show(oops.ToString());
}
}
}
public void CheckBookExist_ADD()
{
using (var db = new momdbDataContext())
{
var originalBook = db.Books.Where(t => t.Title == this.titleTextBox.ToString()).FirstOrDefault();
List<Book> bookList = new List<Book>();
bool exist = db.Books.Any(b => b.Title == this.titleTextBox.Text);
if (exist)
{
AddBookNumber(originalBook.ID);
var AuthorID = (from a in db.Authors
select a.ID).Max();
CheckBookAuthor(originalBook.ID, AuthorID);
}
else
{
Book bookInsert = new Book
{
Title = this.titleTextBox.Text,
Keywords = keywordsTextBox.Text,
Price = Convert.ToDecimal(priceTextBox.Text),
Retired = retiredCheckBox.Checked
};
bookList.Add(bookInsert);
db.Books.InsertAllOnSubmit(bookList);
db.SubmitChanges();
var bookID = (from b in db.Books
select b.ID).Max();
var AuthorID = (from a in db.Authors
select a.ID).Max();
CheckBookAuthor(bookID, AuthorID);
}
}
}
public void AddBookNumber(int ID)
{
using (var db = new momdbDataContext())
{
var max_Surrogate = (from sn in db.BookNumbers
select sn.SurrogateNumber).Max();
List<BookNumber> bookNumberList = new List<BookNumber>();
BookNumber booknumberInsert = new BookNumber { BookID = ID, SurrogateNumber = max_Surrogate + 1 };
bookNumberList.Add(booknumberInsert);
db.BookNumbers.InsertAllOnSubmit(bookNumberList);
db.SubmitChanges();
}
}
public void CheckBookAuthor(int bid, int aid)
{
using (var db = new momdbDataContext())
{
List<BookAuthor> bookAuthorList = new List<BookAuthor>();
bool exist = db.BookAuthors.Any(a => a.BookID == bid && a.AuthorID == aid);
if (exist)
return;
else
{
BookAuthor bookauthorInsert = new BookAuthor { AuthorID = aid, BookID = bid };
bookAuthorList.Add(bookauthorInsert);
db.BookAuthors.InsertAllOnSubmit(bookAuthorList);
db.SubmitChanges();
}
StatusLabel.Text = "Add Complete";
LoadData();
}
}
Please keep in mind that I'm learning this stuff so please don't bash me too hard. Thanks...
modified on Wednesday, February 3, 2010 4:18 PM
|
|
|
|
|
Post the relevant bit of code you have questions about, and be sure to use the pre tags, no one is going to link outside to look at it.
I know the language. I've read a book. - _Madmatt
|
|
|
|
|
Hi All,
I have created a form and placed an image on its back ground. Now the form controls take much time to load.
Is there any way by which we can do this thing quickly.
thanks
Syed Shahid Hussain
|
|
|
|
|
Pick one:
1) Don't put an image on it's background.
2) If you must, then make it a small one.
3) Don't load the image until all the controls are loaded.
4) Get a faster machine.
All those who believe in psycho kinesis, raise my hand.
My 's gonna unleash hell on your ass. tastic!
|
|
|
|
|
I want to give a background image that is very necessary but in vb application it is working very fast but in c# it takes too much time.
I'm using core 2Dou 2.0 GHz.
Syed Shahid Hussain
|
|
|
|
|
Then that has to be (pretty much) down to differences in your code, doesn't it? Since VB and C# both use the same .NET framework and runtime, there has to be a significant difference between the two programs.
What are you doing in the C# that you aren't in the VB?
All those who believe in psycho kinesis, raise my hand.
My 's gonna unleash hell on your ass. tastic!
|
|
|
|
|
sorry I was talking about VB6. It its loading speed is too fast.
And I want to develop an on screen key board. Do u also have any idea about that? Thanks
Syed Shahid Hussain
|
|
|
|
|
CodeProject holds a couple of articles on virtual keyboards. I suggest you search and read them.
Luc Pattyn [Forum Guidelines] [Why QA sucks] [My Articles]
I only read code that is properly formatted, adding PRE tags is the easiest way to obtain that. [The QA section does it automatically now, I hope we soon get it on regular forums as well]
|
|
|
|
|
VB6 I can't help with - I bailed out of VB around Version 2 - but I assume VB6 is native code rather than .NET? If so, then I would expect it to load a lot faster, as there is no large framework to worry about. C# need the framework, so will load slower.
Having said that, just how many controls do you have? And what size/format is the image?
It may be worth just loading the app without the image and seeing if that speeds up the load enough. If it doesn't then I suspect you may have to go the "put-up-a-splash-screen-so-no-one-notices-it-is-so-slow" route that Adobe tried...
See Lucs' answer re the virtual keyboard.
All those who believe in psycho kinesis, raise my hand.
My 's gonna unleash hell on your ass. tastic!
|
|
|
|
|
Thanks a lot Friend
Syed Shahid Hussain
|
|
|
|
|
Pleased I have small problem about controls and class.
I have my new Winform class with a single label in my project and I want to write information in this label in another non GUI class I don't know how I can proceed
Exemple:
public partial class Form1:Form
{
privata Server serv = new Server(); //Form1 has-a Server
....
}
This is my non UI class
public class Server
{
public Server(){}
}
|
|
|
|