2

My array is being loaded and is printing the cards out as planned (in the order they appear in the file). When I try to cycle back through the arraylist in a separate method to check whether or not the data is there, it only prints the last object rather than each of them. Can anybody tell me why?

The load method

public class
    TestFrame {

//VARIABLES
private static Deck deck;
private static Card card;
private static Scanner scan;
private final static String fileName = "cards.txt";
static ArrayList<Card> cards = new ArrayList<>();

private static void Load(){

    deck = new Deck();
    card = new Card();

    // Load in the card file so that we can work with the data from cards.txt internally rather than from the file constantly.

    try(FileReader fr = new FileReader(fileName);
            BufferedReader br = new BufferedReader(fr);
            Scanner infile = new Scanner(br)){

        int numOfCards = infile.nextInt();
        infile.nextLine(); // Do we need this? Yes we do. Illuminati confirmed.
        for(int i=0; i < numOfCards; i++){
            String value = infile.nextLine();
            String suit = infile.nextLine();
            Card newCard = new Card(value, suit);
            card.addCard(newCard);
            System.out.print(newCard.getValue());
            System.out.print(newCard.getSuit());
            System.out.println(" ");
            //Print out the object before cycling through again so we can see if it's working
            //We can use this to add then cards to the shuffle array at a later date
        }

    }

Which prints out this when ran:

ah 
2h 
3h 
4h 
5h 
6h 
7h 
8h 

etc etc. This is the order they're in the .txt file.

I then use these methods to display all of the cards to make sure I can manipulate the data elsewhere

private static void displayAllCards(){
    Card[] cards = Card.getAll();
    for(Card c : cards){
        System.out.print(Card.getValue());
        System.out.print(Card.getSuit());
        System.out.println(" ");
    }
}

and the getAll() method

public static Card[] getAll(){
    Card[] brb = new Card[cards.size()];
    int tempCount = -1;
    for(Card c : cards){
        tempCount++;
        brb[tempCount] = c;
    }
    return brb;
}

When getAll() is ran, it only prints out "ks" (king of spades) which is the last card in the .txt file. Can anybody tell me why this is happening?

Thanks

EDIT: CARD CLASS

package uk.ac.aber.dcs.cs12320.cards;

import java.util.ArrayList;


public class Card {

protected static String value;
protected static String suit;
static ArrayList<Card> cardsList = new ArrayList<>();

public Card(String v, String s){
    this.value = v;
    this.suit = s;
}

public Card() {

}

public static Card[] getAll(){
    Card[] brb = new Card[cardsList.size()];
    int tempCount = -1;
    for(Card c : cardsList){
        tempCount++;
        brb[tempCount] = c;
    }
    return brb;
}

public static void deleteAll(){
    cardsList.clear();
}

public static String getValue() {
    return value;
}

public void setValue(String value) {
    Deck.value = value;
}

public static String getSuit() {
    return suit;
}

public void setSuit(String suit) {
    Deck.suit = suit;
}

public void addCard(Card card){
    cardsList.add(card);
}

}

James Gould
  • 4,492
  • 2
  • 27
  • 50
  • when do you populate your cards arrayList? – jean May 04 '15 at 18:05
  • Show the `Card` class, please. – RealSkeptic May 04 '15 at 18:05
  • I know this may sound stupid, but any chance that you are not populating the `cards` arrayList? Also, your `getAll()` method can be simplified to one line `return cards.toArray(new Card[cards.size()])` – user2829759 May 04 '15 at 18:07
  • As a side note, `System.out.print(newCard.getValue()); System.out.print(newCard.getSuit()); System.out.println(" ");` can be simplified to `System.out.println("" + newCard.getValue() + newCard.getSuit());` – null May 04 '15 at 18:08
  • Can you show your `Card` class? All of it? – Naveed May 04 '15 at 18:12
  • I've updated the OP with the Card class, sorry about that guys, – James Gould May 04 '15 at 18:18
  • Your compiler should be warning you about using `this` on a static variable – Naveed May 04 '15 at 18:28
  • @Naveed Thanks that's sorted the variables. I'm getting errors on method calls (such as Card.getAll()), eclipse is telling me to change getAll() to static. Any idea why/how I can get that working properly? – James Gould May 04 '15 at 18:37
  • That is because you are calling it on the Card class, `static` methods are also called `class` methods (if that helps you understand better) so basically each time you refer to a method with class name `Card.getAll()` it will need to be static (meaning just one copy). You use the instance (multiple copies) method by creating a new instance: `Card c = new Card()` and then `c.getAll()` – Naveed May 04 '15 at 18:44
  • The way you have it now, you getAll method can remain static however you should change the other variables (suit and value to instance) because you can have one list of cards but you will need multiple cards in that list. So just think static = 1, instance = multiple – Naveed May 04 '15 at 18:46

5 Answers5

6
card.addCard(newCard);

I think this should be cards.addCard(newCard); instead

Sizik
  • 1,066
  • 5
  • 11
  • 2
    And thus the danger of using the singular and plural of a word in the same code block! – pens-fan-69 May 04 '15 at 18:04
  • I thought so, too. But there is actually nothing that populates `cards` at all, so it's not supposed to get even a single value in the loop. Hence I'm not sure yours is the correct answer. I think there is something going on in the `Card` class. – RealSkeptic May 04 '15 at 18:07
  • Thanks. @pens-fan-69 I've updated the variable names so its not as confusing. – James Gould May 04 '15 at 18:09
  • 1
    Also, as I commented on the other answer, there is no `addCard` method in `ArrayList`. – RealSkeptic May 04 '15 at 18:12
  • @RealSkeptic I'll update the OP with the Card class to show that there is relevant code in the which holds the values of the cards. – James Gould May 04 '15 at 18:13
  • It now tells me "The method addCard(Card) is undefined for the type ArrayList" on addCard, would you mind explaining this in plain english please? Sorry to pester you! – James Gould May 04 '15 at 18:51
1

Your card class is not implemented well. The reason you are getting only the last value is because your getters and String variables are static. There is only one copy of static variable per class. You need to make those instance variables/methods and change your print loop to

for(Card c : cards){
    System.out.print(c.getValue());
    System.out.print(c.getSuit());
    System.out.println(" ");
}

See this answer for an elaboration on static vs instance

Community
  • 1
  • 1
Naveed
  • 2,942
  • 2
  • 25
  • 58
1

In Card the variables suit and value are declared static. This means that there is only one variable, shared between all cards.

When you load, you create a card. You set suit and value So they get h and a respectively. And then you print them.

Then you load the next two lines. You create a new card. But the variables are static, not part of the state of the new object, but the same suit and value that contain h and a. The new value is 2. And it replaces the a. Then you print it. The old value is gone.

So your print in the load shows the momentary value of suit and value, but in truth, there is only one value - the last value you loaded from the file.

These variables should be part of the card's state, thus, not static!

Note: your IDE notices when you try to use an instance variable from a static context. For example, your getValue method is static. So it can't access instance variables. However, the correction is not to set the variable to static, but rather to change the method, which logically is supposed to be an instance method, to not static. It may complain again because you are calling the method from static context. Then you have to carefully look why: you are not supposed to use this method statically - you should create an instance of Card and call the method using the variable you created.

So, IDE suggestions to "make it static" are not the correct solution! You should make all instance-related variables and methods not static, and solve "static context" issues by checking why you didn't create an instance and decided to call an instance-related method statically instead.

RealSkeptic
  • 33,993
  • 7
  • 53
  • 79
  • Thank you! Me and a friend of mine were wondering why eclipse was asking for everything to be static. Can you tell me why the IDE is telling me to make methods (and thus the variables they work with) static rather than just leaving them as they are? – James Gould May 04 '15 at 18:30
  • Okay I've used this.VARIABLE instead to cancel out the static errors but I'm still getting prompted to change methods (such as Card.getAll()) to static. would you mind explaining why this is? Thank you again, this has been a live saving response. – James Gould May 04 '15 at 18:39
  • @JayGould I added information about that to my answer. – RealSkeptic May 04 '15 at 19:02
  • Oh, and the `cardsList` actually has to remain static... It's not a very good design, but at the very least, it doesn't make sense that each card should have a separate list of cards. So `cardsList` *and the methods that manipulate it* should remain static, unless you are going to make a separate class that represents a list of cards (which is the better design). – RealSkeptic May 04 '15 at 19:13
0

card.addCard(newCard); looks like it could be cards.add(newCard); and also a tip, changing your indexing looks a bit nicer when you start at zero for the getAll function

    public static Card[] getAll(){
    Card[] brb = new Card[cards.size()];
    int tempCount = -1;
    for(Card c : cards){
        tempCount++;
        brb[tempCount] = c;
    }
    return brb;
}

change it to

    public static Card[] getAll(){
    Card[] brb = new Card[cards.size()];
    int tempCount = 0;
    for(Card c : cards){
        brb[tempCount] = c;
        tempCount++;
    }
    return brb;
}
Jurko Guba
  • 630
  • 7
  • 17
  • 2
    Does `ArrayList` have an `addCard` method? – RealSkeptic May 04 '15 at 18:09
  • Good call! Yeah it should just be add then – Jurko Guba May 04 '15 at 18:10
  • I think there is something going on in the `Card` class where there is an `addCard` and probably an internal list, and the list named `cards` has nothing to do with it. – RealSkeptic May 04 '15 at 18:11
  • It has an addCard method, sorry about forgetting to add it in. cards.add(newCard) isn't working but thanks for the tips! – James Gould May 04 '15 at 18:12
  • @JayGould Please edit your question and add all the necessary data, including the source of `Card` and any changes you made to the existing source. – RealSkeptic May 04 '15 at 18:14
  • What do you mean it's not working? Can you post the Card class and perhaps the error it shows? – Jurko Guba May 04 '15 at 18:14
  • @JurkoGuba I;ve added in the Card class. There is no error being shown, it simply displays (prints) nothing and then shows the menu again. – James Gould May 04 '15 at 18:16
  • Cool, did you just change the getAll method to say cardsList or is there another method that has cards? – Jurko Guba May 04 '15 at 18:19
  • @JurkoGuba I updated after someone commented about my variable names, the method itself hasn't been altered, they're just labeled differently so its easier to differentiate. – James Gould May 04 '15 at 18:20
-1

You are calling getAll() on a specific card, however before when you had card.addCard, you would only be adding one card to the list since every Card has its own ArrayList. This is why it only would print ONE CARD. You need to use getAll() on the ArrayList you made in the file.

Make a getAll() method in the main file that uses static ArrayList<Card> cards = new ArrayList<>();

   public static Card[] getAll(){
    Card[] brb = new Card[cards.size()];
    int tempCount = 0;
    for(Card c : cards){
        brb[tempCount] = c;
        tempCount++;
    }
    return brb;

This will work.

Jurko Guba
  • 630
  • 7
  • 17