Skip to content

Update Deck.cs#9

Open
KattaRoopa wants to merge 1 commit intoblackbaud:mainfrom
KattaRoopa:patch-1
Open

Update Deck.cs#9
KattaRoopa wants to merge 1 commit intoblackbaud:mainfrom
KattaRoopa:patch-1

Conversation

@KattaRoopa
Copy link

@KattaRoopa KattaRoopa commented Feb 13, 2026

Summary by CodeRabbit

New Features

  • Deck shuffling capability with support for multiple shuffle iterations
  • Returns a new shuffled deck instance preserving the original
  • Input validation ensures shuffle count must be positive

@coderabbitai
Copy link

coderabbitai bot commented Feb 13, 2026

📝 Walkthrough

Walkthrough

A new public Shuffle(int times) method was added to the Deck class. This method validates the input parameter, shuffles the deck's cards multiple times using a Fisher-Yates-like algorithm, and returns a new Deck instance containing the shuffled cards.

Changes

Cohort / File(s) Summary
Deck Shuffle Implementation
CSharp/Blackbaud.Interview.Cards/Deck.cs
Added Shuffle(int times) method with input validation, card-list conversion, Fisher-Yates-style shuffle logic with configurable iterations, and new Deck return.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A shuffle here, a shuffle there,
With Fisher-Yates beyond compare!
New methods dance in shuffled grace,
One Deck spins many times and place—
Validation guards the way we sway,
Fresh randomness at every play! 🎴✨

🚥 Pre-merge checks | ✅ 3 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Update Deck.cs' is vague and does not convey the specific change being made. It merely indicates a file name without describing the meaningful addition of the shuffle functionality. Revise the title to be more descriptive, such as 'Add Shuffle method to Deck class' or 'Implement card shuffling functionality in Deck', to clearly communicate the primary change to reviewers.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into main

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
CSharp/Blackbaud.Interview.Cards/Deck.cs (1)

52-67: ⚠️ Potential issue | 🔴 Critical

Move Shuffle out of NextCard to fix a compile error.

The Shuffle method is currently nested inside NextCard() with a public access modifier, which violates C# rules: access modifiers are not allowed on local functions. This code will not compile. Close the NextCard() method with a closing brace after the else block (line 60), then declare Shuffle as a class member method.

🐛 Proposed fix
         else
         {
             return null;
         }
-         /// <summary>
-        /// Shuffles the deck a given number of times and returns a new shuffled deck.
-        /// </summary>
-        public Deck Shuffle(int times)
-        {
+    }
+
+    /// <summary>
+    /// Shuffles the deck a given number of times and returns a new shuffled deck.
+    /// </summary>
+    public Deck Shuffle(int times)
+    {
🧹 Nitpick comments (2)
CSharp/Blackbaud.Interview.Cards/Deck.cs (2)

68-69: Prefer an out-of-range exception with parameter name for invalid times.

ArgumentOutOfRangeException(nameof(times), ...) yields clearer diagnostics for callers and is the idiomatic choice for this check.

♻️ Proposed tweak
-            if (times <= 0)
-                throw new ArgumentException("Number of shuffles must be greater than zero.");
+            if (times <= 0)
+                throw new ArgumentOutOfRangeException(nameof(times), "Number of shuffles must be greater than zero.");

73-79: Use Random.Shared instead of per-call new Random().

Creating a new Random instance per call causes identical shuffle sequences when Shuffle() is invoked in quick succession, since Random seeds from the system clock. Your project targets .NET 8.0, so Random.Shared is available and provides a thread-safe shared RNG instance that maintains state across calls.

♻️ Proposed change
-            var random = new Random();
+            var random = Random.Shared;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Comments