pop() undefined type error

[real zinc]

  name: string;
  id: number;
  stats: number;
  energy: number;
};``` 

i have this at the top of my file

and a useState call `  const [handCards, setHandCards] = useState<CardType[]>([]);`

```  const dealHand = () => {
    const newDeckCards = [...deckCards];
    const newHandCards = [];
    // deal 5 cards
    for (let i = 0; i < 5; i++) {
      // if no more cards are in drawDeck, transfer all discardCards to drawDeck
      if (newDeckCards.length === 0) {
        console.log("shuffling discard pile into draw pile");
        newDeckCards.push(...discardCards);
        setDiscardCards([]);
        setDeckCards(newDeckCards);
      }
      // take cards from deck and put them into our hand
      newHandCards.push(newDeckCards.pop());
    }

    setDeckCards(newDeckCards);
    setHandCards(newHandCards); // i get an error here
  };```

`Type 'undefined' is not assignable to type 'CardType'.`

i know this is because pop() can potentially return undefined if the array is empty. how do i change my code to stop the error?

[placid dove]

for (let i = 0; i < 5; i++) {
    const card = newDeckCards.pop()

    if (!card) {
        console.log('shuffling discard pile into draw pile')
        newDeckCards.push(...discardCards)
        setDiscardCards([])
        setDeckCards(newDeckCards)
    } else {
        newHandCards.push(card)
    }
}

Your original code also has logical issue, after checking newDeckCards to be empty, you loop body continues to execute and still pushes an undefined card into hands.

[neon lynx]

if you're popping the new card and pushing the discarded cards, doesnt that mean you'll be drawing discarded cards?

[real zinc]

i don't think this is true

the app is working as intended with this code

newDeckCards gets populated with the content of the discard pile

newDeckCards.push(...discardCards);

[real zinc]

yup

thats how deckbuilders like Slay the Spire work

when you run out of cards to draw, your discard pile gets shuffled back into your draw pile

[placid dove]

I mean take a closer look at your original code.

After the entire if (newDeckCards.length === 0) {} block, it continues to execute, and newHandCards.push(newDeckCards.pop()) will just push a undefined into newHandCards.

It may not affect how your app works, but it is a logical error.

[real zinc]

its not undefined though?

by that point there is a card object in newDeckCards

its intentional

because otherwise only 4 cards would get dealt

a hand is 5 cards

[placid dove]

Ah I misread.

[real zinc]

on each card being dealt it checks to see if deckCard is empty, so it needs to also deal the card on the iteration where it is empty and repopulates deckCards from the discard pile

i am concerned though that it might be undefined at some point because of the life cycle not working as expected

but so far theres no undefined getting pushed in

[pallid mesa]

typescript doesn't know that

[real zinc]

well yea exactly

[placid dove]

That's why you shouldn't rely on checking newDeckCards.length to assert if it's undefined

[placid dove]

Just do this, check directly what you are going to push to see if it is undefined or not.

[pallid mesa]

typescript signature for pop is T |undefined

[real zinc]

yes im aware

[pallid mesa]

it's not smart enough to know about other checks you have done or invariants of your program

[real zinc]

right

so whats the solution

[real zinc]

pretty sure its gonna give the same error for this code because of what michaels saying

[placid dove]

No

if (!card) already checks to see if it's undefined or not.

[pallid mesa]

1. as T
2. assign the value to a variable and use control flow to check for undefined
3. same as 2) but use an assertion function to assert that it's not undefined

[real zinc]

whats as T?

can't i just do pop()!

[pallid mesa]

shorthand way of saying whatever your type is

that is also an option

[real zinc]

yea thats what i ended up doing

    const newDeckCards = [...deckCards];
    const newHandCards = [];
    // deal 5 cards
    for (let i = 0; i < 5; i++) {
      // if no more cards are in drawDeck, transfer all discardCards to drawDeck
      if (newDeckCards.length === 0) {
        console.log("shuffling discard pile into draw pile");
        newDeckCards.push(...discardCards);
        setDiscardCards([]);
        setDeckCards(newDeckCards);
      }
      // take cards from deck and put them into our hand
      if (newDeckCards.length > 0) newHandCards.push(newDeckCards.pop());
    }

    setDeckCards(newDeckCards);
    setHandCards(newHandCards as CardType[]);
  };```

just did as CardType[]

and i make sure to check if newDeckCards has something inside before popping

@placid dove thats good enough right?

[placid dove]

You don't need the last as CardType[]

[pallid mesa]

I generally like to avoid ! and would prefer an assertion that would error in future in case the invariants of my program changed

[placid dove]

Declare it upfront instead:

const newHandCards: CardType[] = [];

[pallid mesa]

that is a personal preference for me

[real zinc]

that doesnt work @placid dove

still get an error on the pop()

  Type 'undefined' is not assignable to type 'CardType'.ts(2```

because of what michael said

[pallid mesa]

one moment I'll write something

[placid dove]

I literally gave you the answer like the first message

const card = newDeckCards.pop()
if (card) newHandCards.push(card)

Instead of:

• Using array.length > 0 to assert array.pop() is not undefined.
  You should instead:
• Just pop and check the result is undefined or not.

[real zinc]

    const newDeckCards = [...deckCards];
    const newHandCards: CardType[] = [];
    // deal 5 cards
    for (let i = 0; i < 5; i++) {
      const card = newDeckCards.pop();
      // if no more cards are in drawDeck, transfer all discardCards to drawDeck
      if (!card) {
        console.log("shuffling discard pile into draw pile");
        newDeckCards.push(...discardCards);
        setDiscardCards([]);
        setDeckCards(newDeckCards);
      }
      // take cards from deck and put them into our hand
      if (card) newHandCards.push(newDeckCards.pop());
    }

    setDeckCards(newDeckCards);
    setHandCards(newHandCards);
  };```

[placid dove]

  const dealHand = () => {
    const newDeckCards = [...deckCards];
    const newHandCards: CardType[] = [];
    // deal 5 cards
    for (let i = 0; i < 5; i++) {
      // if no more cards are in drawDeck, transfer all discardCards to drawDeck
      if (!card) {
        console.log("shuffling discard pile into draw pile");
        newDeckCards.push(...discardCards);
        setDiscardCards([]);
        setDeckCards(newDeckCards);
      }
      // take cards from deck and put them into our hand
      const card = newDeckCards.pop();
      if (card) newHandCards.push(card);
    }

    setDeckCards(newDeckCards);
    setHandCards(newHandCards);
  };

[real zinc]

cards not defined

before the if

[placid dove]

Yes

Ah

Just change that back to your original check then.

[real zinc]

thats exactly what im ding

doing i just copy pasted that code

[placid dove]

  const dealHand = () => {
    const newDeckCards = [...deckCards];
    const newHandCards: CardType[] = [];
    // deal 5 cards
    for (let i = 0; i < 5; i++) {
      // if no more cards are in drawDeck, transfer all discardCards to drawDeck
      if (newDeckCards.length === 0) {
        console.log("shuffling discard pile into draw pile");
        newDeckCards.push(...discardCards);
        setDiscardCards([]);
        setDeckCards(newDeckCards);
      }
      // take cards from deck and put them into our hand
      const card = newDeckCards.pop();
      if (card) newHandCards.push(card);
    }

    setDeckCards(newDeckCards);
    setHandCards(newHandCards);
  };

[real zinc]

yea that still errors

wait

oh sorry

ahh ok

ty

[upbeat flower]

const arrA: string[] = [];

const arrB: string[] = ["foo"];

arrA.push(arrB.pop()) // :(
//        ^^^^^^^^^^
// Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
//   Type 'undefined' is not assignable to type 'string'.

function assert(x: boolean, msg: string): asserts x {
  if (!x) {
    throw new Error(msg)
  }
}

const arrC: string[] = ["foo"];

const arrD: string[] = [];

const value = arrC.pop()
//    ^? - const value: string | undefined

assert(value !== undefined, "expected string") // will error if in future your invariants change 

arrD.push(value) // :)
//          ^? - const value: string

[pallid mesa]

there ^^^

[placid dove]

For future reference, this is the TS way to do things.

[pallid mesa]

that is how I would do it

have a assertion function that asserts some condition

[real zinc]

burritos way is cleaner

for this situation

its not necessary to do an assertion

i think

[pallid mesa]

benefit of my solution is that you will get a runtime error if your assumption later provides to be wrong

[placid dove]

It's the same, michael's has an extra protection.

[pallid mesa]

whereas with burrito it might silently fail

[placid dove]

Tbh I wouldn't write your code that way but eh, if it works it works.

[pallid mesa]

you will have a bug but won't know it

[real zinc]

how should i write it

[real zinc]

cant i just do a if (!card) console.error

[pallid mesa]

generally you just want to error out when an assumption like this is false

there is no point in continuing your program

[real zinc]

so cant i just throw an error

[pallid mesa]

      const card = newDeckCards.pop();
      assert(card !== undefined, "there should always be a card")
      newHandCards.push(card);

why not?

[placid dove]

I don't know exactly what your program is trying to do, but it seems to be "transfer 5 cards from deck to hands; if there aren't enough deck cards, transfer discarded cards into deck"

[real zinc]

have you ever played Slay the Spire

its just a clone of that

[placid dove]

No.

[real zinc]

but yea thats the behavior

[placid dove]

So why not just do the 5 card check upfront?

Why do the check in every iteration of the loop?

[real zinc]

because thats not how STS works

it doesnt deal 5 cards at once

it deals one at a time until the deck is empty

then replenishes the deck

[placid dove]

It doesn't matter

[real zinc]

why not

[real zinc]

i guess i dont get how this is superior to just doing if (!card) throw error

[pallid mesa]

console.error doesn't throw

[placid dove]

There are 3 cards in deck, 10 cards in discarded.
These two methods produce the exact same result:

A:

• Deal 3 cards from deck.
• Move discarded to deck.
• Deal 2 cards.

B:

• Move discarded to deck.
• Deal 5 cards.

You can run it through your head.

[real zinc]

is there no throw command?

[pallid mesa]

well my assertion function does throw

and has the benefit that you don't need to nest things with control flow in your ordinary code

its just an assertion and then you move on

console.error is just logging with a stack trace

[real zinc]

in B, you'd have to place the cards under the current deckCards

[pallid mesa]

but maybe in a video game you want to just let the program continue for as long as it can

even if the state of it is completely unknown once an assumption is wrong

[placid dove]

Sure

[pallid mesa]

log the error

hope for the best

[placid dove]

Concat array in whatever order you need.

[real zinc]

the thing is thats just not how STS works

you can see the draw pile replenish

and the number of cards leaving the draw pile one by one

until i gets to 0 then it replenishes

its their game design,i think there is probably a mechanical reason for it

due to relic mechanics

theres a lot of items and cards that affect draw mechanics

and discard / exhaust

[placid dove]

It has nothing to do with the game.

It's impossible to observe the difference between A and B.

Because your code runs in one single step, and the result of A and B are identical.

[real zinc]

@placid dove ok so the reason i think is because there is a maximum hand size of 10, i havnt written in that logic yet

but lets say the player has 2 cards in drawdeck

and 9 cards in hand

if discard gets shuffled in when you say it should

itll incorrectly empty discard into draw despite max card limit being reached after one card being dealt

not that you could've known that

[placid dove]

That's the intermediate state, it doesn't matter, only state before and after your function runs is observable.

[real zinc]

what?

no im saying the players turn would start with only 1 card being dealt

and that would be correct

and the correct state would be the discard not having been shuffled into draw

if their max hand size was reached after only dealing 1 card

i just havnt written in that logic

for max hand size

[placid dove]

Okay yeah then that would matter.

On a side note, your code also doesn't deal with "what happens even deck + discarded combined still has less than 5 cards"

[real zinc]

will it recursively loop currently

[placid dove]

But anyways, your original question is solved, I'm sure you can figure out the rest yourself 😄

[real zinc]

sure, thank you!

yea i havnt made a card removal mechanic yet but that will have to be addressed

[placid dove]

(Actually, even given the constraint that you can only have maximum 10 cards at once, you can still write code that doesn't involve a loop)

(If player already has 8 cards in hand, then you simply only "draw" 2 cards rather than drawing all 5)

[real zinc]

but discard would be shuffled into draw at that point

which would be incorrect

[placid dove]

No

You check how many you draw before you move discard.

[real zinc]

that logic is hurting my brain

if player has 9 cards in hand and 2 in draw deck

check if ...

what

if 10 - numCardsInHand > 5 ?

also the code does currently all wrk in one step, but ideally it would show each card being dealt one by one

as an animation

[placid dove]

1. Start with drawCount = 5.
2. Update drawCount based on how many player already has in hand and max cards player can have in hand. Eg if player already has 8 cards in hand, and player can at most have 10, then update drawCount to 2.
3. Check if deck has enough for drawCount. If deck doesn't have enough, move discarded into deck.
4. Simply chop the deck for drawCount and move to player's hands.

If you are going to eventually make it have animation between steps then you should just start writing it that way.

Refactoring later is not going to be fun.

[real zinc]

hmm

i thought surface level stuff like that should go last

i guess theres no animation library that will just take my array and deal them into my hand

[placid dove]

That's certainly not a surface level refactor.

You need to completely rip apart your for loop, for example.

[real zinc]

how would i even do that

are there libraries that will just take a simple array and animate it for me

[placid dove]

One way to do it is to have an action queue, and send 5 "draw 1 card" actions into the queue, and the queue simply runs sequentially one by one.

[real zinc]

and the queue does what every step

trigger a rerender?

[placid dove]

It dequeues an action and execute it.

[real zinc]

i actually found a repo of someone who did a STS clone and they had a action queue now that i think about it

[placid dove]

But yeah you should probably start looking into that, it's not going to be a simple refactor.

[real zinc]

hmmmmmmmMMMM

that sounds like too much work

im just going to ignore it lol

im just making this for fun

and a portfolio project

its fine if it doesn't function exactly like STS

but idk maybe ill do it cause it sounds interesting too

[real zinc]

but what i don't get is how that works with react

oh maybe i just need to setState on every iteration of the for loop lol

instead of pushing into the final array

why not that

anyway thanks

i guess i dont see how an action queue is different from just setting state on every iteration

[placid dove]

I don't know enough about React to comment on that, good luck on your project though 😄

[real zinc]

ahh ok

cause everytime you setState in react it triggers a rerender of the dom

then i maybe can just hook a animation to that call

anyway you've given me a lot to consider