4
\$\begingroup\$

This is my revised solution to Problem 42 from Project Euler, based on suggestions in this original post Project Euler 42 - Finding triangle numbers

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.IO;
using System.Collections;
using System.Diagnostics;

namespace ProjectEuler
{
    class p42trianglenumber
    {
        // p42. tn = ½n(n+1)
        // A=1, B=2, etc. case insensitive. 
        public static void NotMain()
        {                   
            var sw = new Stopwatch();
            sw.Start();
            int numberTriangleWords = 0;            
            string[] words = ReturnStringArray("p042_words.txt");
            int[] triangleNumbers = BuildTriangleNumber();

            foreach (string word in words)
            {
                var score = GetScore(word);                

                if (triangleNumbers.Contains(score))
                {
                    numberTriangleWords++;
                }
            }
            sw.Stop();
            Console.WriteLine(numberTriangleWords + Environment.NewLine + sw.ElapsedMilliseconds.ToString() + "ms");
            Console.ReadLine();
        }

        private static string[] ReturnStringArray(string fileLocation)
        {
            StreamReader sr = new StreamReader(fileLocation);
            string[] words = sr.ReadLine().Split(',');
            return words;
        }

        private static bool IsWordTriangleNumber(int wordVal, int[] triangleNumbers)
        {
            return (triangleNumbers.Contains(wordVal));            
        }

        private static int[] BuildTriangleNumber()
        {
            int triangleNumberValue = 0;
            int[] triangleNumbers = new int[50];

            for (int i = 0; i < 50; i++)
            {
                if (i == 0)
                    triangleNumbers[i] = triangleNumberValue;
                else
                {
                    triangleNumberValue += i;
                    triangleNumbers[i] = triangleNumberValue;
                }
            }
            return triangleNumbers;
        }       

        private static int GetScore(string word)
        {
            var total = 0;
            foreach (var letter in word)
            {
                total += GetScore(letter);
            }
            return total;
        }

        private static int GetScore(char letter)
        {
            var start = char.IsLower(letter) ? 'a' : 'A';
            var score = (int)letter - (int)start + 1;
            if (score < 0 || score > 26) return 0;
            return score;
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ What exactly do you need help with? What concerns do you have? \$\endgroup\$ – Nick B. Aug 16 '15 at 17:21
  • \$\begingroup\$ Apologies, I've now included my original link. Essentially I asked for pointers with my original code, went away and updated accordingly, and have been informed that improvements be posted in a new question. I'm happy with the above, and as it's only a small piece of code don't think there's much room for improvement now. \$\endgroup\$ – kafka Aug 16 '15 at 17:28
3
\$\begingroup\$

Possible problem

private static string[] ReturnStringArray(string fileLocation)
{
    StreamReader sr = new StreamReader(fileLocation);
    string[] words = sr.ReadLine().Split(',');
    return words;
}  

With this method you don't read the whole file but only the first line in the textfile. Usually project Euler problem don't involve only one line of anything but rather some 10k of something.


In the first 5 lines of code I would change 3 variables/method names

        int numberTriangleWords = 0;            
        string[] words = ReturnStringArray("p042_words.txt");
        int[] triangleNumbers = BuildTriangleNumber();  

The problem is about getting the number of triangle words.

int numberOfTriangleWords = 0  

You are reading a text file as words.

string[] words = ReadFileAsWords("p042_words.txt");  

You are creating / building an array of numbers

int[] triangleNumbers = BuildTriangleNumbers();  

This is just nitpicking on the naming, but naming is a very important part of writing good readable and maintainable code.


This method

    private static bool IsWordTriangleNumber(int wordVal, int[] triangleNumbers)
    {
        return (triangleNumbers.Contains(wordVal));            
    }  

isn't used anywhere. You should delete it.


    private static int[] BuildTriangleNumber()
    {
        int triangleNumberValue = 0;
        int[] triangleNumbers = new int[50];

        for (int i = 0; i < 50; i++)
        {
            if (i == 0)
                triangleNumbers[i] = triangleNumberValue;
            else
            {
                triangleNumberValue += i;
                triangleNumbers[i] = triangleNumberValue;
            }
        }
        return triangleNumbers;
    }  

Because triangleNumbers is an array of the int struct, the items are initialized using default(T) which for an int is 0. Therefor if you just let the loop start at 1 you wouldn't need the check if (i == 0) at all which would also remove the braceless single line statement after the if.
I would like to encourage you to always use braces {} to make your code less error prone. Saving a few milliseconds by omitting them can lead to problems later on which patching can take a lot of time.

    private static int[] BuildTriangleNumber()
    {
        int triangleNumberValue = 0;
        int[] triangleNumbers = new int[50];

        for (int i = 1; i < 50; i++)
        {
            triangleNumberValue += i;
            triangleNumbers[i] = triangleNumberValue;
        }
        return triangleNumbers;
    }  

I like the overloading of the GetScore() method, but the GetScore(char) method can be improved.

    private static int GetScore(char letter)
    {
        var start = char.IsLower(letter) ? 'a' : 'A';
        var score = (int)letter - (int)start + 1;
        if (score < 0 || score > 26) return 0;
        return score;
    }  

There is no need to first assign a or A to start and then later on cast it to an int. The int value of a == 97 and of A == 65. So using these values should speed the whole thing up.

Edit Based on @Caridorc's comment I have extracted these values to private static reaonly variables using a naming scheme which isn't legit by the meaning of the c# naming guidelines but looks better with the underscore in it.

    private static int ASCIIValue_a = 97;
    private static int ASCIIValue_A = 65;
    private static int GetScore(char letter)
    {
        var start = char.IsLower(letter) ? ASCIIValue_a : ASCIIValue_A;
        var score = (int)letter - start + 1;
        if (score < 0 || score > 26) return 0;
        return score;
    }
\$\endgroup\$
  • 1
    \$\begingroup\$ _ There is no need to first assign a or A to start and then later on cast it to an int_ There is, it is called readibility. Not everyone knows Ascii codes by heart. \$\endgroup\$ – Caridorc Aug 17 '15 at 7:49
  • 1
    \$\begingroup\$ For readability as well as intent, you could use private static int ASCIIValue_a = (int)'a'; or simply private static int ASCIIValue_a = 'a'; \$\endgroup\$ – Rick Davin Aug 17 '15 at 12:30
3
\$\begingroup\$

Two method of yours have the same name (GetScore). Very confusing, rename one of them.

Also some more Linq would make the code better, for example (non-tested):

private static int AlphaFullScore(string word) 
{
    return word.Sum(GetScore);
}

Similarly, the function to generate triangle numbers can be made much simpler (and a tiny bit slower maybe, but it does not matter in this program):

private static int[] FirstTriangleNumbers(int limit)
{
    return Range(1, limit).Select(n => n * (n + 1) / 2);
}     

It takes an argument now, as before it could only generate 50 triangle numbers. You may want to write a comment about the formula for summing all the numbers below n.

\$\endgroup\$
  • \$\begingroup\$ I don't think there's Map in LINQ. It's called Select. In any case, Sum takes a lambda so neither Map nor Select are required - word.Sum(x => GetScore(x)) would be enough, or better still word.Sum(GetScore). Admittedly I didn't test it either, it's just off the top of my head \$\endgroup\$ – Konrad Morawski Aug 17 '15 at 11:14
  • \$\begingroup\$ @KonradMorawski Excellent suggestion, the code is now much simpler \$\endgroup\$ – Caridorc Aug 17 '15 at 11:16
2
\$\begingroup\$

First, In method ReturnStringArray, you're creating an instance of an object, StreamReader that implements the IDisposable interface:

private static string[] ReturnStringArray(string fileLocation)
{
    StreamReader sr = new StreamReader(fileLocation);
    string[] words = sr.ReadLine().Split(',');
    return words;
}

Because of such, you really need to dispose of it deterministically when you're done with it. This is idiomatically done with the using construct as such:

private static string[] ReturnStringArray(string fileLocation)
{
    using (StreamReader sr = new StreamReader(fileLocation))
    {
        string[] words = sr.ReadLine().Split(',');
        return words;
    }
}

You could also shorten the method up a little bit for readability:

private static string[] ReturnStringArray(string fileLocation)
{
    using (var sr = new StreamReader(fileLocation))
    {
        return sr.ReadLine().Split(',');
    }
}

I also like to return the most base type that you're going to need. In this case, you're just using foreach over the returned array in the method NotMain, so it may work better as an IEnumerable<string> (let's rename it to match its new functionality):

private static IEnumerable<string> ReturnStrings(string fileLocation)
{
    using (var sr = new StreamReader(fileLocation))
    {
        return sr.ReadLine().Split(',');
    }
}

As has been mentioned, take heed to note that this only reads the first line of the file, which may or may not meet the requirements of the challenge.

Next, if you're able to, use LINQ to transform the method GetScore from:

private static int GetScore(string word)
{
    var total = 0;
    foreach (var letter in word)
    {
        total += GetScore(letter);
    }
    return total;
}

to

private static int GetScore(string word)
{
    return word.Sum(letter => GetScore(letter));
}

Short, to the point, and, most importantly, shows the intent of the method rather than the mechanisms. You can also do that with the triangle scoring loop in NotMain:

int[] triangleNumbers = BuildTriangleNumber();

foreach (string word in words)
{
    var score = GetScore(word);                

    if (triangleNumbers.Contains(score))
    {
        numberTriangleWords++;
    }
}

becomes a one liner:

var numberTriangleWords = words.Select(GetScore).Count(score => triangleNumbers.Contains(score));

Also, Stopwatch has a handy static method which turns:

var sw = new Stopwatch();
sw.Start();

into:

var sw.Stopwatch.StartNew();

And while we've got that here, I'd like to mention that your use of var is inconsistent, even in that very block. My own personal preference is to use var everywhere I can. But having the explicit type is just as valid as a position (except where var is required - anonymous types). Just be consistent for readability's sake.

Those are just a few things off the top of my head.

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

What's missing here is the (now deleted) update to your original link. In that brief lived update there were 2 problems not known to others here. One was the stopwatch wasn't being started. The other was the OP wanted help understanding this line:

var start = char.IsLower(letter) ? 'a' : 'A';

My (also now deleted answer was):

letter is a char and its checked to see if its lowercase. If it is start gets the value of a. If not, start gets the value of A. It is equivalent to this:

char start;
if (char.IsLower(letter))
{
    start = 'a';
}
else
{
    start = 'A';
}

But a lot shorter. Since I wrote the original snippet, the logic is taht later you will use the integer code for letter and subtract an appropriate offset, that is the integer code of start. Obviously when letter is uppercase we want the offset to be A but if letter is lowercase we want it to be a.

So I trust your timings work better and most of the mystery is gone.

\$\endgroup\$
  • \$\begingroup\$ Thanks again for input. Need to refresh my memory with ASCII codes now! \$\endgroup\$ – kafka Aug 17 '15 at 14:21

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.