My first C# project

Arjen Logghe · Jun 11, 2007

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 · Jun 12, 2007

Arjen said:
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.

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:

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 = "";

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;
private int operandCount = 0;
private string operandType = "";

public MyCalculator()
{
InitializeComponent();
}

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)
{
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;
}
}

This crazy string handling is a sign that you've poorly chosen to use
strings to store your data.

}
}

private void btnOperator_Click(object sender, EventArgs e) //
all the standard arithmetic operators + %-operator
{
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;
}

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;
}

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 · Jun 12, 2007

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.

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

Kevin Spencer · Jun 12, 2007

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!

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

Arjen Logghe · Jun 12, 2007

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!

--
HTH,

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.

Bob Powell [MVP] · Jun 14, 2007

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.

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

Alun Harford · Jun 15, 2007

Bob said:
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

Alun Harford · Jun 15, 2007

Arjen said:
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 (e-mail address removed), 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

Alun Harford · Jun 17, 2007

Alun said:
I don't read (e-mail address removed), 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.

(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 · Jun 17, 2007

(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

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 · Jun 21, 2007

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 · Jun 21, 2007

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 · Jun 22, 2007

Arjen said:
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

I really, really think that's something to implement in this project, so
you can see how much less painful it is. :-)

Alun Harford

My first C# project

Arjen Logghe

Alun Harford

Kevin Spencer

Kevin Spencer

Arjen Logghe

Bob Powell [MVP]

Alun Harford

Alun Harford

Alun Harford

Arjen Logghe

Arjen Logghe

Arjen Logghe

Alun Harford