I had 15 minutes to code the following question:

Given a file name return the char which appears maximal number of times.

please do not take into account the unit test. Please look at the final foreach loop, I could have also used a for loop, I have doubts what is the best way to implement finding a max value and get the key from the dictionary.

Also please comment about code style. Thanks

using System.Collections.Generic;
using System.IO;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace JobInterviewTests
{

    [TestClass]
    public class GetMostRepeatedCharQuestion
    {
        [TestMethod]
        public void TestForAasMax()
        {
            char result = GetMaxCharHelper.GetMaxReaptedCharFromFile("MaxA.txt");
            Assert.AreEqual('A', result);
        }
    }

    public class GetMaxCharHelper
    {
        public static char GetMaxReaptedCharFromFile(string fileName)
        {
            Dictionary<char, int> charToCount = new Dictionary<char, int>();
            using (var streamReader = new StreamReader(fileName))
            {
                string line = streamReader.ReadLine();
                while (line != null)
                {
                    char[] charArray = line.ToCharArray();
                    foreach (var currentChar in charArray)
                    {
                        if (charToCount.ContainsKey(currentChar))
                        {
                            charToCount[currentChar]++;
                        }
                        else
                        {
                            charToCount.Add(currentChar, 1);
                        }
                    }
                    //don't forget to get next line when reading from a file
                    line = streamReader.ReadLine();

                }

                int maxCount = 0;
                char maxChar = '\0';
                foreach (var current in charToCount)
                {
                    if (current.Value > maxCount)
                    {
                        maxCount = current.Value;
                        maxChar = current.Key;
                    }
                }
                return maxChar;
            }
        }
    }
}
up vote 6 down vote accepted

Assuming modern C#, there are a couple of things you could do to be more concise:

  1. Instead of the dance with while((line = reader.NextLine())...) you can just use File.ReadLines and foreach over that.

  2. You can declare variables inside the TryGetValue like so: charToCount.TryGetValue(currentChar, out int count) where you can use count afterwards without having to declare it on its own line.

  3. While keeping score of your most seen char inside the loop is certainly more efficient (as @pattpass did in their answer), you could also extract the character with the highest count at the end with a still readable oneliner:

    return charToCount.OrderByDescending(cc => cc.Value).First().Key;
    
  4. What's completely absent in your method is error handling. What happens if the file does not exist or you aren't allowed to access it? It seems like you handle empty files with returning \0 and 0. It is debatable if that's what one would expect.

All in all, your code could look somewhat likes this:

static char GetMostRepeatedChar(string filename)
{
    // you could also not handle this case here and delegate
    // that responsibility to the caller
    if(!File.Exists(filename))
    {
        return '\0';
    }

    var charCount = new Dictionary<char, int>();
    foreach (var line in File.ReadLines(filename))
    {
        foreach (var c in line)
        {
            charCount.TryGetValue(c, out int count);
            charCount[c] = count + 1;
        }
    }

    if (charCount.Count == 0)
    {
        // this is debatable and depends on the real use case
        // in production code you should never throw a raw 'Exception'
        // but rather something more specific
        throw new Exception("empty file"); 
    }

    return charCount.OrderByDescending(cc => cc.Value).First().Key;
}
  • I think that is is fine to throw an exception if file if not found. I think that's what most developers would expect when working with IO. Otherwise I like your suggestions, good refactoring. – Nikita B Aug 10 at 5:39
  • Germi i did check for file exists however the interviewer said. Why did you check only if file exists? What about file locked. Or havinf no permissions and so on all in all I think stream reader can throw better exceptions and i need to try catch them later on and log them or show error to the user – Gilad Aug 10 at 6:26

According to a modern C# and LINQ you could try to "shock" interviewer with solving in a row:

var res = File.ReadAllText(path)
            .Where(ch => char.IsLetter(ch))
            .GroupBy(x => x)
            .OrderByDescending(x => x.Count())
            .First()
            .Key;
  • 2
    Finally! I was already going to cry because nobody have posted it yet ;-) Additionally instead of sorting it you could just use .Max() – t3chb0t Aug 10 at 10:59
  • Oh, no, you cannot, sorry, my bad - you want the key not the count ;-] – t3chb0t Aug 10 at 11:22
  • You should probably use File.ReadLines instead. Not that it matters much, since GroupBy will load the entire file into memory either way. Still , no reason to load it twice. This attempt to shock the interviewer can easily backfire, if you can't explain the implications of using LINQ very well during the interview. :) – Nikita B Aug 11 at 21:31

Your Answer

 
discard

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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