7
\$\begingroup\$

I wrote a small program to address some assignment tasks related to making a game called 'Dice on a Yacht'. I sent the code to be reviewed by a company I was applying for, and I have not gotten the job (as I had expected shortly after submitting code and thinking over it further). I realize that I need to start preparing more for my next job and so I have started reading material related to algorithm design and analysis (which I suspect may be my strongest weak point) and as well as design patterns and language specific material (mostly C# but also C++).

This is written in Unity C# (the job was for a mobile Unity position). I'm just curious what issues you can find and what recommendations there are regarding the code or anything else.

For more info on the assignment itself: I was tasked with writing two functions getScore() and getSuggestions(). getScore should retrieve the appropriate score for an array that is passed in, and the final score depends on what category is also passed in (i.e small straight, four of a kind, twos, etc. Check out Wikipedia for scoring rules). I wrote a function getDifferences which simply gets the differences between elements and adds them into an array that is then used by three other non-numerical categories for calculating score. Not too much code was expected to be written, but I figured the getDifferences function was a nice way for dealing with the special categories in a reusable manner. The getSuggestions function is one I spent less time on, and is definitely one that would need more improvement.

using UnityEngine;
using System.Collections;

public class AssessmentExam: MonoBehaviour {


    public enum Category{
        Ones,
        Twos,
        Threes,
        Fours,
        Fives,
        Sixes,
        Sevens,
        Eights,
        ThreeOfAKind,
        FourOfAKind,
        FullHouse,
        SmallStraight,
        LargeStraght,
        AllDifferent,
        Chance,
        AllSame
    }

    // Use this for initialization
    void Start () {

        //Example Code to test out the functions in Unity

        //int[] arr = {2,2,2,2,7};//{0,2,4,6,3};
        //Debug.Log (getScore(Category.SmallStraight,arr));
        //Debug.Log (getSuggestions(arr));
    }

    // Update is called once per frame
    void Update () {

    }

    public int getScore(Category category, int[] array){

        int intCategory = (int)category + 1; //Casting enum to int to seperate category types. By default enums start with index 0. 
        int score = 0;  int count = 0;
        int[] diffArray;

        if(intCategory < 9){  //This handles categories from 'Ones' to 'Eights'.
            for(int i = 0; i < array.Length; i++){
                if(array[i] == intCategory){
                    score += intCategory;
                }
            }
        }
        else{ //Handles the other categories (SmallStraight, AllDifferent, AllSame);


            switch(category){
            case Category.SmallStraight:

                //If there are four die in sequence, the score is 30. Otherwise, the score is 0. 

                diffArray = getDifferences(array);  //getDifferences returns an array of elements that each represent the difference between element pairs.                 
                count = 0;

                for(int i = 0; i < diffArray.Length; i++){

                    if((diffArray[i] == 1)){ //Elements pairs that differ by one are considered part of a sequence
                        count++;

                    }else{
                        count = 0; //Reset the count if a sequence is interrupted
                    }

                    if(count == 3){ //Assign score to 30 if all 4 die are in sequence (loop
                        score = 30;
                    }else if(count > 3){ // Prevents largeStraights from scoring as a smallStraight
                        score = 0;
                    }
            }
            break;

            case Category.AllDifferent:

                //If all elements in the passed in array are unique, the score is 40. Otherwise, the score is 0.

                System.Array.Sort(array); //Sort the elements so that duplicate elements evaluate to 0 when the differences are calculated.
                diffArray = getDifferences(array); // Reusing the 'getDifference' function used in the previous category.

                count = 0;

                for(int i = 0; i < diffArray.Length; i++){

                    if(diffArray[i] != 0){ // If there is a difference element of 0, there are duplicate elements in the array. Don't count these.
                        count++;
                    }
                }

                if(count == diffArray.Length){ //If count equals the length of the array, then all elements are unique.
                    score = 40;
                }
            break;

            case Category.AllSame:

                //If all elements are the same, return 50. Otherwise, return 0. 

                diffArray = getDifferences(array); //Get the differences
                count = 0;

                for(int i = 0; i < diffArray.Length; i++){

                    if(diffArray[i] == 0){ //If the difference between an element pair is 0, increase count by one.
                        count++;
                    }
                }

                if(count == diffArray.Length){ //if the count equals to the length of the array, then all elements are the same.
                    score = 50;
                }
            break;
        }
    }
        return score;
}

    public Category getSuggestions(int[] array){

        //Testing out available enumarators that would work with my example code.
        // This was a fast way of testing out the getSuggestions function. It can be improved to evaluate the actual best 'number category' for a given array.

        //Categories are added into an array from best scoring to least to quickly evaluage array without regards to array specifics. ({2,2,2,2,7} would be 8 for Twos, instead of 7 when evaluated with Sevens.
        //In that array example, the algorithm would return 'Sevens' since it is the category of highest priority that does not return 0. 

        Category[] categoryTypes = {Category.AllSame, Category.AllDifferent, Category.SmallStraight, Category.Eights, Category.Sevens, Category.Sixes, Category.Fives,
        Category.Fours, Category.Threes, Category.Twos, Category.Ones};

        foreach(Category category in categoryTypes){

            int score = getScore(category, array);

            if(score > 0){
                return category;
            }
        }

        return Category.Ones;
    }

    private int[] getDifferences(int[] arr){ //Function that is reused by the categories above. Can be used for fullHouse and a few other categories.

        int[] differenceArray = new int[4];

        for (int i = 1; i < arr.Length; i++){

            int difference = arr[i] - arr[i-1];
            differenceArray[i-1] = difference;
        }

        return differenceArray;
    }
}
\$\endgroup\$
3
\$\begingroup\$

I agree that the getSuggestions method has significant room for improvement. The major thing I notice is the method name is plural, yet the method only returns a single item. I would return a collection of suggestions and order them by score or something like that.

Why doesn't your code handle all the categories?

Method names should be UpperCamelCase. That is, GetScore, GetSuggestions, and GetDifferences.

The spacing is messed up in many places. For example here:

        }
    }
        return score;
}

The braces should all be indented by four more spaces, and return score; should not be indented more than the above brace.

array isn't a very descriptive name. I'd go with something like DieNumbers.

Most of your loops can be replaced with LINQ, which is more concise, safer, and easier to read, in my opinion. For example:

for(int i = 0; i < array.Length; i++){
    if(array[i] == intCategory){
        score += intCategory;
    }
}

Can be replaced with:

score = array.Where(x => x == intCategory).Sum();
\$\endgroup\$
  • \$\begingroup\$ Thank you, these are all good suggestions. The assignment required the use of only 3 of the special categories. The rest can still be implemented in a similar fashion although I will have to restructure a bit and use some language features to make things cleaner. :) Thanks \$\endgroup\$ – GelatinFox Sep 23 '15 at 20:13
1
\$\begingroup\$

I sent the code to be reviewed by a company I was applying for

I'd want to see some polish.

  • XML comments on the methods. And the comments do not merely re-state the code.
    • //getDifferences returns an array ... - this is obvious from the method signatures.
    • getDifferences(array); //Get the differences - I call this "comment stuttering"
  • "domain oriented" variable names
    • "array", "array", "array" all over the place! What the heck is that? Could it be your dice?
  • "domain oriented" logic. You examine count a lot for example. Make properties or methods to wrap that. So if (isLargeStraight) instead of if(count == 3)
  • Layered abstractions, even as little code as this is.
    • getScore() must be a 50 line if statement. I cannot read what this does domain wise. if(isOneToEight) ScoreOneToEight(); else ScoreCombinations();
    • Each switch case could be a single, clearly named method call.
\$\endgroup\$
1
\$\begingroup\$

In GetSuggestions(),
You can get an array of all the values of an Enum using:
Enum.GetValues(typeof(Category))
It returns an Array, with which you can do:

foreach (Category category in Enum.GetValues(typeof(Category))
{
    // do whatever code you need that addresses the value of `category`
}

Also, your naming of variables is very poor. I'll pass what knowledge and rules I've gotten.
Your variables need to be named in a way that you must always know the purpose of that variable solely based on the name of the variable, like stated above in Risky Martin's answer.

With whatever has to do with indentation and style,
I would suggest getting ReSharper - It's a life-changer.

This is a project of mine, you can look at how the code is styled and written, if you want some examples


Another suggestion, looking at the switch-case you have, when you have such different ways, you should have the code of each case refactored into a function, for example:

public int getScore(Category category, int[] array){

        int intCategory = (int)category + 1; //Casting enum to int to seperate category types. By default enums start with index 0. 
        int score = 0;  int count = 0;
        int[] diffArray;

        if(intCategory < 9){  //This handles categories from 'Ones' to 'Eights'.
            for(int i = 0; i < array.Length; i++){
                if(array[i] == intCategory){
                    score += intCategory;
                }
            }
        }
        else{ //Handles the other categories (SmallStraight, AllDifferent, AllSame);


            switch(category){
            case Category.SmallStraight:

                //If there are four die in sequence, the score is 30. Otherwise, the score is 0. 

                diffArray = getDifferences(array);  //getDifferences returns an array of elements that each represent the difference between element pairs.                 
                count = 0;

                for(int i = 0; i < diffArray.Length; i++){

                    if((diffArray[i] == 1)){ //Elements pairs that differ by one are considered part of a sequence
                        count++;

                    }else{
                        count = 0; //Reset the count if a sequence is interrupted
                    }

                    if(count == 3){ //Assign score to 30 if all 4 die are in sequence (loop
                        score = 30;
                    }else if(count > 3){ // Prevents largeStraights from scoring as a smallStraight
                        score = 0;
                    }
            }
            break;

            case Category.AllDifferent:

                //If all elements in the passed in array are unique, the score is 40. Otherwise, the score is 0.

                System.Array.Sort(array); //Sort the elements so that duplicate elements evaluate to 0 when the differences are calculated.
                diffArray = getDifferences(array); // Reusing the 'getDifference' function used in the previous category.

                count = 0;

                for(int i = 0; i < diffArray.Length; i++){

                    if(diffArray[i] != 0){ // If there is a difference element of 0, there are duplicate elements in the array. Don't count these.
                        count++;
                    }
                }

                if(count == diffArray.Length){ //If count equals the length of the array, then all elements are unique.
                    score = 40;
                }
            break;

            case Category.AllSame:

                //If all elements are the same, return 50. Otherwise, return 0. 

                diffArray = getDifferences(array); //Get the differences
                count = 0;

                for(int i = 0; i < diffArray.Length; i++){

                    if(diffArray[i] == 0){ //If the difference between an element pair is 0, increase count by one.
                        count++;
                    }
                }

                if(count == diffArray.Length){ //if the count equals to the length of the array, then all elements are the same.
                    score = 50;
                }
            break;
        }
    }
        return score;
}

Can be turned into:

public int getScore(Category category, int[] array){

        int intCategory = (int)category + 1; //Casting enum to int to seperate category types. By default enums start with index 0. 
        int score = 0;  int count = 0;
        int[] diffArray;

        if(intCategory < 9){  //This handles categories from 'Ones' to 'Eights'.
            for(int i = 0; i < array.Length; i++){
                if(array[i] == intCategory){
                    score += intCategory;
                }
            }
        }
        else{ //Handles the other categories (SmallStraight, AllDifferent, AllSame);

            switch(category){
            case Category.SmallStraight:
                score = HandleSmallStrightCase(array);
            break;
            /* Handle all other cases similarly */
        }
    }

    return score;
}

private int HandleSmallStrightCase(int[] array)
{
//If there are four die in sequence, the score is 30. Otherwise, the score is 0. 

                diffArray = getDifferences(array);  //getDifferences returns an array of elements that each represent the difference between element pairs.                 
                count = 0;

                for(int i = 0; i < diffArray.Length; i++){

                    if((diffArray[i] == 1)){ //Elements pairs that differ by one are considered part of a sequence
                        count++;

                    }else{
                        count = 0; //Reset the count if a sequence is interrupted
                    }

                    if(count == 3){ //Assign score to 30 if all 4 die are in sequence (loop
                        score = 30;
                    }else if(count > 3){ // Prevents largeStraights from scoring as a smallStraight
                        score = 0;
                    }
            }
     return score;
}

That is just an example, fixing all indentation and commenting problems is a matter of doing the black work... If you have a way to handle each case, move every case to a method, and call the method in the switch case.
That way your GetScore() method looks nicer, and you have your handling methods seperated.

\$\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.