.NET Forum / Windows Forms / WinForm General / June 2007
My first C# project
|
|
Thread rating:  |
Arjen Logghe - 11 Jun 2007 21:35 GMT Hello all,
I've written my first real C# program. I am posting this here, because I would like your opinion. Is it good, bad, where can I improve etc.? The project is a simple calculator, I've got the basics working and I'll probably extend its functionality with a filelogger and memorybuttons to make it more of a real calculator. I look forward from hearing from you.
Regards,
A.Logghe.
Here's the code:
using System; using System.Collections.Generic; using System.ComponentModel; using System.Data; using System.Drawing; using System.Text; using System.Windows.Forms;
namespace Calculator {
public partial class MyCalculator : Form { private string firstOperand = ""; private string secondOperand = ""; private bool isFirstChar = true; private bool CommaChrUsed = false; private int operandCount = 0; private string operandType = "";
public MyCalculator() { InitializeComponent(); }
private void btnNumeral_Click(object sender, EventArgs e) // buttons (0-9 + comma) { Button b = new Button(); b = (Button)sender;
if (operandCount == 0) { if (isFirstChar) { if (b.Text == "," || b.Text == "0") { firstOperand = "0,"; CommaChrUsed = true; isFirstChar = false; } else { firstOperand += b.Text; txtBoxDisplay.Text = firstOperand; isFirstChar = false; } } else //!isFirstChar { if (b.Text == "," && !CommaChrUsed) { firstOperand += b.Text; txtBoxDisplay.Text = firstOperand; CommaChrUsed = true; }
if (b.Text != ",") { firstOperand += b.Text; txtBoxDisplay.Text = firstOperand; } } } else //operandCount != 0 { if (isFirstChar) { if (b.Text == "," || b.Text == "0") { secondOperand = "0,"; CommaChrUsed = true; isFirstChar = false; } else { secondOperand += b.Text; txtBoxDisplay.Text = secondOperand; isFirstChar = false; } } else //!isFirstChar { if (b.Text == "," && !CommaChrUsed) { secondOperand += b.Text; txtBoxDisplay.Text = secondOperand; CommaChrUsed = true; }
if (b.Text != ",") { secondOperand += b.Text; txtBoxDisplay.Text = secondOperand; } } } }
private void btnOperator_Click(object sender, EventArgs e) // all the standard arithmetic operators + %-operator { Button b = new Button(); b = (Button)sender; operandType = b.Text; if (operandType == "+/-" || operandType == "sqrt" || operandType == "1/x") //plus these unary operators operandCount = 1; else operandCount = 2;
isFirstChar = true; CommaChrUsed = false; }
private void btnClear_Click(object sender, EventArgs e) { txtBoxDisplay.Text = "0,"; firstOperand = ""; secondOperand = ""; isFirstChar = true; CommaChrUsed = false; operandCount = 0; }
private void btnEquals_Click(object sender, EventArgs e) // = operator { if (operandCount > 1) { switch (operandType) { case "+": { double result = double.Parse(firstOperand) + double.Parse(secondOperand); txtBoxDisplay.Text = result.ToString(); firstOperand = txtBoxDisplay.Text; secondOperand = ""; operandCount = 1; isFirstChar = true; CommaChrUsed = false; } break; case "-": { double result = double.Parse(firstOperand) - double.Parse(secondOperand); txtBoxDisplay.Text = result.ToString(); firstOperand = txtBoxDisplay.Text; secondOperand = ""; operandCount = 1; isFirstChar = true; CommaChrUsed = false; } break; case "*": { double result = double.Parse(firstOperand) * double.Parse(secondOperand); txtBoxDisplay.Text = result.ToString(); firstOperand = txtBoxDisplay.Text; secondOperand = ""; operandCount = 1; isFirstChar = true; CommaChrUsed = false; } break; case "/": { double result = double.Parse(firstOperand) / double.Parse(secondOperand); txtBoxDisplay.Text = result.ToString(); firstOperand = txtBoxDisplay.Text; secondOperand = ""; operandCount = 1; isFirstChar = true; CommaChrUsed = false; } break; case "%": { double result = (double.Parse(firstOperand) / 100) * (double.Parse(secondOperand)); txtBoxDisplay.Text = result.ToString(); firstOperand = txtBoxDisplay.Text; secondOperand = ""; operandCount = 1; isFirstChar = true; CommaChrUsed = false; } break; default: { MessageBox.Show("The chosen operator is not a valid operator.", "Unknown operator"); btnClear_Click(sender, e); } break; } } else if (operandCount == 1) { switch (operandType) { case "+/-": { double result = - (double.Parse(firstOperand)); txtBoxDisplay.Text = result.ToString(); firstOperand = txtBoxDisplay.Text; secondOperand = ""; isFirstChar = true; CommaChrUsed = false; } break; case "sqrt": { double result = Math.Sqrt(double.Parse(firstOperand)); txtBoxDisplay.Text = result.ToString(); firstOperand = txtBoxDisplay.Text; secondOperand = ""; isFirstChar = true; CommaChrUsed = false; } break; case "1/x": { double result = 1 / double.Parse(firstOperand); txtBoxDisplay.Text = result.ToString(); firstOperand = txtBoxDisplay.Text; secondOperand = ""; isFirstChar = true; CommaChrUsed = false; } break; default: { MessageBox.Show("The chosen operator is not a valid operator.", "Unknown operator"); btnClear_Click(sender, e); } break; } } } } }
Alun Harford - 12 Jun 2007 02:22 GMT > Hello all, > [quoted text clipped - 4 lines] > memorybuttons to make it more of a real calculator. I look forward > from hearing from you. Well from a practical point of view, as long as it works you're okay! But here are some suggestions that might make programming easier for your next project.
> Here's the code: > [quoted text clipped - 13 lines] > private string firstOperand = ""; > private string secondOperand = ""; You call these variables operands, but then declare them as strings. You should define a class (or more likely an enum) called Operand and then things might well get easier.
> private bool isFirstChar = true; > private bool CommaChrUsed = false; [quoted text clipped - 8 lines] > private void btnNumeral_Click(object sender, EventArgs e) > // buttons (0-9 + comma) Is there really a reason for this comment? Comments in code that isn't hacky for performance reasons are usually a sign that things have gone wrong. Your code should generally explain itself.
> { > Button b = new Button(); > b = (Button)sender; I don't know why you're allocating a Button here. Button b = (Button)sender;
> if (operandCount == 0) > { [quoted text clipped - 61 lines] > } > } This crazy string handling is a sign that you've poorly chosen to use strings to store your data.
> } > } [quoted text clipped - 4 lines] > Button b = new Button(); > b = (Button)sender; Button b = (Button)sender;
> operandType = b.Text; > if (operandType == "+/-" || operandType == "sqrt" || > operandType == "1/x") //plus these unary operators > operandCount = 1; > else operandCount = 2; It looks like you should be using an enum for operandType.
> isFirstChar = true; > CommaChrUsed = false; [quoted text clipped - 85 lines] > break; > } This looks like copy/pasted code. Usually a sign that you need to make a new method. That way you only have one set of code to maintain.
<snip more copy/paste code>
Alun Harford
Kevin Spencer - 12 Jun 2007 12:06 GMT > Is there really a reason for this comment? > Comments in code that isn't hacky for performance reasons are usually a > sign that things have gone wrong. Your code should generally explain > itself. I hate to disagree, but this is patently wrong. Comments are invaluable in terms of both communicating to other developers who may work on your code, and when you have to come back and work on the code months or years later. Comments are not compiled, so there is *no* reason to avoid them. And I don't know about you, but I have written some darned complex code, and over the years, I've learned to comment MORE, not less. I don't know how many times I've kicked myself for NOT commenting when I've had to come back and work on my own code later.
 Signature HTH,
Kevin Spencer Microsoft MVP
Printing Components, Email Components, FTP Client Classes, Enhanced Data Controls, much more. DSI PrintManager, Miradyne Component Libraries: http://www.miradyne.net
>> Hello all, <snip>
> Is there really a reason for this comment? > Comments in code that isn't hacky for performance reasons are usually a [quoted text clipped - 4 lines] >> Button b = new Button(); >> b = (Button)sender; <snip>
> <snip more copy/paste code> > > Alun Harford Bob Powell [MVP] - 14 Jun 2007 18:28 GMT >>Is there really a reason for this comment? Comments in code that isn't hacky for performance reasons are usually a sign that things have gone wrong. Your code should generally explain itself.
You just failed the job interview at my company big-style.
 Signature Bob Powell [MVP] Visual C#, System.Drawing
Ramuseco Limited .NET consulting http://www.ramuseco.com
Find great Windows Forms articles in Windows Forms Tips and Tricks http://www.bobpowell.net/tipstricks.htm
Answer those GDI+ questions with the GDI+ FAQ http://www.bobpowell.net/faqmain.htm
All new articles provide code in C# and VB.NET. Subscribe to the RSS feeds provided and never miss a new article.
>> Hello all, >> [quoted text clipped - 240 lines] > > Alun Harford Alun Harford - 15 Jun 2007 04:03 GMT > >>Is there really a reason for this comment? > Comments in code that isn't hacky for performance reasons are usually a > sign that things have gone wrong. Your code should generally explain > itself. > > You just failed the job interview at my company big-style. There are more programming jobs than there are programmers. I'll live. :-)
However, you're trying to fill one of those jobs. Perhaps you should think more carefully about demanding that potential employees should use your coding style before they join your company - good programmers adjust their programming style to the corporate environment they're working in, and everybody's style changes with time.
However, with that out of the way, I'll now try to convert you to my way of doing things. :-)
5 years ago, I was really into comments - I was mainly using Java at the time, and was a fan of Javadoc.
One day I did an experiment where I spent about an hour writing some code and then I spent another hour refactoring the code to make absolutely sure that anybody could easily understand it, without using any comments.
I discovered, in a very personal and practical way, that well-chosen variable and method names made Javadoc useless. And I discovered that when I made changes to the code, I didn't have to look through to change all the comments relating to that code (comments that are just wrong plague 'well-commented' code).
*** IMPORTANT BIT *** I discovered that the best way to make each line of code understandable is to think about readability when you write the line, instead of adding a comment after you've written the code to try to compensate for the fact that it's unreadable.
With practice, I found that I could write easily understandable code in only 10% more time than rushing and writing code 'normally'. Since I didn't need to write and maintain as much documentation, I saved time, and I found that my code was much easier to understand.
In my current 'personal' project (>100KLOC) I have 5 comments. High-level documentation is stored elsewhere (I use paper for that) and documentation saying what a few lines of code do is redundent if you make sure that those lines express what you're doing in a *clear* way.
Alun Harford
Kevin Spencer - 12 Jun 2007 12:23 GMT Hi Arjen,
There are always ways to optimize code and make it easier to read/modify. As a first C# program, I can not find fault with it. It's a "good start." I can provide a few suggestions.
First, almost any program never remains the same. In later incarnations, features are added, including business logic functionality and interface. It is important to keep them (business and UI) separate for this reason. The business logic is the logic that processes data. The interface is the code that presents the data to the user, takes input from the user, and passes commands to the business "layer" from the user. So, as a first suggestion, I would recommend separating them out.
This means that your project should actually be 2 projects. One is a Form, and the other is a Class Library. Now, this might seem a bit much considering that you are just doing some basic mathematical operations here, but remember that you hope/plan to expand on it later. Some calculators, for example, can track various values in memory. Many can perform undo and redo operations, which also demands memory. Some will convert from one numbering system (radix) to another. Remember that you may plan later to be able to re-skin your UI, or host a calculator in another project. And as you've noticed, while the numbers in the TextBox are strings, in memory they are numbers. Your UI should treat them as strings, and your business layer should treat them as numbers. The business layer should be able to translate strings to numbers and vice versa.
Keeping them separate also makes the whole thing easier to work with. When working on the Form, you're thinking only about what the business layer wants from the user, and how to display it, as well as take input. In the Business layer, you only have to think about data and process.
Microsoft has some great resources about good software design: http://msdn2.microsoft.com/en-us/practices/default.aspx
My Uncle Chutney always sez, "Big things are made up of lots of little things." As Alun pointed out, whenever you see the same block of code repeated, or you have copied and pasted code, you're probably looking at a good candidate for a function, or possibly a class. Breaking your project up into smaller logical pieces will make things much easier for you over the long haul. And breaking the pieces up into smaller pieces is the same idea applied recursively.
Good luck!
 Signature HTH,
Kevin Spencer Microsoft MVP
Printing Components, Email Components, FTP Client Classes, Enhanced Data Controls, much more. DSI PrintManager, Miradyne Component Libraries: http://www.miradyne.net
> Hello all, > [quoted text clipped - 260 lines] > } > } Arjen Logghe - 12 Jun 2007 15:53 GMT > Hi Arjen, > [quoted text clipped - 45 lines] > Kevin Spencer > Microsoft MVP Thanks a lot for taking the time to reply. I replied to Alun but accidentally pressed Reply Author instead of just Reply. Perhaps Alun would be so kind as to post my reply here :).
I'll surely take all the good advice to heart.
Regards,
Arjen.
Alun Harford - 15 Jun 2007 04:16 GMT > Thanks a lot for taking the time to reply. I replied to Alun but > accidentally pressed Reply Author instead of just Reply. Perhaps Alun > would be so kind as to post my reply here :). I don't read devnull@alunharford.co.uk, but it also doesn't actually map to /dev/null - I've found your email :-)
It's pretty late (well, early) here but I'll try to reply to it tomorrow.
Alun Harford
> On 12 jun, 03:22, Alun Harford <devn...@alunharford.co.uk> wrote: > [quoted text clipped - 42 lines] >> Comments in code that isn't hacky for performance reasons are usually a >> sign that things have gone wrong. Your code should generally explain itself.
> I added this comment just for this post, so I didn't have to show the > declaration of all the individual buttons and its fields in the code. [quoted text clipped - 197 lines] > > Arjen. Alun Harford - 17 Jun 2007 00:11 GMT >> Thanks a lot for taking the time to reply. I replied to Alun but >> accidentally pressed Reply Author instead of just Reply. Perhaps Alun [quoted text clipped - 4 lines] > > It's pretty late (well, early) here but I'll try to reply to it tomorrow. (In case anybody is eagerly awaiting my reply, I don't really have a response to the post).
Good luck in your programming, Arjen :-)
Alun Harford
Arjen Logghe - 17 Jun 2007 00:37 GMT > >> Thanks a lot for taking the time to reply. I replied to Alun but > >> accidentally pressed Reply Author instead of just Reply. Perhaps Alun [quoted text clipped - 11 lines] > > Alun Harford I had set up camp at my computer waiting for you to reply :( Nah, I'll add a few more features tomorrow and post the source :). Maybe somebody has any other suggestions or someone may discover some errors in the code.
Regards,
Arjen.
Arjen Logghe - 21 Jun 2007 13:18 GMT As promised, here is the complete source :)
http://filebeam.com/a3a8d84f25cb0e3aea109b1b70134c97
I would appreciate if someone is willing to look at the source and offer some additional suggestions. I sticked with my initial design because changing the whole design to the offered suggestions would be too much work (well at least for me), that's something for me to implement in one of my next projects :). Let me know if you find any bugs :). The only problem I've come across is that if you press Clear and Show Log quickly, the log is being written to while you're attempting to read it, so the program crashes after throwing an IOException. I didn't know a way to force File.Create(logPath) to finish before processing the btnShowLog event, other then multithreading or asynchronous stuff. Which is something that I'm not quite comfortable with ;).
Thanks for your time.
Regards,
Arjen.
Arjen Logghe - 21 Jun 2007 21:05 GMT Fixed the bug :)
private void btnClearLog_Click(object sender, EventArgs e) { FileStream fs = new FileStream(logPath, FileMode.Create); fs.Close(); }
Final source:
http://filebeam.com/2086b7f8ea9c57d0f3ca27c15047459d
~Arjen.
Alun Harford - 22 Jun 2007 00:01 GMT > As promised, here is the complete source :) > [quoted text clipped - 5 lines] > too much work (well at least for me), that's something for me to > implement in one of my next projects :) I really, really think that's something to implement in this project, so you can see how much less painful it is. :-)
Alun Harford
Free MagazinesGet these publications absolutely FREE for up to 12 months. There are no hidden fees and no obligation. Simply choose a title, complete the application form and submit it. Read more ...
|
|
|