13
\$\begingroup\$

Note: This ended up being much longer than I was expecting. I have a number of side questions that relate to specific parts of the code for if you don't want to slog through all this mess.

Background

I have had some experience in Java, but recently decided to learn C#. I would like to know if my first program is idiomatic.

I got the idea for this program out of a Java textbook I had lying around. Here's what it says:

Write a program that evaluates expressions typed in by the user. The expressions can use real numbers, variables, arithmetic operators, parentheses, and standard functions (sin, cos, tan, abs, sqrt, and log.) A line of input must contain exactly one such expression. If extra data is found on a line after an expression has been read, it is considered an error. A variable name must consist of letters. Names are case-sensitive. The program should accept commands of two types from the user. For a command of the form print <expression>, the expression is evaluated and the value is output. For a command of the form let <variable> = <expression>, the expression is evaluated and the value is assigned to the variable. If a variable is used in an expression before it has been assigned a value, an error occurs.

I learned about using text mode in C#, so I went a little crazy with it.

Structure

My program is probably over 1000 lines long, so I'm not going to post the whole thing. Here's the basic breakdown of all the classes:

  • BraceMatcher.cs Contains a class that validates that the braces in an expression match.
  • CommandHandler.cs Contains a class that handles the commands sent from the user. A private nested inner exception class called MalformedAssignmentException deals with variable assignment commands that are malformed.
  • ConsoleFormatter.cs Contains a class that formats the Console IO for text mode.
  • IExpression.cs Contains an interface that represents a mathematical expression. The only method it defines is double Evaluate().
  • TermExpression.cs Contains a class that implements IExpression for addition and subtraction operations.
  • FactorExpression.cs Contains a class that implements IExpression for multiplication, division, and modulo operations.
  • FunctionExpression.cs Contains a class that implements IExpression for the standard functions.
  • NumericExpression.cs Contains a class that implements IExpression as a simple wrapper around a real number.
  • ExpressionReader.cs Contains a class that reads expressions and returns a simplified form. The main method exposed is public IExpression Read() which will return a NumericExpression when called from outside the class.
  • HelpCommandInfo.cs Contains a class that formats the information relating to commands given in the help menu. A public nested struct named CommandPair associates a command example with its corresponding explanation.
  • InvalidExpressionException.cs Contains a class that represents an exception thrown when the user enters an invalid expression.
  • TextReaderExtensions.cs Contains a static class that extends TextReader by providing two additional methods: public static string ReadLetters(this TextReader source) which reads only a sequence of letters from the stream and public static void SkipBlanks(this TextReader source) which reads and ignores whitespace.
  • Program.cs - The main program.

Example Code

Because my code is so large, I will be showing bits and pieces of files.

IExpression.cs

namespace SimpleInterpreter
{
    /// <summary>
    /// Represents a mathematical expression.
    /// </summary>
    interface IExpression
    {
        /// <summary>
        /// Returns the value of the expression.
        /// </summary>
        /// <returns>The value of the expression.</returns>
        double Evaluate();
    }
}

TermExpression.cs

namespace SimpleInterpreter
{
    using System;
    using System.Collections.Generic;

    /// <summary>
    /// An expression take operates on two terms.
    /// </summary>
    class TermExpression : IExpression
    {
        private static readonly Dictionary<char, Operator> Operators =
            new Dictionary<char, Operator>()
        {
            { '+', Operator.Addition }, { '-', Operator.Subtraction }
        };

        private enum Operator { Addition, Subtraction }

        readonly IExpression x, y;

        readonly Operator termOperator;

        /// <summary>
        /// Creates a new TermExpression that operates of the specified inner
        /// expressions.
        /// </summary>
        /// <param name="x">The first term.</param>
        /// <param name="operatorSymbol">
        /// The symbol that represents the operator this expression uses.
        /// </param>
        /// <param name="y">The second term.</param>
        /// <exception cref="ArgumentException">
        /// If <paramref name="operatorSymbol"/> refers to an invalid operator.
        /// </exception>
        public TermExpression(IExpression x, char operatorSymbol, IExpression y)
        {
            this.x = x;
            this.y = y;

            try
            {
                termOperator = Operators[operatorSymbol];
            }
            catch (KeyNotFoundException)
            {
                throw new ArgumentException(
                    String.Format("Invalid operator: {1}", operatorSymbol));
            }
        }

        public double Evaluate()
        {
            switch (termOperator)
            {
                case Operator.Addition: return x.Evaluate() + y.Evaluate();
                case Operator.Subtraction: return x.Evaluate() - y.Evaluate();
                default:
                    throw new InvalidOperationException(
                        "Should not be reached.");
            }
        }

        public override string ToString()
        {
            return Convert.ToString(Evaluate());
        }
    }
}

TextReaderExtensions.cs

/// <summary>
    /// Provides a set of static methods for specialized reading.
    /// </summary>
    static class TextReaderExtensions
    {
    /// <summary>
            /// Reads a sequence of letters.
            /// </summary>
            /// <param name="source">A TextReader to read from.</param>
            /// <returns>The letters read.</returns>
            public static string ReadLetters(this TextReader source)
            {
                var wordBuilder = new StringBuilder();
                var i = source.Peek();

                if (i == -1)
                {
                    i = source.Read();
                    var ch = Convert.ToChar(i);
                    if (!Char.IsLetter(ch)) return Convert.ToString(ch);
                    wordBuilder.Append(ch);
                    i = source.Peek();
                }

                while (true)
                {
                    var ch = Convert.ToChar(i);
                    if (!Char.IsLetter(ch)) return wordBuilder.ToString();
                    wordBuilder.Append(ch);
                    source.Read();
                    i = source.Peek();
                }
            }

        // snip
    }

InvalidExpressionException.cs

/// <summary>
    /// An exception that is thrown when the user enters an invalid expression.
    /// </summary>
    [Serializable()]
    class InvalidExpressionException : Exception
    {
        public InvalidExpressionException() : base() { }
        public InvalidExpressionException(string message) : base(message) { }
        public InvalidExpressionException(string message, Exception inner)
            : base(message, inner) { }
        protected InvalidExpressionException(
            SerializationInfo info, StreamingContext context)
            : base(info, context) { }
    }

HelpCommandInfo.cs

/// <summary>
    /// Formats the information relating to commands given in the help menu.
    /// </summary>
    class HelpCommandInfo : IEnumerable<HelpCommandInfo.CommandPair>
    {
        /// <summary>
        /// Associates a command to its description.
        /// </summary>
        public struct CommandPair
        {
            readonly string command;
            readonly List<string> description;

            /// <summary>
            /// Gets the command.
            /// </summary>
            /// <value>The command.</value>
            public string Command { get { return command; } }

            /// <summary>
            /// Gets the formatted description of the command.
            /// </summary>
            /// <value>The formatted description of the command.</value>
            public List<string> Description { get { return description; } }

            /// <summary>
            /// Creates a new CommandPair that associates the specified command
            /// to its specified description.
            /// </summary>
            /// <param name="command">The command.</param>
            /// <param name="description">The description.</param>
            public CommandPair(string command, List<string> description)
            {
                this.command = command;
                this.description = description;
            }
        }

        // snip
    }

ExpressionReader.cs

        double ReadNumber()
        {
            var numberBuilder = new StringBuilder();
            var hasDecimal = false;

            while (true)
            {
                var ch = Convert.ToChar(reader.Peek());
                var isUnaryMinus = ch == '-' && numberBuilder.Length == 0;
                var isDecimalPoint = ch == '.';

                if (isDecimalPoint)
                {
                    if (hasDecimal)
                    {
                        throw new FormatException(
                            "Number cannot have multiple decimal points.");
                    }

                    hasDecimal = true;
                }

                if (!Char.IsDigit(ch) && !isDecimalPoint && !isUnaryMinus)
                {
                    return Convert.ToSingle(numberBuilder.ToString());
                }

                numberBuilder.Append(ch);
                reader.Read();
            }
        }

ConsoleFormatter.cs

/// <summary>
        /// Creates a new ConsoleFormatter that resizes the Console to the
        /// specified width, uses the specified color for the background of
        /// Console output, and initially outputs the specified introduction.
        /// </summary>
        /// <param name="intro">The introduction.</param>
        /// <param name="consoleWidth">
        /// The width to resize the Console to.
        /// </param>
        /// <param name="outputBackground">
        /// The color of the background for Console output.
        /// </param>
        /// <exception cref="ArgumentNullException">
        /// If <paramref name="intro"/> is null.
        /// </exception>
        public ConsoleFormatter(string intro, int consoleWidth=75,
            ConsoleColor outputBackground=ConsoleColor.DarkCyan,
            ConsoleColor foreground=ConsoleColor.White)
        {
            if (intro == null)
            {
                throw new ArgumentNullException("intro");
            }

            this.consoleWidth = consoleWidth;
            ValidateConsoleWidth();

            this.outputBackground = outputBackground;

            Console.ForegroundColor = foreground;
            Console.WindowWidth = consoleWidth;
            Console.CursorVisible = false;

            WriteCentered(intro);
            inputX = Console.CursorLeft;
            inputY = Console.CursorTop;
        }

BraceMatcher.cs

static readonly Dictionary<char, char> Braces =
            new Dictionary<char, char>()
        {
            { '(', ')' }, { '[', ']' }, { '<', '>' }, { '{', '}' }
        };

        /// <summary>
        /// Returns true if the specified character is a left brace.
        /// </summary>
        /// <param name="brace">
        /// The character tested for whether it is a left brace.
        /// </param>
        /// <returns>
        /// <c>true</c> if <paramref name="brace"/> is a left brace.
        /// </returns>
        public static bool IsLeftBrace(char brace)
        {
            return Braces.ContainsKey(brace);
        }

        /// <summary>
        /// Returns true if the specified character is a right brace.
        /// </summary>
        /// <param name="brace">
        /// The character tested for whether it is a right brace.
        /// </param>
        /// <returns>
        /// <c>true</c> if <paramref name="brace"/> is a right brace.
        /// </returns>
        public static bool IsRightBrace(char brace)
        {
            return Braces.ContainsValue(brace);
        }

        readonly TextReader reader;

        readonly Stack<char> braceMatcher;

        public bool IsEmpty { get { return braceMatcher.Count == 0; } }

CommandHandler.cs

readonly ConsoleFormatter formatter;

        readonly ExpressionReader expressionReader;

        readonly Dictionary<string, string> helpCommandInfo;

        readonly Dictionary<string, string> specificHelpCommandInfo;

        readonly Dictionary<string, CommandHandlerFunc> commandHandler;

        int numBadCommands;

        delegate void CommandHandlerFunc();

        /// <summary>
        /// Creates a new CommandHandler that uses the specified
        /// ConsoleFormatter to format its output.
        /// </summary>
        /// <param name="formatter">Formats the output.</param>
        /// <exception cref="ArgumentNullException">
        /// If <paramref name="formatter"/> is null.
        /// </exception>
        public CommandHandler(ConsoleFormatter formatter)
        {
            if (formatter == null)
            {
                throw new ArgumentNullException("formatter");
            }

            this.formatter = formatter;

            expressionReader = new ExpressionReader(Console.In);

            helpCommandInfo = new Dictionary<string, string>()
            {
                { "let <name> = <expression>",
                    "Assigns the value of the expression " +
                    "into the variable name" },
                { "print <expression>",
                    "Outputs the value of the expression" },
                { "help <command>",
                    "Prints a help message that explains " +
                    "how to use the command" },
                { "quit", "Ends the program" }
            };

            specificHelpCommandInfo = new Dictionary<string, string>()
            {
                { "let",
                    "Format:\tlet <name> = <expression>\n" +
                    "The 'let' command assigns an expression to\n" +
                    "a variable. Valid names must have only letters.\n" +
                    "Names are case-sensitive letters and must not be one\n" +
                    "of the standard functions (sin, cos, tan, abs, sqrt,\n" +
                    "log) or a command for this program (let, print, help,\n" +
                    "quit). You can store mathematical expressions in the\n" +
                    "variable you create. You can combine real numbers,\n" +
                    "arithmetic operators, parenthetical expressions, the\n" +
                    "built-in functions, and even other variables within\n" +
                    "your expression. You can redefined your variables as\n" +
                    "many times as you want. The mathematical constants\n" +
                    "'e' and 'pi' have already been defined for you." },

                { "print",
                    "Format:\tprint <expression>\n" +
                    "The 'print' command outputs the value of the\n" +
                    "expression you enter. You may use combinations of\n" +
                    "real numbers, arithmetic operators, parenthetical\n" +
                    "expressions, standard functions (sin, cos, tan, abs,\n" +
                    "sqrt, log), and variables you have already defined\n" +
                    "using the 'let' command." }
            };

            commandHandler = new Dictionary<string, CommandHandlerFunc>()
            {
                { "let", ReadVariable },
                { "print", WriteExpression },
                { "help", WriteHelp },
                { "quit", Quit }
            };
        }

        void ReadVariable()
        {
            try
            {
                numBadCommands = 0;
                var name = ReadVariableName();
                ReadAssignmentOperator();
                var value = expressionReader.Read().Evaluate();

                expressionReader.StoreVariable(name, value);

                formatter.WriteOutput(
                    String.Format("{0} set to {1}", name, value));
            }
            catch (Exception ex)
            {
                if (Console.In.Peek() != -1) Console.ReadLine();

                if (!(ex is MalformedAssignmentException) 
                    && !(ex is InvalidExpressionException))
                { throw; }

                formatter.WriteOutput(ex.Message);
            }
        }

Program.cs

/// <summary>
    /// The main program for the Simple Interpreter.
    /// </summary>
    class Program
    {
        static void Main(string[] args)
        {
            var formatter = new ConsoleFormatter(
                intro: "\nWelcome to my Simple Interpreter!\n" +
                "Please enter a command (or enter \"help\" for help).\n");

            var commandHandler = new CommandHandler(formatter);
            string command;

            do
            {
                formatter.ColorInputBackground(ConsoleColor.DarkBlue);
                Console.In.SkipBlanks();
                command = Console.In.ReadLetters().ToLower();
            }
            while (commandHandler.Execute(command));

            formatter.MakeOutputInvisible();
            Console.ReadLine();
        }
    }

Questions

My main question is

Where does my code stray from idiomatic C#?

Additional Questions:

  1. In ExpressionReader.cs, I wrote a method double ReadNumber() which manually checks for numeric input. Usually I frown at this, but I couldn't find a method where I could just get the next numbers in the stream. Is there one that I'm missing?
  2. I used default parameters in the constructor for the ConsoleFormatter.cs and call it in Program.cs. It seems to me like a cleaner alternative to the Builder Pattern. Am I on the right track here?
  3. In BraceMatcher.cs, I made a property IsEmpty that doesn't have an analog in a member variable. I haven't seen this anywhere else. Is that bad practice?
  4. In CommandHandler.cs, I use a delegate void CommandHandlerFunc() purely for the purpose of putting functions in a Dictionary. I saw delegates used in conjunction with events elsewhere, but I didn't understand it very well. Is it common to use delegates by themselves without events?
  5. Java 7 has a very handy multi-catch exception feature. I was trying a workaround for it in the void ReadVariable() method in CommandHandler.cs. Is there a better way to do that?
\$\endgroup\$
  • 1
    \$\begingroup\$ You should look into automatic properties. You can write things like public string Command { get; private set; } and not need backing fields. \$\endgroup\$ – Bobson Mar 15 '13 at 15:16
  • \$\begingroup\$ @Bobson I don't like setters. I also saw this, \$\endgroup\$ – Eva Mar 16 '13 at 2:01
  • 1
    \$\begingroup\$ I never understood why, but Mark only talks about fully public properties in that article. Properties with private setters aren't as bad - but you can't make them readonly. On a different note, the mostly missing access modifiers are mildly irritating to me; you don't see that much in C#. That can also make it trickier for you, since I seem to remember that Java's defaults are different from those in C#. \$\endgroup\$ – TeaDrivenDev Mar 16 '13 at 2:58
  • 1
    \$\begingroup\$ @Eva - I disagree with that article, but to each his own. \$\endgroup\$ – Bobson Mar 18 '13 at 13:21
  • 2
    \$\begingroup\$ @Eva I read that article as being about encapsulation. The difference is between {get;set;} and {get;private set;}. I don't think the author was saying auto-property setters are bad but that setting fields (from a property in this case) from outside is not good. The suggestion made by @Bobson will generate a backing field for you automagically. The only thing you really lose (assuming JIT in-lining) is readonly which is a shame but you will move on with your life, I promise. Also if you want you can apply Contract.Invariant to a property using Microsoft Code Contracts. \$\endgroup\$ – Dandy Mar 22 '13 at 21:48
13
\$\begingroup\$

First off, for somebody who is just learning C# you use it better than some of the people I work with.

Now to answer your questions:

  1. You are correct, there is no native way to read numbers from the Console in C#. You could look at decimal.TryParse which tries parsing the input.

    The code would look something like this:

    var input = Console.ReadLine();
    
    var inputAsNumber = 0d;
    if (!decimal.TryParse(input, out inputAsNumber)
    {
        // throw favorite exception
    }
    
  2. There is nothing wrong with default parameters. I like them better than having multiple constructors. I too think it's much cleaner.

  3. There is nothing wrong with an IsEmpty method. C# has one for strings string.IsNullOrEmpty(string)

  4. Using delegates this way is perfectly acceptable. I would look into Action and Action<paramTypes[]> though. I think they are a little more common.

  5. Your exception handling could be improved. You can catch multiple exceptions from one call:

    try
    {
        numBadCommands = 0;
        var name = ReadVariableName();
        ReadAssignmentOperator();
    
        var value = expressionReader.Read().Evaluate();
    
        expressionReader.StoreVariable(name, value);
    
        formatter.WriteOutput(
           String.Format("{0} set to {1}", name, value));
    }
    catch (MalformedAssignmentException ex)
    {
        throw;
    }
    catch (InvalidExpressionException ex)
    {
        throw;
    }
    catch (Exception ex)
    {
        if (Console.In.Peek() != -1) 
        {
            Console.ReadLine();
        }
             formatter.WriteOutput(ex.Message);
    }
    
\$\endgroup\$
  • 6
    \$\begingroup\$ As for default parameters, be careful with those, especially if you expose those calls as some kind of public API - default parameter values will be compiled into the calling assembly. That means if you have a library that uses default parameters and later change those values, it will not be enough to exchange the DLL file - all calling assemblies will have to be recompiled against the new version to pick up the updated default values. This is something to keep in mind when using default parameters. (As a matter of fact, I never use them at all, not least because of this.) \$\endgroup\$ – TeaDrivenDev Mar 15 '13 at 15:18
  • 3
    \$\begingroup\$ This: "for somebody who is just learning C# you use it better than some of the people I work with". I was very impressed how good your code was and the fact you were using languages features not in Java, like extensions methods and delegates. \$\endgroup\$ – Daniel Imms Mar 16 '13 at 3:31
  • \$\begingroup\$ Thanks! Action is quite handy. I have been putting it in other places where I have delegates in Dictionaries. \$\endgroup\$ – Eva Mar 16 '13 at 17:21
  • 2
    \$\begingroup\$ var inputAsNumber = 0d; I think decimal inputAsNumber; is better, because it ensures you won't use the default value. \$\endgroup\$ – svick Mar 17 '13 at 15:55
  • \$\begingroup\$ > First off, for somebody who is just learning C# you use it better than some of the people I work with. @Eva For sure it looks good! \$\endgroup\$ – Dandy Mar 22 '13 at 21:49
4
\$\begingroup\$

Have you considered subclassing TermExpression for the various operations? This way you don't have to worry, in Evaluate, if the state of the object is incorrect (which can't be, but code changes...).

Building on the above: instead of using a Dictionary<char, Operator> and catch KeyNotFoundExceptions you could use a switch to build the correct subclass of TermExpression. This way you only switch once, while building, and then every subclass can just do math without having to check who it is everytime.

Another thing I'm not completely convinced with is your use of Dictionary everywhere. They're usually useful for bigger collections that two or three elements.

Oh, and remember that ContainsValue is O(n). Although n is fixed in your code, bear that in mind.

As Jeff said, the code is clean and idiomatic.

\$\endgroup\$
  • \$\begingroup\$ I actually use dictionaries quite often for small "decision tables" as well - it's easily understood and easily extended. \$\endgroup\$ – TeaDrivenDev Mar 16 '13 at 14:39
  • \$\begingroup\$ You're right. Dictionary has been my Swiss army knife whenever I have key-value pairs or just a list of pairs I want to keep together. What should I use instead of Dictionary? \$\endgroup\$ – Eva Mar 16 '13 at 17:20
  • \$\begingroup\$ Dictionaries are usually good, but consider using a Good Old Switch, or polymorphism. This isn't a rule in either direction though, it depends on readability/speed requirements \$\endgroup\$ – miniBill Mar 18 '13 at 21:43

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.