5
\$\begingroup\$

The task is to:

  1. parser text, break it into sentences
  2. break sentences into words
  3. output words in XML or CSV sorted
  4. allow some whitespace around words and delimiters

This is a task I was given for a job interview. I'm rather concerned with maintainbility, readablity, general advice rather then performance. But any suggestions are welcome (maybe I've made some serious and abvious performance mistakes).

Beside implementing the parser and writer I had to expose it in a ASP.NET MVC Web application (using Web Api). The wole code is available here: https://github.com/inwenis/NorParser

Input:

Mary had a little lamb. Aesop and. 

Expected XML format:

<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<text>
    <sentence>
        <word>a</word>
        <word>had</word>
        <word>lamb</word>
        <word>little</word>
        <word>Mary</word>
    </sentence>
    <sentence>
        <word>Aesop</word>
        <word>and</word>
    </sentence>
</text>

Expected CSV format

, Word 1, Word 2, Word 3, Word 4, Word 5, Word 6, Word 7, Word 8
Sentence 1, a, had, lamb, little, Mary
Sentence 2, Aesop, and

Sentence.cs:

using System.Collections.Generic;

namespace NorParser
{
    public class Sentence
    {
        public List<string> Words { get; set; }
    }
}

Parser.cs:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.RegularExpressions;

namespace NorParser
{
    public class Parser
    {
        private readonly char[] _sentenceSeparators = { '.' };

        public List<Sentence> Parse(string input)
        {
            var parsedSentences = new List<Sentence>();
            var sentences = input.Split(_sentenceSeparators, StringSplitOptions.RemoveEmptyEntries);
            foreach (var sentence in sentences)
            {
                var words = ReplaceCharactersNotAllowedInWordsWithSpaces(sentence)
                    .Split(new []{' '}, StringSplitOptions.RemoveEmptyEntries)
                    .Select(RemoveLeadingHyphen)
                    .Select(RemoveTrailingHyphen)
                    .Select(RemoveLeadingApostrophe)
                    .Where(w => !string.IsNullOrWhiteSpace(w))
                    .Where(w => !w.All(char.IsPunctuation))
                    .OrderBy(s => s)
                    .ToList();
                if(words.Any())
                {
                    parsedSentences.Add(new Sentence {Words = words});
                }
            }
            return parsedSentences;
        }

        private string RemoveLeadingApostrophe(string s)
        {
            return Regex.Replace(s, "^'+", "");
        }

        private string RemoveLeadingHyphen(string o)
        {
            return Regex.Replace(o, "^-+|-+$", "");
        }

        private string RemoveTrailingHyphen(string o)
        {
            return Regex.Replace(o, "-+$", "");
        }

        private string ReplaceCharactersNotAllowedInWordsWithSpaces(string input)
        {
            return Regex.Replace(input, "[^a-zA-Z'-]+", " ");
        }
    }
}

XmlWriter.cs

using System.Collections.Generic;
using System.Linq;
using System.Xml.Linq;

namespace NorParser
{
    public class XmlWriter
    {
        public XDocument Write(IReadOnlyCollection<Sentence> input)
        {
            var sentenceElements = input.Select(sentence =>
            {
                var wordElements = sentence.Words.Select(word => new XElement("word", word));
                return new XElement("sentence", wordElements);
            });
            var rootElement = new XElement("text", sentenceElements);
            var xmlDeclaration = new XDeclaration("1.0", "UTF-8", "yes");
            return new XDocument(xmlDeclaration, rootElement);
        }
    }
}

CsvWriter.cs

using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace NorParser
{
    public class CsvWriter
    {
        public string Write(IReadOnlyCollection<Sentence> sentences)
        {
            var stringBuilder = new StringBuilder();

            var maxWordsCount = sentences.Max(s => s.Words.Count);
            var columnHeaders = Enumerable.Range(1, maxWordsCount).Select(i => $", Word {i}");
            stringBuilder.AppendLine(string.Join("", columnHeaders));

            var index = 1;
            foreach (var sentence in sentences)
            {
                stringBuilder.Append($"Sentence {index}, ");
                stringBuilder.AppendLine(string.Join(", ", sentence.Words));
                index++;
            }

            return stringBuilder.ToString();
        }
    }
}

Usage:

using System.Xml.Linq;

namespace NorParser
{
    public static class XDocumentExtensions
    {
        public static string ToStringWithDeclaration(this XDocument xDoc)
        {
            return $"{xDoc.Declaration}\n{xDoc}";
        }
    }
}

...

public string ToXml(string text)
{
    var parser = new Parser();
    var sentences = parser.Parse(text);
    var xmlWriter = new XmlWriter();
    var xDocument = xmlWriter.Write(sentences);
    return xDocument.ToStringWithDeclaration();
}

public string ToCsv(string text)
{
    var parser = new Parser();
    var sentences = parser.Parse(text);
    var csvWriter = new CsvWriter();
    var csv = csvWriter.Write(sentences);
    return csv;
}

Parser tests:

using System.Collections.Generic;
using System.Linq;
using Machine.Specifications;
using NorParser;

[Subject(typeof(Parser))]
class Parsing_a_simple_sentence
{
    Establish context = () =>
    {
        sut = new Parser();
    };

    Because of = () =>
    {
        output = sut.Parse("Mary had a little lamb.");
    };

    It returns_one_sentence = () =>
    {
        output.Count.ShouldEqual(1);
    };

    It returns_all_words = () =>
    {
        output.First().Words.ShouldContainOnly("Mary", "had", "a", "little", "lamb");
    };

    It returns_words_in_alphabetical_order = () =>
    {
        output.First().Words.ShouldEqual(new List<string> {"a", "had", "lamb", "little", "Mary"});
    };

    It does_not_return_empty_sentences = () =>
    {
        output.ShouldNotContain(s => s.Words== null || s.Words.Count == 0);
    };

    static Parser sut;
    static List<Sentence> output;
}

[Subject(typeof(Parser))]
class Parsing_sentence_with_multiple_spaces
{
    Establish context = () =>
    {
        sut = new Parser();
    };

    Because of = () =>
    {
        output = sut.Parse("  had \t ");
    };

    It returns_only_valid_nonempty_words = () => {
        output.First().Words.ShouldContainOnly("had");
    };

    static Parser sut;
    static List<Sentence> output;
}

[Subject(typeof(Parser))]
class Parsing_sentence_with_nonalphabetic_characters
{
    Establish context = () =>
    {
        sut = new Parser();
    };

    Because of = () =>
    {
        output = sut.Parse("  i ~ had!a @mad# $ % ^ & * ( ) _ + - = ` { } |  : \" < > ? [ ] \\ ; , . / * + , ");
    };

    It treats_non_alphabetic_characters_as_spaces = () =>
    {
        output.First().Words.ShouldContainOnly("i", "had", "a", "mad");
    };

    static Parser sut;
    static List<Sentence> output;
}

[Subject(typeof(Parser))]
class Parsing_multiple_sentences_separated_by_dots
{
    Establish context = () =>
    {
        sut = new Parser();
    };

    Because of = () =>
    {
        output = sut.Parse("i had a mad idea. The idea was great. Why didn't we use it");
    };

    It returnes_all_sentences = () =>
    {
        output.Count.ShouldEqual(3);
    };

    static Parser sut;
    static List<Sentence> output;
}

[Subject(typeof(Parser))]
class Parsing_words_with_special_nonalphabetic_characters
{
    Establish context = () =>
    {
        sut = new Parser();
    };

    Because of = () =>
    {
        output = sut.Parse("don't Graham-Cumming lists'");
    };

    It treats_special_nonalphabetic_characters_as_part_of_words = () =>
    {
        output.First().Words.ShouldContain("don't", "Graham-Cumming", "lists'");
    };

    static Parser sut;
    static List<Sentence> output;
}

[Subject(typeof(Parser))]
class Parsing_sentences_with_special_characters_not_being_part_of_words
{
    Establish context = () =>
    {
        sut = new Parser();
    };

    Because of = () =>
    {
        output = sut.Parse("aaa ' bbb - ccc '&' *-= -xxx 'yyy zzz-");
    };

    It return_only_valid_words = () =>
    {
        output.First().Words.ShouldContainOnly("aaa", "bbb", "ccc", "xxx", "yyy", "zzz");
    };

    static Parser sut;
    static List<Sentence> output;
}

[Subject(typeof(Parser))]
class Parsing_sentence_with_only_nonalphabetical_characters
{
    Establish context = () =>
    {
        sut = new Parser();
    };

    Because of = () =>
    {
        output = sut.Parse("this is a sentence. ';[] ;'[] - 0 909 . the previous sentence was not a real sentece");
    };

    It does_not_return_empty_sentence = () =>
    {
        output.ShouldEachConformTo(s => s.Words.Any());
    };

    static Parser sut;
    static List<Sentence> output;
}

XmlWriter Tests:

using System.Collections.Generic;
using System.Linq;
using System.Xml.Linq;
using System.Xml.XPath;
using Machine.Specifications;
using NorParser;

[Subject(typeof(XmlWriter))]
class Serializing_simple_sentence
{
    Establish context = () =>
    {
        input = new List<Sentence>
        {
            new Sentence {Words = new List<string> {"a", "bb", "ccc"}}
        };
        sut = new XmlWriter();
    };

    Because of = () =>
    {
        result = sut.Write(input);
    };

    It returns_all_words_from_input = () =>
    {
        result
            .XPathSelectElements("/text/sentence/word")
            .Select(n => n.Value)
            .ShouldContain("a", "bb", "ccc");
    };

    It returnes_xml_with_proper_declaration = () =>
    {
        result.Declaration.Encoding.ShouldEqual("UTF-8");
        result.Declaration.Version.ShouldEqual("1.0");
        result.Declaration.Standalone.ShouldEqual("yes");
    };

    static XmlWriter sut;
    static List<Sentence> input;
    static XDocument result;
}

[Subject(typeof(XmlWriter))]
class Serializing_multiple_sentences
{
    Establish context = () =>
    {
        input = new List<Sentence>
        {
            new Sentence {Words = new List<string> {"a", "bb", "ccc"}},
            new Sentence {Words = new List<string> {"d", "ee", "fff"}}
        };
        sut = new XmlWriter();
    };

    Because of = () =>
    {
        result = sut.Write(input);
    };

    It returnes_xml_with_element_for_each_sentence = () =>
    {
        result.XPathSelectElements("/text/sentence").Count().ShouldEqual(2);
    };

    static XmlWriter sut;
    static List<Sentence> input;
    static XDocument result;
}

CsvWriter tests:

using System.Collections.Generic;
using Machine.Specifications;
using NorParser;

[Subject(typeof(CsvWriter))]
class Serializing_simple_sentence_to_csv
{
    Establish context = () =>
    {
        input = new List<Sentence>
        {
            new Sentence {Words = new List<string> {"a", "bb", "ccc"}}
        };
        sut = new CsvWriter();
    };

    Because of = () =>
    {
        result = sut.Write(input);
    };

    It returns_all_words_from_input = () =>
    {
        result.ShouldContain("a");
        result.ShouldContain("bb");
        result.ShouldContain("ccc");
    };

    It returnes_csv_with_proper_column_headers = () =>
    {
        result.ShouldStartWith(", Word 1, Word 2, Word 3");
    };

    It returnes_csv_with_row_header_for_each_sentence = () =>
    {
        result.Split('\n')[1].ShouldStartWith("Sentence 1, ");
    };

    static CsvWriter sut;
    static List<Sentence> input;
    static string result;
}
using System.Collections.Generic;
using Machine.Specifications;
using NorParser;

[Subject(typeof(CsvWriter))]
class Serializing_simple_sentence_to_csv
{
    Establish context = () =>
    {
        input = new List<Sentence>
        {
            new Sentence {Words = new List<string> {"a", "bb", "ccc"}}
        };
        sut = new CsvWriter();
    };

    Because of = () =>
    {
        result = sut.Write(input);
    };

    It returns_all_words_from_input = () =>
    {
        result.ShouldContain("a");
        result.ShouldContain("bb");
        result.ShouldContain("ccc");
    };

    It returnes_csv_with_proper_column_headers = () =>
    {
        result.ShouldStartWith(", Word 1, Word 2, Word 3");
    };

    It returnes_csv_with_row_header_for_each_sentence = () =>
    {
        result.Split('\n')[1].ShouldStartWith("Sentence 1, ");
    };

    static CsvWriter sut;
    static List<Sentence> input;
    static string result;
}
\$\endgroup\$
3
  • \$\begingroup\$ Pretty minor but have a ctor for Sentence for Words \$\endgroup\$
    – paparazzo
    Commented Mar 15, 2017 at 17:02
  • \$\begingroup\$ Thanks @Paparazzi for you comment, could you explain why I should add a ctor for to the Sentence class? Btw. I've added the Sentence class to the question. \$\endgroup\$
    – inwenis
    Commented Mar 15, 2017 at 17:06
  • 1
    \$\begingroup\$ I just think it looks cleaner to pass Words in a ctor. Your option. \$\endgroup\$
    – paparazzo
    Commented Mar 15, 2017 at 17:08

2 Answers 2

2
\$\begingroup\$
private readonly char[] _sentenceSeparators = { '.' };

What happened to the ? and !? Those are sentence separators too.


    private string RemoveLeadingHyphen(string o)
    {
        return Regex.Replace(o, "^-+|-+$", "");
    }

This is supposed to remove only leading hypens but it removes trailing ones too.

I would use only one method like TrimSpecialCharacters and use only one regex:

^[-']+|[-']+$

As a matter of fact you can make the entire Parse method a single LINQ expression:

public IEnumerable<Sentence> Parse(string input)
{
    return
        (input ?? throw new ArgumentNullException(nameof(input)))
        .Split(_sentenceSeparators, StringSplitOptions.RemoveEmptyEntries)
        .Select(sentence => new Sentence
        {
            Words =
                 ReplaceCharactersNotAllowedInWordsWithSpaces(sentence)
                .Split(new[] { ' ' }, StringSplitOptions.RemoveEmptyEntries)
                .Select(RemoveLeadingHyphen)
                .Select(RemoveTrailingHyphen)
                .Select(RemoveLeadingApostrophe)
                .Where(w => !string.IsNullOrWhiteSpace(w))
                .Where(w => !w.All(char.IsPunctuation))
                .OrderBy(s => s)
                .ToList()
        }).Where(sentence => sentence.Words.Any());                   
}

The CsvWriter is not a real writer yet, it cannot write to files. I'd call it CsvGenerator or CsvCreator because this is what it does.


You nowhere handle the , but you use it for csv generation. If any sentence contains a , you won't be able to read it later. The cleaning methods does not trim it.

I suggest adjusting the regex for this case:

^[-']+|[-',]+$

or adding it to the split list in case someone did not put a space after it.


As a final word: I like your code because you separated all responsibilities and you can test it.

\$\endgroup\$
5
  • \$\begingroup\$ Thanks very much for your review @t3chb0t , it's very useful! Reg. ? and ! as sentence seprators: I've completly forgotten about them, thanks! Reg. RemoveLeadingHyphen - I've forgotten to remove the -+$ part from the regex, thanks! Reg. using just one regex: I don't want to trim trailing ' - ex. don't Graham-Cumming lists' I would like the result to inculde lists' \$\endgroup\$
    – inwenis
    Commented Mar 15, 2017 at 17:48
  • \$\begingroup\$ Reg. naming of CsvWriter - I wanted to keep the name of the CsvWriter and XmlWriter follow the same pattern. Whould you rather rename both to Csc/Xml Creator or leave them as is? Reg. handling of , - they re replaced with spaces in ReplaceCharactersNotAllowedInWordsWithSpaces - I think this should work fine. \$\endgroup\$
    – inwenis
    Commented Mar 15, 2017 at 17:49
  • 1
    \$\begingroup\$ @inwenis oh, crap, you're right, you're handling the , so it's fine ;-) Somehow I missed the ^ in the last pattern but still, you can tell the regex engine to ignore the case with RegexOptions.IgnoreCase so the final pattern can be shorter [^a-z'-]+. \$\endgroup\$
    – t3chb0t
    Commented Mar 15, 2017 at 17:53
  • \$\begingroup\$ @inwenis you can have two classes, a CsvCreator and a CsvWriter, if it makes sense to separate those two then why not ;-] \$\endgroup\$
    – t3chb0t
    Commented Mar 15, 2017 at 17:55
  • \$\begingroup\$ I don't need a class writing the csv to a file since it's only sent to the client (browerser ) as text and displayed in the website. I wanted to avoid having a XmlWriter class converting the List<Sentence> to a xml and a CsvCreator converting the List<Sentence> to csv \$\endgroup\$
    – inwenis
    Commented Mar 15, 2017 at 18:02
2
\$\begingroup\$

In Parser.cs, some tips:

a.- why are you using s, o and then input as a parameters? I would recommend use just one.

b.- Instead of using hardcoded expression you can use a const text (and follow some convention for the names).

Ie;

namespace NorParser
{
    public class Parser
    {
        private const char[] SENTENCE_SEPARATORS = { '.' };
        private const string LEADING_APOSTROPHE_REGEX = "^'+";
        private const string LEADING_HYPHEN_REGEX = "^-+|-+$";
        private const string TRAILING_HYPHEN_REGEX = "-+$";
        private const string CHARS_NOT_ALLOWED_REGEX = "[^a-zA-Z'-]+";
        ...
        ...

        private string RemoveLeadingApostrophe(string input)
        {
            return Regex.Replace(input, LEADING_APOSTROPHE_REGEX, "");
        }

        private string RemoveLeadingHyphen(string input)
        {
            return Regex.Replace(input, LEADING_HYPHEN_REGEX, "");
        }

        private string RemoveTrailingHyphen(string input)
        {
            return Regex.Replace(input, TRAILING_HYPHEN_REGEX, "");
        }

        private string ReplaceCharactersNotAllowedInWordsWithSpaces(string input)
        {
            return Regex.Replace(input, CHARS_NOT_ALLOWED_REGEX, " ");
        }

        ...

And the same for other files. Try to put all the the hardcoded text as const variables.

\$\endgroup\$
4
  • \$\begingroup\$ Thanks @mayo for the review. Reg. parameter naming: thanks, I've renamed the parameters to s in all private methods Reg. using consts for regexes: They're not reused and extracting them to consts repeats the names of functions, ie. RemoveLeadingHyphen and LEADING_HYPHEN_REGEX. If a const string is reused in a class I would put it in a cost, or if naming it would explain it's purpose I would extract it too. But in this case I rather leave them inline. \$\endgroup\$
    – inwenis
    Commented Mar 15, 2017 at 17:57
  • \$\begingroup\$ Good idea with the constants. \$\endgroup\$
    – t3chb0t
    Commented Mar 15, 2017 at 17:57
  • 1
    \$\begingroup\$ @inwenis it would even make sense to create a TextCleaner and put all those expressions there. \$\endgroup\$
    – t3chb0t
    Commented Mar 15, 2017 at 17:58
  • \$\begingroup\$ @inwenis 'repeats the names of functions' yes, you are right. But I think that improves the maintainability and readability. Also create a TextCleaner it would be a good idea! One class for one purpose! \$\endgroup\$
    – mayo
    Commented Mar 15, 2017 at 18:03

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.