Skip to main content
added 1 character in body
Source Link

You have nicely read tothe rank count into the variable rankCount, but later, you are writing index % 13. It is not immediately clear where this number comes from. Use the variable there too.

Since these numbers never change and are well known for a 52-card deck, you could also simply define them as constants.

const int RankCount = 13;
const int SuitCount = 4;
const int DeckCount = RankCount * SuitCount;  

I usually do not use var for built in types. You do not save a lot of typing by writing int or string instead of var and it is easier to read.


The two integer operators % and / are complementary and play well together.

var rank = (Ranks)(index % RankCount);
var suit = (Suits)(index / RankCount);

Note that the integer division truncates the result. This makes the rankIteration variable superfluous.

Alternatively, you could also use two nested loops, looping over suits and ranks. Using foreach makes all the indexes and suit and rank index calculations as well as the related constants superfluous. Additionally, foreach automatically casts the values to match the type of the loop variable:

IEnumerable<Card> GetSortedCardsBySuits()
{
    foreach (Suits suit in Enum.GetValues(typeof(Suits))) {
        foreach (Ranks rank in Enum.GetValues(typeof(Ranks))) {
            yield return new Card(rank, suit);
        }
    }
}

Using List<T>.AddRange() makes the explicit loop superfluous

void CreateSortedCards()
{
    deck.AddRange(GetSortedCardsBySuits());
}

There is a minor naming problem here. The deck list is created elsewhere. Here it is only initialized. Since the list has a constructor overload List<T>(IEnumerable<T>), we could create and initialize the deck in the class constructor instead (or, if we make CreateSortedCards static, also in an initializer)

private readonly List<Card> deck;

public Standard52Deck()
{
    deck = new List<Card>(GetSortedCardsBySuits());
}

read-only fields can be initialized in an initializer or in a constructor. This also ensures that there is always an initialized card deck.


The canonical way of looping a number range is (instead of the while-loop)

for (int index = 0; index < DeckCount; index++)
{
    ...
}

It unifies and standardizes the declaration and initialization of the loop variable, the loop condition and incrementing the loop variable. Also, it scopes the loop variable locally to the loop. However, I prefer the foreach approach mentioned earlier.


Note that these are only suggestions. There are always different ways to approach a problem.

You have nicely read to rank count into the variable rankCount, but later, you are writing index % 13. It is not immediately clear where this number comes from. Use the variable there too.

Since these numbers never change and are well known for a 52-card deck, you could also simply define them as constants.

const int RankCount = 13;
const int SuitCount = 4;
const int DeckCount = RankCount * SuitCount;  

I usually do not use var for built in types. You do not save a lot of typing by writing int or string instead of var and it is easier to read.


The two integer operators % and / are complementary and play well together.

var rank = (Ranks)(index % RankCount);
var suit = (Suits)(index / RankCount);

Note that the integer division truncates the result. This makes the rankIteration variable superfluous.

Alternatively, you could also use two nested loops, looping over suits and ranks. Using foreach makes all the indexes and suit and rank index calculations as well as the related constants superfluous. Additionally, foreach automatically casts the values to match the type of the loop variable:

IEnumerable<Card> GetSortedCardsBySuits()
{
    foreach (Suits suit in Enum.GetValues(typeof(Suits))) {
        foreach (Ranks rank in Enum.GetValues(typeof(Ranks))) {
            yield return new Card(rank, suit);
        }
    }
}

Using List<T>.AddRange() makes the explicit loop superfluous

void CreateSortedCards()
{
    deck.AddRange(GetSortedCardsBySuits());
}

There is a minor naming problem here. The deck list is created elsewhere. Here it is only initialized. Since the list has a constructor overload List<T>(IEnumerable<T>), we could create and initialize the deck in the class constructor instead (or, if we make CreateSortedCards static, also in an initializer)

private readonly List<Card> deck;

public Standard52Deck()
{
    deck = new List<Card>(GetSortedCardsBySuits());
}

read-only fields can be initialized in an initializer or in a constructor. This also ensures that there is always an initialized card deck.


The canonical way of looping a number range is (instead of the while-loop)

for (int index = 0; index < DeckCount; index++)
{
    ...
}

It unifies and standardizes the declaration and initialization of the loop variable, the loop condition and incrementing the loop variable. Also, it scopes the loop variable locally to the loop. However, I prefer the foreach approach mentioned earlier.


Note that these are only suggestions. There are always different ways to approach a problem.

You have nicely read the rank count into the variable rankCount, but later, you are writing index % 13. It is not immediately clear where this number comes from. Use the variable there too.

Since these numbers never change and are well known for a 52-card deck, you could also simply define them as constants.

const int RankCount = 13;
const int SuitCount = 4;
const int DeckCount = RankCount * SuitCount;  

I usually do not use var for built in types. You do not save a lot of typing by writing int or string instead of var and it is easier to read.


The two integer operators % and / are complementary and play well together.

var rank = (Ranks)(index % RankCount);
var suit = (Suits)(index / RankCount);

Note that the integer division truncates the result. This makes the rankIteration variable superfluous.

Alternatively, you could also use two nested loops, looping over suits and ranks. Using foreach makes all the indexes and suit and rank index calculations as well as the related constants superfluous. Additionally, foreach automatically casts the values to match the type of the loop variable:

IEnumerable<Card> GetSortedCardsBySuits()
{
    foreach (Suits suit in Enum.GetValues(typeof(Suits))) {
        foreach (Ranks rank in Enum.GetValues(typeof(Ranks))) {
            yield return new Card(rank, suit);
        }
    }
}

Using List<T>.AddRange() makes the explicit loop superfluous

void CreateSortedCards()
{
    deck.AddRange(GetSortedCardsBySuits());
}

There is a minor naming problem here. The deck list is created elsewhere. Here it is only initialized. Since the list has a constructor overload List<T>(IEnumerable<T>), we could create and initialize the deck in the class constructor instead (or, if we make CreateSortedCards static, also in an initializer)

private readonly List<Card> deck;

public Standard52Deck()
{
    deck = new List<Card>(GetSortedCardsBySuits());
}

read-only fields can be initialized in an initializer or in a constructor. This also ensures that there is always an initialized card deck.


The canonical way of looping a number range is (instead of the while-loop)

for (int index = 0; index < DeckCount; index++)
{
    ...
}

It unifies and standardizes the declaration and initialization of the loop variable, the loop condition and incrementing the loop variable. Also, it scopes the loop variable locally to the loop. However, I prefer the foreach approach mentioned earlier.


Note that these are only suggestions. There are always different ways to approach a problem.

added 68 characters in body
Source Link

You have nicely read to rank count into the variable rankCount, but later, you are writing index % 13. It is not immediately clear where this number comes from. Use the variable there too.

Since these numbers never change and are well known for a 52-card deck, you could also simply define them as constants.

const int RankCount = 13;
const int SuitCount = 4;
const int DeckCount = RankCount * SuitCount;  

I usually do not use var for built in types. You do not save a lot of typing by writing int or string instead of var and it is easier to read.


The two integer operators % and / are complementary and play well together.

var rank = (Ranks)(index % RankCount);
var suit = (Suits)(index / RankCount);

Note that the integer division truncates the result. This makes the rankIteration variable superfluous.

Alternatively, you could also use two nested loops, looping over suits and ranks. Using foreach makes all the indexes and suit and rank index calculations as well as the related constants superfluous. Additionally, foreach automatically casts the values to match the type of the loop variable:

IEnumerable<Card> GetSortedCardsBySuits()
{
    foreach (Suits suit in Enum.GetValues(typeof(Suits))) {
        foreach (Ranks rank in Enum.GetValues(typeof(Ranks))) {
            yield return new Card(rank, suit);
        }
    }
}

Using List<T>.AddRange() makes the explicit loop superfluous

void CreateSortedCards()
{
    deck.AddRange(GetSortedCardsBySuits());
}

There is a minor naming problem here. The deck list is created elsewhere. Here it is only initialized. Since the list has a constructor overload List<T>(IEnumerable<T>), we could create and initialize the deck in the class constructor instead (or, if we make CreateSortedCards static, also in an initializer)

private readonly List<Card> deck;

public Standard52Deck()
{
    deck = new List<Card>(GetSortedCardsBySuits());
}

read-only fields can be initialized in an initializer or in a constructor. This also ensures that there is always an initialized card deck.


The canonical way of looping a number range is (instead of the while-loop)

for (int index = 0; index < DeckCount; index++)
{
    ...
}

It unifies and standardizes the declaration and initialization of the loop variable, the loop condition and incrementing the loop variable. Also, it scopes the loop variable locally to the loop. However, I prefer the foreach approach mentioned earlier.


Note that these are only suggestions. There are always different ways to approach a problem.

You have nicely read to rank count into the variable rankCount, but later, you are writing index % 13. It is not immediately clear where this number comes from. Use the variable there too.

Since these numbers never change and are well known for a 52-card deck, you could also simply define them as constants.

const int RankCount = 13;
const int SuitCount = 4;
const int DeckCount = RankCount * SuitCount;  

I usually do not use var for built in types. You do not save a lot of typing by writing int or string instead of var and it is easier to read.


The two integer operators % and / are complementary and play well together.

var rank = (Ranks)(index % RankCount);
var suit = (Suits)(index / RankCount);

Note that the integer division truncates the result. This makes the rankIteration variable superfluous.

Alternatively, you could also use two nested loops, looping over suits and ranks. Using foreach makes all the indexes and suit and rank index calculations as well as the related constants superfluous. Additionally, foreach automatically casts the values to match the type of the loop variable:

IEnumerable<Card> GetSortedCardsBySuits()
{
    foreach (Suits suit in Enum.GetValues(typeof(Suits))) {
        foreach (Ranks rank in Enum.GetValues(typeof(Ranks))) {
            yield return new Card(rank, suit);
        }
    }
}

Using List<T>.AddRange() makes the explicit loop superfluous

void CreateSortedCards()
{
    deck.AddRange(GetSortedCardsBySuits());
}

There is a minor naming problem here. The deck list is created elsewhere. Here it is only initialized. Since the list has a constructor overload List<T>(IEnumerable<T>), we could create and initialize the deck in the class constructor instead

private readonly List<Card> deck;

public Standard52Deck()
{
    deck = new List<Card>(GetSortedCardsBySuits());
}

read-only fields can be initialized in an initializer or in a constructor. This also ensures that there is always an initialized card deck.


The canonical way of looping a number range is (instead of the while-loop)

for (int index = 0; index < DeckCount; index++)
{
    ...
}

It unifies and standardizes the declaration and initialization of the loop variable, the loop condition and incrementing the loop variable. Also, it scopes the loop variable locally to the loop. However, I prefer the foreach approach mentioned earlier.


Note that these are only suggestions. There are always different ways to approach a problem.

You have nicely read to rank count into the variable rankCount, but later, you are writing index % 13. It is not immediately clear where this number comes from. Use the variable there too.

Since these numbers never change and are well known for a 52-card deck, you could also simply define them as constants.

const int RankCount = 13;
const int SuitCount = 4;
const int DeckCount = RankCount * SuitCount;  

I usually do not use var for built in types. You do not save a lot of typing by writing int or string instead of var and it is easier to read.


The two integer operators % and / are complementary and play well together.

var rank = (Ranks)(index % RankCount);
var suit = (Suits)(index / RankCount);

Note that the integer division truncates the result. This makes the rankIteration variable superfluous.

Alternatively, you could also use two nested loops, looping over suits and ranks. Using foreach makes all the indexes and suit and rank index calculations as well as the related constants superfluous. Additionally, foreach automatically casts the values to match the type of the loop variable:

IEnumerable<Card> GetSortedCardsBySuits()
{
    foreach (Suits suit in Enum.GetValues(typeof(Suits))) {
        foreach (Ranks rank in Enum.GetValues(typeof(Ranks))) {
            yield return new Card(rank, suit);
        }
    }
}

Using List<T>.AddRange() makes the explicit loop superfluous

void CreateSortedCards()
{
    deck.AddRange(GetSortedCardsBySuits());
}

There is a minor naming problem here. The deck list is created elsewhere. Here it is only initialized. Since the list has a constructor overload List<T>(IEnumerable<T>), we could create and initialize the deck in the class constructor instead (or, if we make CreateSortedCards static, also in an initializer)

private readonly List<Card> deck;

public Standard52Deck()
{
    deck = new List<Card>(GetSortedCardsBySuits());
}

read-only fields can be initialized in an initializer or in a constructor. This also ensures that there is always an initialized card deck.


The canonical way of looping a number range is (instead of the while-loop)

for (int index = 0; index < DeckCount; index++)
{
    ...
}

It unifies and standardizes the declaration and initialization of the loop variable, the loop condition and incrementing the loop variable. Also, it scopes the loop variable locally to the loop. However, I prefer the foreach approach mentioned earlier.


Note that these are only suggestions. There are always different ways to approach a problem.

added 538 characters in body
Source Link

You have nicely read to rank count into the variable rankCount, but later, you are writing index % 13. It is not immediately clear where this number comes from. Use the variable there too.

Since these numbers never change and are well known for a 52-card deck, you could also simply define them as constants.

const int RankCount = 13;
const int SuitCount = 4;
const int DeckCount = RankCount * SuitCount;  

I usually do not use var for built in types. You do not save a lot of typing by writing int or string instead of var and it is easier to read.


The two integer operators % and / are complementary and play well together.

var rank = (Ranks)(index % RankCount);
var suit = (Suits)(index / RankCount);

Note that the integer division truncates the result. This makes the rankIteration variable superfluous.

Alternatively, you could also use two nested loops, looping over suits and ranks. Using foreach makes all the indexes and suit and rank index calculations as well as the related constants superfluous. Additionally, foreach automatically casts the values to match the type of the loop variable:

IEnumerable<Card> GetSortedCardsBySuits()
{
    foreach (Suits suit in Enum.GetValues(typeof(Suits))) {
        foreach (Ranks rank in Enum.GetValues(typeof(Ranks))) {
            yield return new Card(rank, suit);
        }
    }
}

Using List<T>.AddRange() makes the explicit loop superfluous

void CreateSortedCards()
{
    deck.AddRange(GetSortedCardsBySuits());
}

There is a minor naming problem here. The deck list is created elsewhere. Here it is only initialized. Since the list has a constructor overload List<T>(IEnumerable<T>), we could create and initialize the deck in the class constructor instead

private readonly List<Card> deck;

public Standard52Deck()
{
    deck = new List<Card>(GetSortedCardsBySuits());
}

read-only fields can be initialized in an initializer or in a constructor. This also ensures that there is always an initialized card deck.


The canonical way of looping a number range is (instead of the while-loop)

for (int index = 0; index < DeckCount; index++)
{
    ...
}

It unifies and standardizes the declaration and initialization of the loop variable, the loop condition and incrementing the loop variable. Also, it scopes the loop variable locally to the loop. However, I prefer the foreach approach mentioned earlier.


Note that these are only suggestions. There are always different ways to approach a problem.

You have nicely read to rank count into the variable rankCount, but later, you are writing index % 13. It is not immediately clear where this number comes from. Use the variable there too.

Since these numbers never change and are well known for a 52-card deck, you could also simply define them as constants.

const int RankCount = 13;
const int SuitCount = 4;
const int DeckCount = RankCount * SuitCount;  

I usually do not use var for built in types. You do not save a lot of typing by writing int or string instead of var and it is easier to read.


The two integer operators % and / are complementary and play well together.

var rank = (Ranks)(index % RankCount);
var suit = (Suits)(index / RankCount);

Note that the integer division truncates the result. This makes the rankIteration variable superfluous.

Alternatively, you could also use two nested loops, looping over suits and ranks. Using foreach makes all the indexes and suit and rank index calculations as well as the related constants superfluous. Additionally, foreach automatically casts the values to match the type of the loop variable:

IEnumerable<Card> GetSortedCardsBySuits()
{
    foreach (Suits suit in Enum.GetValues(typeof(Suits))) {
        foreach (Ranks rank in Enum.GetValues(typeof(Ranks))) {
            yield return new Card(rank, suit);
        }
    }
}

Using List<T>.AddRange() makes the explicit loop superfluous

void CreateSortedCards()
{
    deck.AddRange(GetSortedCardsBySuits());
}

The canonical way of looping a number range is

for (int index = 0; index < DeckCount; index++)
{
    ...
}

It unifies and standardizes the declaration and initialization of the loop variable, the loop condition and incrementing the loop variable. Also, it scopes the loop variable locally to the loop.

You have nicely read to rank count into the variable rankCount, but later, you are writing index % 13. It is not immediately clear where this number comes from. Use the variable there too.

Since these numbers never change and are well known for a 52-card deck, you could also simply define them as constants.

const int RankCount = 13;
const int SuitCount = 4;
const int DeckCount = RankCount * SuitCount;  

I usually do not use var for built in types. You do not save a lot of typing by writing int or string instead of var and it is easier to read.


The two integer operators % and / are complementary and play well together.

var rank = (Ranks)(index % RankCount);
var suit = (Suits)(index / RankCount);

Note that the integer division truncates the result. This makes the rankIteration variable superfluous.

Alternatively, you could also use two nested loops, looping over suits and ranks. Using foreach makes all the indexes and suit and rank index calculations as well as the related constants superfluous. Additionally, foreach automatically casts the values to match the type of the loop variable:

IEnumerable<Card> GetSortedCardsBySuits()
{
    foreach (Suits suit in Enum.GetValues(typeof(Suits))) {
        foreach (Ranks rank in Enum.GetValues(typeof(Ranks))) {
            yield return new Card(rank, suit);
        }
    }
}

Using List<T>.AddRange() makes the explicit loop superfluous

void CreateSortedCards()
{
    deck.AddRange(GetSortedCardsBySuits());
}

There is a minor naming problem here. The deck list is created elsewhere. Here it is only initialized. Since the list has a constructor overload List<T>(IEnumerable<T>), we could create and initialize the deck in the class constructor instead

private readonly List<Card> deck;

public Standard52Deck()
{
    deck = new List<Card>(GetSortedCardsBySuits());
}

read-only fields can be initialized in an initializer or in a constructor. This also ensures that there is always an initialized card deck.


The canonical way of looping a number range is (instead of the while-loop)

for (int index = 0; index < DeckCount; index++)
{
    ...
}

It unifies and standardizes the declaration and initialization of the loop variable, the loop condition and incrementing the loop variable. Also, it scopes the loop variable locally to the loop. However, I prefer the foreach approach mentioned earlier.


Note that these are only suggestions. There are always different ways to approach a problem.

added 538 characters in body
Source Link
Loading
deleted 2 characters in body
Source Link
Loading
Source Link
Loading