[skybomb0]

Hope this is the right forum for posting code examples. If not, please redirect me to the correct forum.

I am completely new to C# and OOP programming as a whole. As my first project, I decided to make a calculator. I based the display off of the standard TI 30 calculators, so when you click "log", the word log actually shows up in the equation. Anyways, just wanted a bit of feedback on what everyone thought about my first project. The code is a bit messy; if you have any coding style suggestions, I am open to hear them.

Attached File(s)

•  Calculator.zip (91.59K)
  Number of downloads: 1925

[eker676]

I looked at it out of curiosity and was pleased to see readable code for a change.

Your coding style is fine. The code documented itself for the most part. A comment or two describing each function and you'll be good.

The GUI wasn't spectacular. I would have done the same thing considering it's only a calculator.

One thing that jumped out at me was getting an error message if you didn't close the parenthesis will using one of the functions. (i.e. log(5 = )
I would suggest checking for unclosed parenthesis' and adding them before evaluating the equation.

[Cookiesliyr]

not bad, you can always improve it as well, i took a look on the code and i believe you can make it shorter it will be a good practice for you, for example when i saw this code

switch(ctrl.Text)
{
	case "/":
		switch(textBox.Text.Substring(textBox.Text.Length - 1))
		{
		case "/":
			return;
		case "*":
			return;
		case "-":
		        return;
		case "+":
			return;
		}
	break;
	case "*":
		switch(textBox.Text.Substring(textBox.Text.Length - 1))
		{
		case "/":
			return;
		case "*":
			return;
    	        case "-":
			return;
		case "+":
			return;
		}
		break;
		case "-":
		    switch(textBox.Text.Substring(textBox.Text.Length - 1))
			{
			case "/":
				return;
			case "*":
				return;
			}
		break;
		case "+":
		    switch(textBox.Text.Substring(textBox.Text.Length - 1))
		    {
		    case "/":
				return;
			case "*":
	    		        return;
			case "-":
				return;
			case "+":
				return;
			}
		break;
	}
			
textBox.AppendText(ctrl.Text);

i believe you want to make the program write nothing when the user press one of the operations while there is another operation before it, right ?

for example if the textbox has this
15+

and the user pressed -+*\ the program won't add them so it won't be resulted into
15+-

wonderful, but what will happens if there is nothing in the text ? well ... yes our dear friend Exception will jump on us because the substring can't take negative numbers, it will fire ArgumentOufOfRangeException that's mean your are trying to reach somewhere which is out of the range (the length in our case in here) so you can jump over this problem easily like this

            if (ctrl.Text == "/" || ctrl.Text == "*" || ctrl.Text == "-" || ctrl.Text == "+")
            {
                if (textBox.Text.Length == 0)
                    return;
                switch (textBox.Text.Substring(textBox.Text.Length - 1))
                {
                    case "/": case "+": case "-": case "*":
                        return;
                }

            }
			
	textBox.AppendText(ctrl.Text);

with this code

if (textBox.Text.Length == 0)

it will check if the text is empty or not, this is not the only way but it is one way to do that,
now when the text is empty it will return nothing so no Exception will be fired from the compiler.

yes you can make more then one case after each others if they do the same job like this

case 1: case2: case3:
do something funny
break;
case 4: case 5: case6:
do something else funny
break;

and in your code all the cases will return nothing which will let the program exit from the function so you can put all the cases in one line.

a bitter way to do that instead of doing nothing when an operation exist and the user enter a new one to let the new operation replace the old one and that won't be hard to do as well just a simple trick can do the job

            if (ctrl.Text == "/" || ctrl.Text == "*" || ctrl.Text == "-" || ctrl.Text == "+")
            {
                if (textBox.Text.Length == 0)
                    return;
                switch (textBox.Text.Substring(textBox.Text.Length - 1))
                {
                    case "/": case "+": case "-": case "*":
                        textBox.Text = textBox.Text.Substring(0, textBox.Text.Length - 1);
                        break;
                }

            }
			
			textBox.AppendText(ctrl.Text);

as u can see this code

textBox.Text = textBox.Text.Substring(0, textBox.Text.Length - 1);

will take the whole text except the last char which is the operation itself and assign it to the text box and it will break the switch instead of return nothing so it will add the operation which the user pressed.
so in English it will replace the last operation with the new one.

you can improve allot of things in your code but it doesn't mean it wasn't good not at all, i liked the way which you have organized the code, when you write a code and it have some tricks or allot of ifs statements and switches try to write a comment before it to remind yourself what does it do, if you have a weird function or some variables you can put a comments on them as well Why have you created them.

you can check my code in here and see if it work or not and figure out what does it do, and you can try by yourself to make your code shorter, as my old teacher told me once "a good programmer is a LAZY programmer !" he meant by lazy not by doing nothing of course XD he meant the one who can make the code do the same thing in the shortest way.

if i were you will make a check box which will let the error message not appear because some users hate to see error messages and they know what is the mistake when the calculator won't do anything for them.

when you improve the code i would like to see it so post it here please

best regards,

This post has been edited by Cookiesliyr: 13 May 2010 - 12:36 AM

[skybomb0]

Thanks for the tips guys! I thought there might be a way to use multiple cases on the same line, but never looked for it. I will definately come back with a few fixes later.

By the way, I found this really cool way of creating code in memory and running the code to get the answer to problems. Is there a way to find out what type of exceptions are occurring in the ProcessCommand method? How would I interpret what the error is?

This post has been edited by skybomb0: 13 May 2010 - 01:26 PM

[Cookiesliyr]

Of course there is ! try to find something about Try and catch statements, it is the formal way to deal with exceptions, in java every exception is a class by itself and i believe it is the same case in C# as well, in C# i believe you can use on error conditions just like Visual basic some how but i don't know why programmers don't like them XD you can always check google for programing tips just write the programing language name followed with ":" and what are you looking for like

C#: switch
java: double linked list
fortran : if else statement

i found some tips about error handling in here for you they are not the best i just picked the first one i found so you can find more by yourself, but it is the best if you asks me to prevent the user to make the error by using if-then statements or just ignore it completely just what i have did in your code to solve these kinds of problem

if (textBox.Text.Length == 0)
                    return;

because i know the substring will check the litter before the one i will have enter and the text is empty i will just do nothing in this case so i didn't have to put all my code in one try and catch statements but sometimes you can't do that ... or it is easier to use try and catch instead of guessing and putting allot of ifs to solve each problems at once. it really depends on your style of coding and in the problem itself.

when you get use on programing in C# and you wander "where can i find about more functions ? and learning more tricks ?" well ... you can check out the Object Browser in C# in any project just click on its icon you will see it in the tool bar on the top right side, or you can see the MSDN library it is a very useful place to learn about Microsoft programing languages.

i am looking forward to see your improved code and take it as fun more then learning a new thing, your are the wizard which use magic codes to impress the user

good luck with that and,
best regards

[skybomb0]

I'm sorry, I didn't phrase my question correctly. I was talking about inside the ProcessCommand method and returning the type of exception instead of a general exception with just the error text as the parameter. Kind of hard to understand, but I think I got around it by just recognizing the error text in a try catch statement inside the equals method like you said. I have updated my first post.

In this new version, I have added:
1. Increased error checking
2. Automatically balancing right parenthesis
3. Logging of unknown errors in AppData directory
4. Moved TempModule file to AppData directory
5. Statusbar instead of annoying MessageBoxes
6. Tooltip that tells you the value of "Ans"

Edit: Nevermind, didn't know you could only update your last post. Here is the new code.

Attached File(s)

•  Calculator.zip (97.57K)
  Number of downloads: 286

This post has been edited by skybomb0: 14 May 2010 - 05:52 PM

[Cookiesliyr]

hey, sorry didn't replay one ya soon, i downloaded your calculator and took a look on it but get busy with other stuff when i get a call so suddenly, well i see you have add and modified some stuff but to be hones with you ... are you happy with it ? the most important thing in the program is reliability and you have done a good job with that, then we consider other aspects like readability of the code and efficiency, in larger programs and one which other programs depends on allot cares about efficiency allot but in a simple program like this calculator it won't matter allot but it won't hurt to try our best to write a good useful and easy to read code.

your code need some general functions to solve your problem, for example your delete functions are just too redundant, you can make one delete function with one int variable inside its parameter and replace all these function and just call it with a switch case depending on the function that you want to delete, i can show you an example but it is better to let you try that out by yourself.

take a look on this for example

			if(textBox.Text.Length >= 4)
			{
				switch(textBox.Text.Substring(textBox.Text.Length - 4))
				{
					case "sin(":
						delete = 4;
						break;
					case "cos(":
						delete = 4;
						break;
					case "tan(":
						delete = 4;
						break;
					case "log(":
						delete = 4;
						break;
				}
			}

isn't a similar to operations replacement code ? you can make one delete function to replace all the code region and i believe you can do it, i wish if i can put some more notes on the calculator ... there something bugs me in the code ... the tons of

command = command.Replace();

the tons of them ... i will try to analyze them later on and see what is going on in that block.

best regards,

[skybomb0]

I definately am not finished with this project. Since it is my first project, I think it would be in my best interest to continue to learn about how to make this code better than it would be to make another halfway decent project.

Replacing redundent code would be a good idea. To start out, I was just trying to get everything to work and was coding as my mind was working. Now I can go back and rework the logic to make more sense and maybe be a bit more efficient.

The command.Replace calls are taking the logical input (text from the textbox) and formatting it to work as code logic. From what I understand, the most versatile way I found to solve equations without a ton of string parsing with exceptions is to allow the c# compiler to do all the work. I format the input into code that can be run by the compiler and it will return the answer, following all rules such as order of operations and stuff like that. So the command.Replace calls are just for the compiler.

The replacement patterns as well as the replacement text could probably be put into a 2D array allowing me to easily change the replacement calls as well as loop through the calls in a simple for loop.

I will post back when I have done a little more work on the logic of my code.

This post has been edited by skybomb0: 19 May 2010 - 02:12 PM

[skybomb0]

I have finished a bit of an update. I have added a lot of comments to make the code more self-explanatory. I have cleaned up small parts of the code. I have also added tooltips that explain what each button does.

One problem I am having is sometimes not all the tooltips will display when the corresponding button is hovered over. Is this happening to anyone else or is it local to my computer. I have been having trouble with the tooltips for some reason.

Also, since I can't edit my post or the files I have attached, here is a link to the most recent version.

This post has been edited by skybomb0: 21 May 2010 - 04:15 PM

[Oler1s]

A note on distribution. Either distribute clean source, or clean binaries. In your case, you want to distribute source code. That means you should clean out the bin and obj directories. It doesn't help that VS likes to pollute the source directories by default (and it's rather hard to consistently prevent this). But, do make an effort.

I notice two major points. The first is that you are building a god class. Your one class does everything. It's a design flaw that will become more prominent in larger programs.

The second is that your calculator doesn't parse and evaluate. It translates to code. This is not a real world approach. Unless you explicitly were making a code snippet evaluator, you would actually tokenize and parse mathematical expressions. And evaluate them. You would have to deal with real concerns like creating a robust tokenizer, and manipulation approaches like infix to postfix, and the actual parser and evaluator. That's what a real calculator involves, and you've skipped that.

[skybomb0]

I have fixed the distribution by only adding files in the root directory and the properties directory.

I did recognize that my one class is doing everything, but aren't classes supposed to represent the nouns in programming. The only noun I see is "Calculator". I guess I could move all the code not dealing with the GUI to another class, but that hardly seems useful. But if I did, would I create an instance of this new class in the Program class? What classes do you suggest I create?

My implementation of solving mathematical expressions is definately reliant on the C# compiler, but I think this is the easiest way. It may not be the most robust way, but for a simple calculator, I don't think this is much of a concern. And from what I'v read, implementing a mathematical tokenizer and interpreting it is not a simple task. Once my calculator is operational and I am satisfied, I will move on to learn more in a different project. I do realize that the method I am currently using is not the best, but killing myself over using the correct method will not provide me with any more knowledge apart from improving my string parsing ability.

[Oler1s]

Just as a note, think of everything I say as just points to consider. You can make different arguments, and you should. You should question everything I say and have arguments in favor of your decisions. Just, consider what I'm bringing up.

Quote

but aren't classes supposed to represent the nouns in programming.

Well...I can see why people teach OO concepts by talking about nouns and verbs, but I don't model that way, and I don't think it's a good way to model either. I think classes = nouns is more of a result. It turns out that way, when you model well. You shouldn't approach modeling that way though.

Quote

I guess I could move all the code not dealing with the GUI to another class, but that hardly seems useful.

From a functional standpoint, no. I'm just talking about creating a good design. Should the user interface be tied together with the actual calculation engine? I don't think so. They are decoupled. What if you decided to move from Winforms to WPF? My issue is that the Form class is very tightly coupled to the Winforms API. So I naturally want to keep that separate from the rest of the program. Every instance of that class represents a window right? But should each window have a copy of the calculation engine? Why would anyone want multiple calculator windows? I don't know. But I think it's funky to communicate in your code that the calculator itself is also a part of the interface.

Quote

But if I did, would I create an instance of this new class in the Program class?

That's a good question. Is it meaningful to create instances of this class? How would one instance differ from another? I can make arguments for both. If I couldn't, I would just ask myself: what's the simplest thing to do? If your class is just a bag of related functions, the best thing to do is to make a static class, and thus all the functions are static. There's no meaning to an instance of that class.

Maybe there is. What if you expanded your parser to be able to handle larger expressions, consisting of many lines? You need to be able to store data. Thus, each parser has some internal memory.

Quote

What classes do you suggest I create?

I'm not suggesting a particular design. I just want you to think about the consequence of your design. Let me give you an analogy. Let's say you had an English book to translate into French. What is the best kind of translation? It's a translation where the English book communicates exactly the same thing as the French book. Someone reading either book gets the same ideas in their head. A good translator will accomplish that.

You as a programmer translate from pen and paper to code. You have a a design on paper, and then you represent it in code. As a software developer, you create that design on paper. We can then talk about how good that on paper design is. And then as a programmer, you need code that is read by both the machine and humans. Your program needs to be correct. It should do what it's supposed to do. But someone reading your code should get the same idea as from reading your paper designs.

So I want you to think about that. What does your paper design look like? Is it a "good" design, whatever that means? Will someone who reads you program get the same idea as someone who reads your paper design?

Quote

And from what I'v read, implementing a mathematical tokenizer and interpreting it is not a simple task.

Well it's more complex than what you are doing now But it's a fairly beginner task. For comparison, if you were in your university, you might tackle this in your second semester, or whenever you took your data structures & algorithms course (which would be very early). And mathematical parsing is one of the easier tasks you will come across as a developer

Quote

I do realize that the method I am currently using is not the best, but killing myself over using the correct method will not provide me with any more knowledge apart from improving my string parsing ability.

Well that's fine. I'm just pointing it out. I hope one day you come back to this project and reimplement a proper parser. Just, file that away in the back of your mind.

[skybomb0]

Thank you for the thoughtful explanation. Based on your advice, I will move the calculation code into a separate class, thus, making it more modular. I will see about implementing a tokenizer, but I think my time would be best spent on something other than an expression parser. It wouldn't be too hard, I just meant it would be a bit of a pain to implement due to its complexity.

[niclairedamien]

Dear Skybomb0, I have been following with keen interest the thread of post about your scientific calculator project and indeed, i was very impressed with the work you did.
However, i was unable to download the last edited-version of the program that you placed in a external link.
Please if you still have it, try and re-post it. It has really been a great tool for studying C# programming.
I'm indeed very grateful. Thank you.

[skybomb0]

The link in post #9 doesn't work. I switched hosting providers for my website, so the link broke. I don't have the code that I created that day so I whipped up a bit of code today that is probably representative of what the code at the broken link looked like. This code hasn't been extensively tested either.

As others have said, it would be best to implement a tokenizer to parse the expression rather than regular expressions as I did. Because of the way this code is implemented, expressions are not handled correctly in all situations (e.g. parenthesis and caret), but I did not want to stray too far away from how my original code operated. The code is simple, but by no means perfect.

Attached File(s)

•  Calculator v2.zip (8.06K)
  Number of downloads: 56

This post has been edited by skybomb0: 27 December 2012 - 07:43 AM