3
\$\begingroup\$

I was just looking for some feedback for my c# program to run Euler's method. I was just wondering on any possible improvements, refactoring, inefficiencies... the usual stuff you probably deal with here.

I am using YAMP, a math parser that can evaluate string inputs (so I can run things like calculating the derivative), and is capable of assignment operators (so I can replace x= with its actual value). Here is my code:

class Program
{
    static void Main(string[] args)
    {
        string derivativeinput;
        string[] xyvals;
        int xval;
        int yval;
        int numxval;
        int stepby;
        int stepto;
        derivativeinput = GetDerivative();
        xyvals = GetInitVals();
        Parser.Load();
        Console.WriteLine("Estimate what value of x?");
        stepto = int.Parse(Console.ReadLine());
        Console.WriteLine("Step by=?");
        stepby = int.Parse(Console.ReadLine());
        yval = Int32.Parse(xyvals[1]);
        xval = Int32.Parse(xyvals[0]);
        LoadInitVals(xval, yval);
        for (int i = 1; i <= stepto; i+= stepby)
        {
            int derivative = Int32.Parse(Parser.Parse(derivativeinput).Execute().ToString());

            yval = yval + derivative * stepby;
            Parser.Parse("y =" + yval.ToString()).Execute();

            xval = xval + stepby;
            Parser.Parse("x = " + xval.ToString()).Execute();                          
        }
        Console.WriteLine(yval);
        Console.ReadKey();
    }

    static string GetDerivative()
    {
        Console.WriteLine("Dy/Dx=");
        return Console.ReadLine();
    }


    static string[] GetInitVals()
    {
        Console.WriteLine("Initial value: X,Y");
        var initval = Console.ReadLine();
        return initval.Split(',');
    }


    static void LoadInitVals(int initx, int inity)
    {
        Parser.Parse("x = " + initx.ToString()).Execute();
        Parser.Parse("y =" + inity).Execute();
    }

}

I realize there probably isn't much to evaluate (the parser does a lot of the work). I guess some of the things I'm interested in is handling input, converting (I believe this parser will only evaluate and output a string at the moment), and my methods (for instance, GetDerivative- do I return a string, like I am? Or do I go ahead and set the 'derivative' string to the derivative in the function).

Anything else is welcome as well.

\$\endgroup\$
1
\$\begingroup\$

Your code is essentially fine, but stylistically, I would nitpick a few of your patterns.

Merging the declaration with the assignment of variables

You declare (almost) all of your variables at the start of your method, and then assign them later. In general, it's better to declare the variable as late as possible, as it makes it more clear when the variable is actually relevant. (of course, this is a matter of opinion) Furthermore, it makes the code more concise with no sacrifice in readability, which makes it easier to reason about the code later.

Standard C# naming conventions

For variables in C#, the convention is camcelCasing. Yet all of your variables are strictly lowercase even when they clearly represent multiple words. For example, stepby ought to be stepBy, etc.

Using the built-in keywords when applicable

When using static methods on the int primitive, prefer to use the reserved word. Instead of:

Int32.Parse(xyvals[1]);

Prefer:

int.Parse(xyvals[1]);

If, for the sake of discussion, you prefer Int32, then be consistent and avoid using int to declare your variables. It makes it more clear when you reference the same type consistently in your code.

Use string interpolation if using C# 6

String interpolation is a new feature in C# 6 and in general I would recommend you prefer it to string concatenation (there are performance caveats, but you should focus on that only when that performance becomes an issue over readability). Instead of:

Parser.Parse("y =" + yval.ToString()).Execute();

Use string interpolation:

Parser.Parse($"y = {yval}").Execute();

The code becomes more concise, and (I believe) a little easier to read. Also, the .ToString() is unnecessary as that call will be performed implicitly if the type of the variable is not already a string. (as a side-note, for some reason you format x = ... differently than y =... -- it should probably be consistent.)


I admit a lot of this guidance is somewhat subjective, but I hope you find it helpful. :)

\$\endgroup\$
1
\$\begingroup\$

Input handling

The derivative and the initial values have separate methods to prompt and to take input; the target and step size don't.

None of the methods have error handling if the value given is invalid. You should expect the users to mess things up.

I would create a combined fix for the two along the lines of

T PromptForInput<T>(string prompt, Func<string, T> parse)
{
    Console.WriteLine(prompt);
    while (true)
    {
        try
        {
            return parse(Console.ReadLine());
        }
        catch
        {
            Console.WriteLine("Invalid input");
        }
    }
}

Then the input phase becomes

    string derivativeinput = PromptForInput("Dy/Dx=", s => s);
    int[] xyvals = PromptForInput("Initial value: X,Y", parsePair);
    int stepto = PromptForInput("Estimate what value of x?", int.Parse);
    int stepby = PromptForInput("Step by=?", int.Parse);

    Parser.Load();
    LoadInitVals(xyvals[0], xyvals[1]);

with an auxiliary method parsePair which can check that it gets two values and parse them directly.

(Alternatively, you could use one of the many Point / Point2D structs in the standard library which has a Parse method).

Fractional step count

The lack of comments and input validation means that I'm not sure what the intended behaviour is if I supply the following input:

x
0,0
3
2

Should that be an error because 3 - 0 is not a multiple of 2? If not, what should it do? At present I think it outputs the value extrapolated for x = 2, without any warning that it's not the value for x = 3, and I think that's a bug.

Types

I'm surprised by the heavy use of int. I would have expected double.

L10n

Note that if you switch to using double then you need to be wary when parsing the X,Y input because some cultures use , as the decimal point.

Library

The YAMP changelog lists "Changed static Parser to instance model" two versions ago, so you seem to have an outdated version of the library. For that change alone I would think it's worth updating.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.