2
\$\begingroup\$

What is the best way to considerably decrease the size of the following code without sacrificing functionality, and to make it more readable, and well, make it look professional?

class Compiler
{        
    public string Compile(TextReader src)
    {
        string rtn = SourceTemplate.Top;

        var preprocessor = new PreProcessor();

        Parser[] ParserRounds = {
                                    new PredefinedConstantParser(),
                                    new VariableDeclarationParser(), 
                                    new ConditionalExpressionParser(),
                                    new FunctionCallParser()//,
                                  //new FunctionParser
                                };

        foreach (var l in preprocessor.Process(src))
        {
            var line = l;

            foreach (var parser in ParserRounds)
            {
                line = parser.Parse(line);
            }

            rtn += "\t\t\t" + line + "\n";
        }

        return rtn + SourceTemplate.Bottom;
    }

    public static void Start(string[] args)
    {
        TextReader src = File.OpenText(args[0]);      

        var com = new Compiler();
        var source = com.Compile(src);

        string target = args[0].TrimEnd(".hack".ToCharArray());
        target += ".cs";                     
    }

    public void SaveToFile(string source, string target)
    {
        TextWriter wsrc = File.CreateText(target);
        wsrc.WriteLine(source);
        wsrc.Close();
    }
}

In my personal opinion, the above code looks like a mess at this stage. Also, please suggest good and expressive variable names instead of rtn , src etc. Should I also make this class generic?

\$\endgroup\$
0

3 Answers 3

8
\$\begingroup\$

Just some generic notes since I'm not too familiar with C#:

  1. rtn should be StringBuilder

  2. I'd change the preprocessor and ParserRounds variables to fields and pass their values to the constructor of the class. It makes testing easier (you can test only one parser at a time, you can test with a mocked preprocessor etc.) and it results looser coupling.

  3. Using SourceTemplate.Top and SourceTemplate.Bottom also tight coupling, pass these values to the constructor too, store them in a field and use these fields in the Compile method. (Maybe it's worth creating a SourceTemplate interface with GetTop() and GetBottom() methods and passing a proper instance to the Compiler class.)

  4. I'm really missing the input validation. For example, if the Compile method get a null as a value of TextReader you should throw an exception immediately. It helps debugging a lot. Why would you do any compiling if it will throw an exception later since the TextReader is null?

    The Start method also should check the input values.

  5. Some variable naming ideas:

    • src -> sourceReader or srcReader
    • com -> compiler
    • target -> targetFileName
    • rtn -> result
    • wsrc -> targetWriter
\$\endgroup\$
2
\$\begingroup\$

Building on the great answers provided by @palacsint and @toto2 I have put together a quick sample of what could be done to improve your application.

For me code speaks louder than words so here goes:

// SOLID: Interface Segregation Principle
//        Use an interface to allow better / easier unit testing 
//        through mocking.
    internal interface ICompiler
{
    string Compile(string textFileContents);
}

// SOLID: Interface Segregation Principle
//        Once again using an interface will allow the preprocessor 
//        to be mocked in unit testing.
interface ITextPreProcessor
{
    IEnumerable<string> Process(string lines);
}

// SOLID: Interface Segregation Principle
//        Use an interface to allow better / easier unit testing 
//        through mocking.
internal interface ITextParser
{
    string Parse(string line);
}

// SOLID: Single Responsibility Principle
//        This class is responsible for the parsing of text files.
//        The class is NOT responsible for loading or pesisting the
//        parsed response.
internal class Compiler : ICompiler
{
    public Compiler()
        : this(new PreProcessor())
    { }

    // other text parsers have been omitted from this code sample
    public Compiler(ITextPreProcessor textPreProcessor)
        : this(textPreProcessor, new ITextParser[] { new PredefinedConstantParser(), new VariableDeclarationParser() }
            )
    { }

    // using this constructor allows for better object mocking and unit testing.
    public Compiler(ITextPreProcessor textPreProcessor, IEnumerable<ITextParser> parserRounds)
    {
        // depending on your business logic you may want to check for null value objects 
        // and throw an exception here
        if (textPreProcessor == null) throw new ArgumentNullException("textPreProcessor");
        if (parserRounds == null) throw new ArgumentNullException("parserRounds");

        TextPreProcessor = textPreProcessor;
        ParserRounds = parserRounds;
    }

    /// <summary>
    /// Gets the text pre processor.
    /// </summary>
    public ITextPreProcessor TextPreProcessor { get; private set; }

    /// <summary>
    /// Gets the parser rounds.
    /// </summary>
    public IEnumerable<ITextParser> ParserRounds { get; private set; }

    public string Compile(string textFileContents)
    {
        // depending on your business rules you may want to throw an
        // exception if the textFileContents variable is null???
        if (string.IsNullOrEmpty(textFileContents)) throw new ArgumentNullException("textFileContents");

        var builder = new StringBuilder();

        builder.AppendLine(SourceTemplate.Top);
        builder.AppendLine("\t\t\t");

        foreach (var line in TextPreProcessor.Process(textFileContents))
        {
            foreach (var parser in ParserRounds)
            {
                // depending on your business rules you may want to either use a 
                // try catch exception block here if the returnv value from "Parse"
                // is null or empty by letting the exceptions bubble up
                // to the layer responsible for doing something with the exception???
                builder.AppendLine(parser.Parse(line));
            }

            builder.AppendLine("\n");
        }

        builder.AppendLine(SourceTemplate.Bottom);
        return builder.ToString();
    }
}

// SOLID: This class is responsible for the management
public class ParseManager
{
    public static void Start(string[] args)
    {
        var sourceFileContents = File.ReadAllText(args[0]);

        try
        {
            ICompiler compiler = new Compiler();
            var parsedContents = compiler.Compile(sourceFileContents);

            string destinationPath = string.Format("{0}.cs", args[0].TrimEnd(".hack".ToCharArray()));

            // writing text files can be done in 1 line so there
            // is no need to wrap this in a seperate method
            File.WriteAllText(destinationPath, parsedContents);

            // sample 
            Console.WriteLine("Parsed file saved: {0}", destinationPath);
        }
        catch (Exception exception)
        {
            Console.WriteLine(exception.ToString());
        }

        Console.Read();
    }
}

Below are some sample class implementations to show how this improvement could be used

// sample class implementation
internal class PreProcessor : ITextPreProcessor
{
    public IEnumerable<string> Process(string lines)
    {
        throw new NotImplementedException();
    }
}

// sample parser implementation
internal class PredefinedConstantParser : ITextParser
{
    public string Parse(string line)
    {
        var builder = new StringBuilder();
        return builder.ToString();
    }
}

// sample parser implementation
internal class VariableDeclarationParser : ITextParser
{
    public string Parse(string line)
    {
        var builder = new StringBuilder();
        return builder.ToString();
    }
}

With naming standards I tend to use those offered by IDesign as they take out any personal preference and are in line with best practice suggested by Microsoft.

For details on the SOLID principles refer to here and here. IMHO SOLID is one of the most important aspects in writing good software applications.

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

ParserRounds should not start with a capital since it is an instance.

But much worse is that it should not exist at all: you are straying very far from OO. Instead of the for-loop that calls the members of parserRounds you should think of some OO pattern to use, where the line goes through some "automatic" chain of processing. There are probably many different ways to proceed, but I can think of passing the line to some instance of Parser that itself contains the other subclasses of Parser in order.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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