8
\$\begingroup\$

This is a programming challenge I submitted as part of a job interview, which I failed because it lacked "maintainability" and "patterns and best industry practices", so I guess we could all learn something from my mistakes.

The challenge was to write a program that, given a list of programming languages, it'd return a list of result counts in various search engines, a winner on each engine, and a total winner:

C:\> searchfight.exe .net java
.net: Google: 4450000000 MSN Search: 12354420
java: Google: 966000000 MSN Search: 94381485
Google winner: .net
MSN Search winner: java
Total winner: .net

I was not allowed to use any external libraries, so no HtmlAgilityPack or stuff like that. I will only post what I consider the most relevant sections of the code to keep it short, but I also just uploaded the whole project to GitHub.

So here is my Main function:

static void Main(string[] args)
{
    try
    {
        Run(args);
    }
    catch (Exception ex)
    {
        Console.WriteLine();
        Console.WriteLine("An unexpected exception has occurred: " + Environment.NewLine + ex.ToString());
    }
}

Here is Run:

private static void Run(string[] args)
{
    try
    {
        if (args.Length == 0)
            throw new ConfigurationException("Expected at least one argument.");

        var runners = ReadConfiguration().SearchRunners.Where(runner => !runner.Disabled).ToList();
        var results = CollectResults(args, runners).Result;

        Console.WriteLine();

        ConsoleHelpers.PrintAsTable(results.Languages, results.Runners, results.Counts, "{0:n0}"); // Using 'ConsoleHelpers.PrintAsList' will print as a list instead.

        Console.WriteLine();

        ConsoleHelpers.PrintAsTable(
            new[] { "Winner" },
            results.Winners.Select(winner => winner.Key).ToList(),
            new[] { results.Winners.Select(w => w.Value).ToList() }.ToRectangularArray()
        );

        Console.WriteLine();

        Console.WriteLine("Total winner: {0}", results.Winner);
    }
    catch (ConfigurationException ex)
    {
        Console.WriteLine();
        Console.WriteLine(ex.Message);
    }
    catch (AggregateException ex)
    {
        ex.Handle(e =>
        {
            var searchException = e as SearchException;

            if (searchException != null)
            {
                Console.WriteLine();
                Console.WriteLine(string.Format("Runner '{0}' failed. {1}", searchException.Runner, searchException.Message));
                return true;
            }
            else
                return false;
        });
    }
}

PrintAsTable, which has this signature:

public static void PrintAsTable<T>(IReadOnlyList<string> rowHeaders, IReadOnlyList<string> colHeaders, T[,] values, string formatString = "{0}")

just prints the table to the console. Here is how the output would look like:

     | bing       | google      | stackoverflow | bingScrape | googleScrape
.net | 50,200,000 | 867,000,000 | 221,181       | 50,700,000 | 7,180,000,000
java | 41,800,000 | 47,600,000  | 963,553       | 41,700,000 | 407,000,000

       | bing | google | stackoverflow | bingScrape | googleScrape
Winner | .net | .net   | java          | .net       | .net

Total winner: .net

I did give them an option to print exactly as the original output by using PrintAsList instead of PrintAsTable.

The ReadConfiguration static method deserializes a Configuration class from an XML which is included with the project. Here is the Configuration class:

public class Configuration
{
    [XmlArrayItem("SearchRunner")]
    public List<SerializableSearchRunner> SearchRunners { get; set; }
}

The point of this class is to be serializable, so that an end user could add, remove or modify search engines by editing the XML, which is read at run time. Here is SerializableSearchRunner:

[XmlInclude(typeof(WebClientSearchRunner))]
public abstract class SerializableSearchRunner : ISearchRunner
{
    [XmlAttribute]
    public string Name { get; set; }

    [XmlAttribute]
    [DefaultValue(false)]
    public bool Disabled { get; set; }

    public abstract Task<long> Run(string query);
}

And here is ISearchRunner:

public interface ISearchRunner
{
    string Name { get; }
    bool Disabled { get; }
    Task<long> Run(string query);
}

This represents a search engine. Run is the function that takes a query string (a programming language name) and returns a long that contains the result count. So far, the only actual search runner is WebClientSearchRunner:

public class WebClientSearchRunner : SerializableSearchRunner
{
    [XmlAttribute]
    public string Address { get; set; }

    [XmlAttribute]
    public string QueryName { get; set; }

    public StringDictionary Headers { get; set; }
    public StringDictionary Parameters { get; set; }
    public ResultFinder Finder { get; set; }
    public ResultParser Parser { get; set; }
    public QueryFormatter QueryFormatter { get; set; }
    public TextClient Client { get; set; }

    public WebClientSearchRunner() { }

    public override async Task<long> Run(string query)
    {
        if (Finder == null)
            throw new ConfigurationException("Finder cannot be null.");

        if (Address == null)
            throw new ConfigurationException("Address cannot be null.");

        if (string.IsNullOrWhiteSpace(QueryName))
            throw new ConfigurationException("QueryName cannot be empty.");

        var uriBuilder = BuildUri(query);
        var responseText = await (Client ?? TextClient.Default).GetResponseText(uriBuilder.Uri, Headers);
        var resultText = Finder.Find(responseText);
        return (Parser ?? ResultParser.Default).Parse(resultText);
    }

    private UriBuilder BuildUri(string query)
    {
        var parameters = HttpUtility.ParseQueryString(String.Empty);

        if (Parameters != null)
        {
            foreach (var param in Parameters)
                parameters[param.Key] = param.Value;
        }

        parameters[QueryName] = (QueryFormatter ?? QueryFormatter.Default).FormatQuery(query);

        try
        {
            var uriBuilder = new UriBuilder(Address);
            uriBuilder.Query = parameters.ToString();
            return uriBuilder;
        }
        catch (UriFormatException ex)
        {
            throw new ConfigurationException("The given Address is not a valid URL.", ex);
        }
    }
}

StringDictionary is a serializable dictionary of strings (both key and values). ResultFinder, ResultParser, QueryFormatter and TextClient are all single method abstract classes (I removed a few things to make it shorter):

public abstract class ResultFinder { public abstract string Find(string responseText); }
public abstract class ResultParser { public abstract long Parse(string result); }
public abstract class QueryFormatter { public abstract string FormatQuery(string query); }
public abstract class TextClient { public abstract Task<string> GetResponseText(Uri uri, StringDictionary headers); }

I'm not posting the classes that implement these but they're all pretty straightforward. Here is a sample XML with a couple of search runners. This should give you an idea of what some implementations do:

<?xml version="1.0" encoding="utf-16"?>
<Configuration xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
  <SearchRunners>
    <SearchRunner xsi:type="WebClientSearchRunner" Name="stackoverflow" Address="https://api.stackexchange.com/2.2/questions" QueryName="tagged" Disabled="false">
      <Parameters>
        <Item Name="site" Value="stackoverflow" />
        <Item Name="filter" Value="!bCzphOiWu)Q3g)" />
      </Parameters>
      <Finder xsi:type="JSONResultFinder">
        <Path>["total"]</Path>
      </Finder>
    </SearchRunner>
    <SearchRunner xsi:type="WebClientSearchRunner" Name="bingScrape" Address="https://www.bing.com/search" QueryName="q" Disabled="false">
      <Finder xsi:type="RegexResultFinder" GroupIndex="1">
        <Pattern>\&lt;span[^\&gt;]+class="sb_count"[^\&gt;]*\&gt;([\d\.\,]+)</Pattern>
      </Finder>
    </SearchRunner>
  </SearchRunners>
</Configuration>

The RegexResultFinder is a nasty Finder that uses regex to search the result count in an HTML page. But then the JSONResultFinder can read JSON in case you'd rather use official APIs. I figured they may want me to read HTML since they're using MSN Search in the sample output, so I added both options.

Finally, the CollectResults static method (called from the static method Run on the very top of this post) is a simple wrap around the static method Collect in the Results class. Here is the Results class:

public class Results
{
    private readonly Lazy<IReadOnlyList<KeyValuePair<string, string>>> _Winners;
    private readonly Lazy<string> _Winner;
    public readonly IReadOnlyList<string> Languages;
    public readonly IReadOnlyList<string> Runners;
    public readonly long[,] Counts;

    public IReadOnlyList<KeyValuePair<string, string>> Winners
    {
        get { return _Winners.Value; }
    }

    public string Winner
    {
        get { return _Winner.Value; }
    }

    private Results(IReadOnlyList<string> languages, IReadOnlyList<string> runners, long[,] counts)
    {
        if (languages == null)
            throw new ArgumentNullException("languages");

        if (runners == null)
            throw new ArgumentNullException("runners");

        if (counts == null)
            throw new ArgumentNullException("results");

        if (languages.Count != counts.GetLength(0))
            throw new InvalidOperationException("Counts first length must equal languages length.");

        if (runners.Count != counts.GetLength(1))
            throw new InvalidOperationException("Counts second length must equal ruunners length.");

        Languages = languages;
        Runners = runners;
        Counts = counts;

        _Winners = new Lazy<IReadOnlyList<KeyValuePair<string, string>>>(GetWinners);
        _Winner = new Lazy<string>(GetWinner);
    }

    private IReadOnlyList<KeyValuePair<string, string>> GetWinners()
    {
        var result = new KeyValuePair<string, string>[Runners.Count];

        for (var ri = 0; ri < Runners.Count; ri++)
        {
            var winnerIndex = Languages.Indexes().Select(li => Counts[li, ri]).MaxIndex();
            var winner = Languages[winnerIndex];
            result[ri] = new KeyValuePair<string, string>(Runners[ri], Languages[winnerIndex]);
        }

        return result;
    }

    private string GetWinner()
    {
        var winnerIndex = Languages.Indexes().Select(li =>
            Runners.Indexes().Sum(ri => Counts[li, ri])
        )
        .MaxIndex();

        return Languages[winnerIndex];
    }

    public static async Task<Results> Collect(IReadOnlyList<string> languages, IReadOnlyList<ISearchRunner> runners, IProgressReporter progressReporter = null)
    {
        if (languages == null)
            throw new ArgumentNullException("languages");

        if (runners == null)
            throw new ArgumentNullException("runners");

        var results = new long[languages.Count, runners.Count];

        if (progressReporter != null)
        {
            progressReporter.Initialize(languages.Count * runners.Count);
        }

        List<Task> tasks = new List<Task>();

        for (var li = 0; li < languages.Count; li++)
        {
            for (var ri = 0; ri < runners.Count; ri++)
            {
                tasks.Add(StartTask(languages, runners, progressReporter, results, li, ri));
            }
        }

        await Task.WhenAll(tasks.ToArray());
        return new Results(languages, runners.Select(r => r.Name).ToList(), results);
    }

    private static async Task StartTask(IReadOnlyList<string> languages, IReadOnlyList<ISearchRunner> runners, IProgressReporter progressReporter, long[,] results, int li, int ri)
    {
        var arg = languages[li];
        var runner = runners[ri];

        try
        {
            var result = await runner.Run(arg);
            results[li, ri] = result;
        }
        catch (ConfigurationException ex)
        {
            throw new SearchException(arg, runner.Name, string.Format(ex.Message, arg, runner.Name), ex);
        }
        catch (WebRequestException ex)
        {
            throw new SearchException(arg, runner.Name, string.Format(ex.Message, arg, runner.Name), ex);
        }
        catch (ParsingException ex)
        {
            throw new SearchException(arg, runner.Name, string.Format(ex.Message, arg, runner.Name), ex);
        }

        if (progressReporter != null)
            progressReporter.Advance();
    }
}

Indexes is an IReadOnlyCollection extension that returns Enumerable.Range(0, collection.Count), and MaxIndex returns the index of the max element given an enumerable.

That's it. Hope is not too much code. Guess there's no need to analyze every single line to get an idea of what I did wrong.

\$\endgroup\$
3
  • 3
    \$\begingroup\$ I would say that "Don't use any external libraries" and "Adhere to patterns and best industry practices" are mutually exclusive. :) \$\endgroup\$ Nov 19, 2015 at 11:32
  • \$\begingroup\$ I hope you took away the comments from the original code for us, did you? \$\endgroup\$
    – N74
    Nov 19, 2015 at 12:10
  • \$\begingroup\$ @N74 Actually, I read these guy's email a couple of days late and didn't have enough time to write any documentation/comments. I also didn't feel it was a priority since all of these methods are pretty self explanatory. \$\endgroup\$
    – Juan
    Nov 20, 2015 at 8:33

1 Answer 1

5
\$\begingroup\$

Your code is really hard to follow. I think it is beacause the API you used is just way too complex for such a straightforward task. It's also weird. Like, why would result class... collect itself? I had troubles wrapping my head around this stuff.

I think you should keep it simple:

interface ISearchEngineFactory
{
    ISearchEngine[] CreateEngines();
}

interface ISearchEngine
{
    string Name { get; }
    Response Send(string query);
}

public class Response 
{
    public int HitCount { get; set; }
    public string Query { get; set; }
    public string SourceName { get; set; }
}

public class Result
{
    //make sure to synchronize this method
    public void Aggregate(Response response)
    {
        ...
    }

    //override it to return required output
    public override string ToString()
    {
        ...
    }
}

Your Main function will then look like this (you can wrap it in try catch if you feel like it):

//move your initialization logic to factory
ISearchEngineFactory factory = new ConfigurationFactory();
var engines = factory.CreateEngines();
var result = new Result();
Parallel.ForEach(engines, engine => 
                                {
                                    foreach(var query in args)
                                    {
                                        var response = engine.Send(query);
                                        result.Aggregate(response);
                                    }
                                });
Console.WriteLine(result);

Some things can still be improved. You could add a timeout parameter to Send method, for example. But hopefully you get the idea.

As for your WebClient implementation - I can't really comment on that, since I am not experienced enough in web development. Maybe someone else will. :)

\$\endgroup\$
5
  • \$\begingroup\$ The problem with storing the query and sources by name in Response is that the original order is lost, so you'd need to also store the indexes (of both search engines and queries), but having both names and indexes seems redundant. Also, having indexes for a table that is stored in another class seems weird, if not plain wrong. Which is why I let the Results table deal with the indexes while creating itself. The other option would be a class whose only purpose is to create a Results instance. But I don't think is uncommon for classes to create themselves in a static method. \$\endgroup\$
    – Juan
    Nov 23, 2015 at 20:52
  • \$\begingroup\$ Also, the ToString method in the Result table seems weird to me. Since there's no obvious and easy way of printing the results, I'd need to edit ToString every time I decide to change how the output looks. And it feels like there'd be too much visual logic inside a class that is just meant to collect and store data. That's why I have a PrintAsTable method, which would belong to the "view" side of the program's structure. I do agree with moving the configuration loading logic outside Program.cs. I guess could've even put it inside the Configuration class itself. \$\endgroup\$
    – Juan
    Nov 23, 2015 at 21:09
  • \$\begingroup\$ @Juan, "but I don't think is uncommon for classes to create themselves in a static method" - it is not. It is uncommon for a class to create a collection of generic tasks, which return instances of this class, though. Managing tasks manually is a complex problem, I think it deserves a dedicated entity. \$\endgroup\$
    – Nikita B
    Nov 24, 2015 at 7:06
  • \$\begingroup\$ @Juan, I didn't think that order is important. If it is, then I would additionally pass collection of queries (the args parameter) to Result constructor. Then you could easily generate required output with a couple of LINQ queries over the collection of responses. I don't see anything wrong with that. \$\endgroup\$
    – Nikita B
    Nov 24, 2015 at 7:16
  • \$\begingroup\$ @Juan, as for ToString method: changing the output format should be as easy as overriding ToString again in a subclass. Yes, you can create a dedicated class for printing, but should you? I mean, you were not asked to create diffferent outputs. Aren't you overengineering? If you are not, then you should implement it properly. Create an IOutput interface with single Print method and create two different implementations - one to print as a table, another to print as text. And then use this interface in your code instead of static methods. \$\endgroup\$
    – Nikita B
    Nov 24, 2015 at 7:31

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.