11
\$\begingroup\$

I am filling a bag full of scrabble tiles. There should be 100 tiles in the bag of these letters:

A-9, B-2, C-2, D-4, E-12, F-2, G-3, H-2, I-9, J-1, K-1, L-4, M-2, N-6, O-8, P-2, Q-1, R-6, S-4, T-6, U-4, V-2, W-2, X-1, Y-2, Z-1 and Blanks-2.

These numbers/letters should never change. I have a working solution, but it is horrible. There is a lot of duplicate code, a lot of room for errors, and it has very poor readability. I am stumped on how I can make this code more readable and less error-prone. Any suggestions? I am not opposed to scratching this class altogether, if anyone knows of a better approach.

Bag.cs

public class Bag
{
    private Tile[] tiles = new Tile[100];
    private const int numA = 9;
    private const int numB = 2;
    private const int numC = 2;
    private const int numD = 4;
    private const int numE = 12;
    private const int numF = 2;
    private const int numG = 3;
    private const int numH = 2;
    private const int numI = 9;
    private const int numJ = 1;
    private const int numK = 1;
    private const int numL = 4;
    private const int numM = 2;
    private const int numN = 6;
    private const int numO = 8;
    private const int numP = 2;
    private const int numQ = 1;
    private const int numR = 6;
    private const int numS = 4;
    private const int numT = 6;
    private const int numU = 4;
    private const int numV = 2;
    private const int numW = 2;
    private const int numX = 1;
    private const int numY = 2;
    private const int numZ = 1;
    private const int numBlank = 2;

    public void populateTiles()
    {
        int i = 0;
        for (int j = 0; j < numA; j++)
        {
            tiles[i] = new Tile('A');
            i++;
        }
        for (int j = 0; j < numB; j++)
        {
            tiles[i] = new Tile('B');
            i++;
        }
        for (int j = 0; j < numC; j++)
        {
            tiles[i] = new Tile('C');
            i++;
        }
        for (int j = 0; j < numD; j++)
        {
            tiles[i] = new Tile('D');
            i++;
        }
        for (int j = 0; j < numE; j++)
        {
            tiles[i] = new Tile('E');
            i++;
        }
        for (int j = 0; j < numF; j++)
        {
            tiles[i] = new Tile('F');
            i++;
        }
        for (int j = 0; j < numG; j++)
        {
            tiles[i] = new Tile('G');
            i++;
        }
        for (int j = 0; j < numH; j++)
        {
            tiles[i] = new Tile('H');
            i++;
        }
        for (int j = 0; j < numI; j++)
        {
            tiles[i] = new Tile('I');
            i++;
        }
        for (int j = 0; j < numJ; j++)
        {
            tiles[i] = new Tile('J');
            i++;
        }
        for (int j = 0; j < numK; j++)
        {
            tiles[i] = new Tile('K');
            i++;
        }
        for (int j = 0; j < numL; j++)
        {
            tiles[i] = new Tile('L');
            i++;
        }
        for (int j = 0; j < numM; j++)
        {
            tiles[i] = new Tile('M');
            i++;
        }
        for (int j = 0; j < numN; j++)
        {
            tiles[i] = new Tile('N');
            i++;
        }
        for (int j = 0; j < numO; j++)
        {
            tiles[i] = new Tile('O');
            i++;
        }
        for (int j = 0; j < numP; j++)
        {
            tiles[i] = new Tile('P');
            i++;
        }
        for (int j = 0; j < numQ; j++)
        {
            tiles[i] = new Tile('Q');
            i++;
        }
        for (int j = 0; j < numR; j++)
        {
            tiles[i] = new Tile('R');
            i++;
        }
        for (int j = 0; j < numS; j++)
        {
            tiles[i] = new Tile('S');
            i++;
        }
        for (int j = 0; j < numT; j++)
        {
            tiles[i] = new Tile('T');
            i++;
        }
        for (int j = 0; j < numU; j++)
        {
            tiles[i] = new Tile('U');
            i++;
        }
        for (int j = 0; j < numV; j++)
        {
            tiles[i] = new Tile('V');
            i++;
        }
        for (int j = 0; j < numW; j++)
        {
            tiles[i] = new Tile('W');
            i++;
        }
        for (int j = 0; j < numX; j++)
        {
            tiles[i] = new Tile('X');
            i++;
        }
        for (int j = 0; j < numY; j++)
        {
            tiles[i] = new Tile('Y');
            i++;
        }
        for (int j = 0; j < numZ; j++)
        {
            tiles[i] = new Tile('Z');
            i++;
        }
        for (int j = 0; j < numBlank; j++)
        {
            tiles[i] = new Tile(' ');
            i++;
        }
    }
}

Tile.cs

public class Tile
{
    private char letter;
    private int value;

    public Tile(char letter)
    {
        switch (letter)
        {
            case ' ':
                value = 0;
                break;
            case 'A':
            case 'E':
            case 'I':
            case 'O':
            case 'U':
            case 'L':
            case 'N':
            case 'S':
            case 'T':
            case 'R':
                value = 1;
                break;
            case 'D':
            case 'G':
                value = 2;
                break;
            case 'B':
            case 'C':
            case 'M':
            case 'P':
                value = 3;
                break;
            case 'F':
            case 'H':
            case 'V':
            case 'W':
            case 'Y':
                value = 4;
                break;
            case 'K':
                value = 5;
                break;
            case 'J':
            case 'X':
                value = 8;
                break;
            case 'Q':
            case 'Z':
                value = 10;
                break;
            default:
                Console.Error("Invalid Tile created!");
                break;
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Where's the code for how many points each tile is worth? (Do I even dare to ask?) \$\endgroup\$ – Simon Forsberg Jun 20 '15 at 20:25
  • \$\begingroup\$ @SimonAndréForsberg When a new Tile is created, it checks the letter, and assigned the associated value. Good or bad? (I added the code) \$\endgroup\$ – Evorlor Jun 20 '15 at 20:26
  • \$\begingroup\$ Use a map to store the scores of each letter! Then to do a lookup you can do something like scoreOfLetter['a']. If will add some overhead, but it should be cinsiderably neater. \$\endgroup\$ – Carcigenicate Jun 21 '15 at 16:19
13
\$\begingroup\$

I'm sorry, I just love to make things flexible so much that I have to take it one step further...

I will use a mix between @EthanBierlein's version and your original, and improve on that, making the following changes:

  • Extracting a method for adding a specific amount of a tile
  • Being able to specify the points you get from a tile here as well

There is really no reason in my opinion to separate the configuration of how many tiles of a certain letter there should be, and how many points that tile should be worth. It is more reasonable, and more maintainable to keep them all at one place (consider the places you would need to change if you wanted to make Scrabble for other languages, different languages have different amount of letters and different score for them, not to mention different alphabets...)

This will lead to the Scrabble setup being much more changable, you can have A tiles have different point values, you can easily change the number of E tiles you have in your board, etc. You could even extend on this idea and read from a configuration file to determine how many tiles you should have for each square.

I'm not saying that you should do all this, I'm just saying that by changing your code a bit, you can do this.

using System.Collections.Generic;

class Bag
{
    public List<Tile> tiles = new List<Tile>();

    /// <summary>
    /// Populate the list tiles with scrabble tiles.
    /// </summary>
    public void PopulateTiles()
    {
        AddTile(9, 'A', 1);
        AddTile(2, 'B', 42);
        AddTile(2, 'C', 42);
        AddTile(4, 'D', 42);
        // ...
    }

    public void AddTile(int count, char letter, int score)
    {
        for (int i = 0; i < count; i++)
        {
            Tile tile = new Tile(letter, score); // Pass the score along to the tile as well
            tiles.Add(tile);
        }
    }
}

Small note: Method names should use PascalCase naming conventions, not camelCase. As can be seen in my code, populateTiles should be PopulateTiles.

\$\endgroup\$
13
\$\begingroup\$

Forewarning: I'm not the most experienced C# programmer, so if anything is wrong, just mention it.


There is way too much useless repetition here. This begs for an implementation using System.Collections.Generic.Dictionary. You can do something like this to store the possible tiles, and their counts.

public Dictionary<char, int> possibleTiles = new Dictionary<char, int>() {
    {'a', 9},
    {'b', 2},
    {'c', 2},
    ...
};

You can then loop over this dictionary like this. You would use the key as the tile to be added to your tiles array, and the value as the amount of those specific tiles.

foreach(KeyValuePair<char, int> item in possibleTiles)
{
    // Add item.key item.value times to the tiles array.
}

Finally, for reference, here's the my refactored version of the Bag class. Do note though, it uses List<Tile> rather than an array.

using System;
using System.Collections.Generic;

/// <summary>
/// Represents a bag of scrabble tiles.
/// </summary>
class Bag
{
    public List<Tile> tiles = new List<Tile>();
    private Dictionary<char, int> possibleTiles = new Dictionary<char, int>() {
        {'a', 9}, {'b', 2}, {'c', 2}, {'d', 4}, {'e', 12},
        {'f', 2}, {'g', 3}, {'h', 2}, {'i', 9}, {'j', 1},
        {'k', 1}, {'l', 4}, {'m', 2}, {'n', 6}, {'o', 8},
        {'p', 2}, {'q', 1}, {'r', 6}, {'s', 4}, {'t', 6},
        {'u', 4}, {'v', 2}, {'w', 2}, {'x', 1}, {'y', 2},
        {'z', 1}, {' ', 2}
    };

    /// <summary>
    /// Populate the list tiles with scrabble tiles.
    /// </summary>
    public void PopulateTiles()
    {
        foreach(KeyValuePair<char, int> item in possibleTiles)
        {
            Tile tile = new Tile(item.Key);
            int amount = item.Value; 
            for(int n = 0; n <= amount; n++)
            {
                tiles.Add(tile);
            }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ You can use char I think, no need to treat them as string objects. \$\endgroup\$ – Simon Forsberg Jun 20 '15 at 20:19
  • \$\begingroup\$ @SimonAndréForsberg Edited answer. \$\endgroup\$ – Ethan Bierlein Jun 20 '15 at 20:21
  • 1
    \$\begingroup\$ Can quite easily be refactored again to use an array instead, but I think this is a quite good solution. \$\endgroup\$ – Simon Forsberg Jun 20 '15 at 20:41
  • \$\begingroup\$ @SimonAndréForsberg Thanks! I usually prefer to use List<T> because I come from a Python world with certain methods on lists like C#'s List<T>, like .Add(). \$\endgroup\$ – Ethan Bierlein Jun 20 '15 at 20:42
8
\$\begingroup\$

Hmm, you do seem to have a lot of repetition:

    for (int j = 0; j < numD; j++)
    {
        tiles[i] = new Tile('D');
        i++;
    }
    for (int j = 0; j < numE; j++)
    {
        tiles[i] = new Tile('E');
        i++;
    }

What is unique between these two? Why can't we just copy and paste them? The answer is numD\numE and 'D'\'E'.

We can immediately then think - imagine if these were values instead of hardcoded?

List<Tile> tiles = new List<Tile>(); //For simplicity.

public void AddTiles(char letter, int count)
{
    var newTiles = Enumerable.Range(0,count)
                   .Select(_ => new Tile(letter));
    tiles.AddRange(newTiles);
}

Wow that's simpler! We don't have to keep track of our current index i, the index of each loop j, how many tiles we need to add new Tile[100] or any loop conditionals!

We aren't keeping track of the values well, so let's follow Simon André Forsberg's lead and also keep track of the score:

public void AddTiles(char letter, int count, int score)
{
    var newTiles = Enumerable.Range(0,count)
                   .Select(_ => new Tile(letter, score));
    tiles.AddRange(newTiles);
}

Wow! We just made a huge change to the function of the program, and only had to add a single parameter! Pretty simple eh?

Still, I'm not happy. How are we calling AddTiles? Like this?

 public void PopulateTiles()
{
    AddTiles(9, 'A', 1);
    AddTiles(2, 'B', 3);
    AddTiles(2, 'C', 3);
    AddTiles(4, 'D', 2);
    // ...
}

That doesn't feel right to me. It's long, it's repetitive, and it's very hard coded - we can't easily change dictionaries. If only we had a simple way of representing the tiles we want? Oh wait, we do!

A-9, B-2, C-2, D-4, E-12, F-2, G-3, H-2, I-9, J-1, K-1, L-4, M-2, N-6, O-8, P-2, Q-1, R-6, S-4, T-6, U-4, V-2, W-2, X-1, Y-2, Z-1 and Blanks-2.

You already have a simple concise way of getting your intent out... so why don't you use that? Why reinvent the wheel?

I will however make some minor variations to reduce risk of typo, allow scores in, and make 'blank' less magical:

A:9:1,B:2:3,C:2:3,D:4:2,E:12:1,F:2:4,G:3:2,H:2:4,I:9:1,J:1:8,K:1:5,L:4:1,M:2:3,N:6:1,O:8:1,P:2:3,Q:1:10,R:6:1,S:4:1,T:6:1,U:4:1,V:2:4,W:2:4,X:1:8,Y:2:4,Z:1:10, :2:0

Now, that's pretty easy to modify change and read (though with more time/space I'd use newline instead of ,, or json/csv to make reading a little cleaner).

Now, my program looks a little something like so:

const string defaultAlphabet = "A:9:1,B:2:3,C:2:3,D:4:2,E:12:1,F:2:4,G:3:2,H:2:4,I:9:1,J:1:8,K:1:5,L:4:1,M:2:3,N:6:1,O:8:1,P:2:3,Q:1:10,R:6:1,S:4:1,T:6:1,U:4:1,V:2:4,W:2:4,X:1:8,Y:2:4,Z:1:10, :2:0"

List<Tile> tiles = new List<Tile>(); //For simplicity.

public void Populate(string alphabet = defaultAlphabet)
{
    foreach (var stringTile in alphabet.Split(','))
    {
        var parts = stringTile.Split(':');
        AddTiles(parts[0], int.Parse(parts[1]), int.Parse(parts[2]));
    }
}

public void AddTiles(string letter, int count, int score)
{
    var newTiles = Enumerable.Range(0,count)
                   .Select(_ => new Tile(letter, score));
    tiles.AddRange(newTiles);
}

Notice how little code we now have? This is good - because if we want to change the behaviour of the application, we only have to change a few lines of code - there are no redundancies for us to worry about. However without changing any code at all, we can drastically change the alphabet used - maybe even make it user changeable!

\$\endgroup\$
  • \$\begingroup\$ I like your use of LINQ, but I don't like the const string and the use of alphabet.Split, it would be very easy to accidentally slip in A:s9:1 or A::9:1 or something similar that would screw things up. I don't see what this string format gains, if you would compare it with for example Ethan's dictionary \$\endgroup\$ – Simon Forsberg Jun 25 '15 at 10:10
  • \$\begingroup\$ @SimonAndréForsberg the string A) is a much shorter, simpler variation B) is trivial to store in a database, text file, enter in manually etc. Using a dictionary just keeps enforce the idea that this is hardcoded, something I'm arguing against. As I mention in my post I'm not happy with using , as a delimiter and give alternatives. Most importantly though, it was what the developer thought of - don't underestimate the value of intent. \$\endgroup\$ – NPSF3000 Jun 25 '15 at 11:04
  • \$\begingroup\$ I'm not really buying the "hardcoded" argument, as I consider your string to be exactly hardcoded. It is quite trivial to store a dictionary in a database too, and would also lead to better database normalization. It is not very hard to load a dictionary from file or anything either... Sorry, you can of course use it if you want to, but I wouldn't recommend it. \$\endgroup\$ – Simon Forsberg Jun 25 '15 at 11:40
  • \$\begingroup\$ @SimonAndréForsberg "as I consider your string to be exactly hardcoded." Sure if you ignore the text. I focus on a string, because my text describes how to get the string out of the code. Ethan Bierlein focusses on Dictionary because his post is about a particular implementation in C#. I show that external data can be parsed from one human-readable format to another, more computable format, with a local string as a default. Ethan shows one can hard code data in a dictionary, and then copy that data to another collection (why not just hard code it correctly there?) \$\endgroup\$ – NPSF3000 Jun 25 '15 at 12:20
7
\$\begingroup\$

Here's a much simpler way of doing this by abstracting the operation.

public class Bag
{
    const string tileTypes =
        "AAAAAAAAABBCCDDDDEEEEEEEEEEEEFFGGGHHIIIIIIIIIJKLLLL" +
        "MMNNNNNNOOOOOOOOPPQRRRRRRSSSSTTTTTTUUUUVVWWXYYZ  ";

    private char[] tiles = new char[tileTypes.Length];

    public void populateTiles()
    {
        for (int i = 0; i < tiles.Length; i++)
        {
            tiles[i] = new Tile(tileTypes[i]);
        }
    }
}

One could use a run-length encoded data structure for tileTypes instead, but I don't see any particular benefit.

\$\endgroup\$
  • \$\begingroup\$ Although possible, this is not the approach I would take. \$\endgroup\$ – Simon Forsberg Jun 20 '15 at 20:18
  • \$\begingroup\$ I agree with @SimonAndréForsberg. While this is certainly one of the shortest ways to do it, it's not quite as readable as some of the other approaches. \$\endgroup\$ – Ethan Bierlein Jun 20 '15 at 20:20
  • 4
    \$\begingroup\$ @JeroenVannevel Sorry, I just find it hard to determine exactly how many tiles there are. Can you quickly tell me how many A's or E's there are, without having memorized the actual amount? This approach hurts readability. \$\endgroup\$ – Simon Forsberg Jun 20 '15 at 20:56
  • 1
    \$\begingroup\$ @SimonAndréForsberg: why would you ever need to know that quickly, though? If that's the argument, you could always clarify that in a comment. \$\endgroup\$ – Jeroen Vannevel Jun 20 '15 at 20:57
  • 4
    \$\begingroup\$ @JeroenVannevel Different languages use different amounts of letters, and different scores for the letters. (And then there's the different alphabets of course...) \$\endgroup\$ – Simon Forsberg Jun 20 '15 at 21:02
4
\$\begingroup\$

This is basically the same as NPSF3000's answer, but it uses LINQ's query syntax. It's more readable to me and doesn't create side effects like having a method populate a list declared outside of it.

const string defaultAlphabet = "A:9:1,B:2:3,C:2:3,D:4:2,E:12:1,F:2:4,G:3:2,H:2:4,I:9:1,J:1:8,K:1:5,L:4:1,M:2:3,N:6:1,O:8:1,P:2:3,Q:1:10,R:6:1,S:4:1,T:6:1,U:4:1,V:2:4,W:2:4,X:1:8,Y:2:4,Z:1:10, :2:0";

List<Tile> tiles =
   (from stringTile in defaultAlphabet.Split(',')
    let parts = stringTile.Split(':')
    let letter = parts[0]
    let count = int.Parse(parts[1])
    let score = int.Parse(parts[2])
    from x in Enumerable.Range(0, count)
    select new Tile(letter, score)).ToList();
\$\endgroup\$

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.