Code Review - Olivia Davey

What you did well:

Really great code overall! It is well-structured, organised, consistent naming conventions are used, and I didn't encounter any errors - the code seemed to run fine, with every type of shuffle called under the Testing class working correctly.

I liked how you added two extra methods here in the Testing class, it makes the code and its functionality even simpler to understand.

https://github.com/euanl26/CMP1903-Assessment-1/blob/00da4877232c2312e9f29657d98664d633757108/CMP1903M%20A01%202223/Testing.cs#L8-L27

It's also good that you implemented encapsulation with condition validation for the suit and value properties, and how you used exception handling to ensure an incomplete pack doesn't enter the shuffle function.

https://github.com/euanl26/CMP1903-Assessment-1/blob/00da4877232c2312e9f29657d98664d633757108/CMP1903M%20A01%202223/Card.cs#L18-L32

Ensuring that all errors caused by incorrect values are handled appropriately helps to make the program more secure. The exceptions are accompanied with clear error messages, so the user is able to understand what has gone wrong.

https://github.com/euanl26/CMP1903-Assessment-1/blob/00da4877232c2312e9f29657d98664d633757108/CMP1903M%20A01%202223/Pack.cs#L33-L36

Recommendation 1: Could a for loop could have been used here, to condense the code down into only a few lines?

https://github.com/euanl26/CMP1903-Assessment-1/blob/00da4877232c2312e9f29657d98664d633757108/CMP1903M%20A01%202223/Program.cs#L11-L25

Each shuffle type being passed into oneCardDeal and multiCardDeal functions could have been iterated over. As an example:

for (int i = 1; i < 5; i++)
{
         test.oneCardDeal(i);
         test.multiCardDeal(i, 5);
}

Recommendation 2: Rather than using if-else statements here, consider using a switch() statement because this will make your code more efficient and easier to read (especially since you are comparing the value of a single variable, in this case typeOfShuffle).

https://github.com/euanl26/CMP1903-Assessment-1/blob/00da4877232c2312e9f29657d98664d633757108/CMP1903M%20A01%202223/Pack.cs#L40-L89

As an example:

switch(typeOfShuffle)
{
        case 1:
        //Code for Fisher-Yates shuffle here
            break;

       case 2:
       //Code for Riffle shuffle here
           break;

       case 3:
       //No shuffle
           break;

       case 4:
       //Console output for invalid shuffle
           break;
}

Recommendation 3: Perhaps adding some more variety when calling the multiCardDeal method from the Testing class may benefit the program further. You could try dealing different numbers of cards as opposed to just 5 every time, so you can check whether different values for the amount variable all work as intended.

https://github.com/euanl26/CMP1903-Assessment-1/blob/00da4877232c2312e9f29657d98664d633757108/CMP1903M%20A01%202223/Program.cs#L20-L23

As an example:

test.multiCardDeal(1, 5);
test.multiCardDeal(2, 10);
test.multiCardDeal(3, 12);
test.multiCardDeal(4, 15);

Most of your code is commented really thoroughly, especially in the Pack() class. One example is the below where you clearly explain how the Riffle shuffle works and is implemented within your code. However, I would consider adding a few additional comments in the Program() class, just to clearly explain what exactly each method is testing (which type of shuffle, etc.).

https://github.com/euanl26/CMP1903-Assessment-1/blob/00da4877232c2312e9f29657d98664d633757108/CMP1903M%20A01%202223/Pack.cs#L56

Well done though!

euanl26 / CMP1903-Assessment-1

Code Review - Olivia Davey #2