Good OOP practice?

[altergothen]

I'm currently doing a course in C# programming fundamentals.
Please will you check and comment on the following assignment:

Assignment: Create a simple calculator prgram that illistrates good OOP
practice.

I have written the following console program that I hope answers this
question (and it does work), unfortunately I couldn't really find a place
to demonstrate the concept of Encapsulation (or maybe I just dont understand
it : )
Anyway here it is:

1. What is your opinion?
2. Are all my //comments correct?

----------------------------------------------------------------------------
----------------------

using System;

class NumericInput
{
// Declare Class variables
private double convertme;
private string input;
private double converted;

// Conversion Method
public double convertMethod(string inputstring)
{
convertme = Convert.ToDouble(inputstring);
return convertme;
}

// Get User Input Method
public double getinput(string prompt)
{
Console.Write(prompt);
input = Console.ReadLine();
converted = convertMethod(input);
return converted;
}
}

class Calculate
{
// Declare Class variables
private double result;

// Sum Method
public double addMethod(double num1, double num2)
{
result = (num1+num2);
return result;
}
}

class Calculator
{
// main Method runs on startup
public static void Main()
{
// Declare Variables
double myresult;
double input1, input2;

// Instantiate Object NumericInput
NumericInput userInput = new NumericInput();

input1 = userInput.getinput("Enter your first number:");
input2 = userInput.getinput("Enter your second number");

// Instantate Object Calculate
Calculate calc = new Calculate();

// Invoke Object Calculate's addMethod
myresult = calc.addMethod(input1,input2);
Console.WriteLine("The answer is: " + myresult);
}
}

 

[pdavis68]

Okay, I'll make some comments on this.

1: where you have "// Declare Class variables", the more accurate
terminology is "fields" not variables.

2: Your convertMethod() function simply converts a string to a double. Why
not change the line:

converted = convertMethod(input);
to
converted = Convert.ToDouble(input);

That's exactly what convertMethod is doing and your simply adding a layer of
method calls, not to mention ambiguity to the code. convertMethod is a poor
name for the method because it doesn't accurately describe what it does.
Your method names should be descriptive of what the function does.
convertStringToDouble() would be a much better name for it, but in this
case, simply removing it and using Convert.ToDouble() is the best idea. I
simply mention the naming because it's important to name your class members
well to make the code readable by you and others.

As a further note on naming, it's a bad idea to use the word "Method" in
your method names unless it's in a sense that descibes a technique like,
MultiplyMatricesWithStraussenMethod where Straussen's Method is the
technique you're using to multiple matrices. Programmers will presumably
know already that your method is a method. Naming it somethingMethod is
redundant.

Finally, I would get rid of the Calculate class altogether. All it does is
adds two numbers. Creating a class to do something so simply is excessive
and in the real world, there's a huge performance penalty involved for
creating classes to do something so simple. The time spent instantiating the
class and initializing members is going to cost your more than actually
performing the addition that it exists to perform.

But, I will make one comment about your implementation of the Calculate
class. You have "result" as a field of the class but it's used as a if it
were a local variable by the addMethod method (see the redundancy in
naming?). It would be much better ot implement addMethod as follows:

public double addMethod(double num1, double num2)
{
double result = (num1+num2);
return result;
}

and not have result as a class field.

I hope I don't sound overly harsh. My intention is simply to provide
constructive criticism.

Pete

 

[B S]

Thanks a Million Pete !!!
Now this is constructive criticism that a newbie like me can really use
and learn from. All your points make alot of sense. Your time and effort
to comment on my code is much appreciated >

- thanks again.

 

[pdavis68]

You're welcome, and after re-reading my post, let me apologize for my
atrocious grammar and spelling. Made me wince to read it.

Pete

 

[Jon Skeet [C# MVP]]

Okay, I'll make some comments on this.

1: where you have "// Declare Class variables", the more accurate
terminology is "fields" not variables.

Not necessarily. The correct terminology (according to the C# spec) is
"instance variables" or "static variables" depending on declaration.
Both are described as fields in the C# spec.

But, I will make one comment about your implementation of the Calculate
class. You have "result" as a field of the class but it's used as a if it
were a local variable by the addMethod method (see the redundancy in
naming?). It would be much better ot implement addMethod as follows:

public double addMethod(double num1, double num2)
{
double result = (num1+num2);
return result;
}

I would go further, and avoid the local variable which serves no real
purpose here:

public double Add(double num1, double num2)
{
return num1+num2;
}

Note the capitalisation of the method name to fit with MS guidelines.

 

[pdavis68]

Jon,

You are correct on both points. Thanks for the corrections.

Pete