• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Jeanne Boyarsky
  • Ron McLeod
  • Paul Clapham
  • Liutauras Vilda
Sheriffs:
  • paul wheaton
  • Rob Spoor
  • Devaka Cooray
Saloon Keepers:
  • Stephan van Hulst
  • Tim Holloway
  • Carey Brown
  • Frits Walraven
  • Tim Moores
Bartenders:
  • Mikalai Zaikin

Some constructive criticism please and also hello

 
Greenhorn
Posts: 7
2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I have searched a few forums and I keep coming back to this one. So this is going to be home for Java, I really like the site. My name is Brandon and I have a little programming knowledge in Visual Basic. I like learning new things and I decided to pick up Java about a month ago. I bought and have been learning from a book called Head First Java. It puts me to sleep (I learn better being hands on and trial and error), but I try to retain as much as I can, and have been supplementing the reading with a Java tutorial series by Derek Banas on YouTube.

I am just now getting ready to learn the GUI part of it but before I get into all of that, I wanted to write a simple card game. I finally completed it after 3 versions (improving on each) but I sat back and looked at it and I am sure it is not OOP.

So my first real semi-program in Java, I was hoping that maybe someone would take the code I have written and give me a hard lesson in Java. It is less than 200 lines of code and only deals with one deck and 2 players. I am learning this on my own as a hobby because I have some ideas I want to implement once I feel I can tackle the job.

I was reading over some of the other beginner posts and I hope I can help on some of those posts. In the meantime, here is the code from my War card game. If anyone wants to give out pointers or show me how to make this OOP that would be great. I kind of feel there should be these classes: Main, Player, Hand and Deck. I am not sure how to make them fit together though.

Thanks in advance and I can't wait to check back tomorrow.

edit: oh, I am not asking for anyone to re-do the program, but if someone felt they wanted to in order to illustrate better what OOP was ... well . I really just need someone that is better than me to give me ideas and thoughts on how to better my own understanding and any bad habits that might be visible from the code below. I rather break those bad habits now than later.




 
Saloon Keeper
Posts: 10687
85
Eclipse IDE Firefox Browser MySQL Database VI Editor Java Windows ChatGPT
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Some comments:
'suits' and 'cards' should be in all upper-case because it's a static final, ie a constant.
Too many static members, they should be non-static fields.
Only use under-scores (_) in constant names. Use camel-case for everything else. Eg change Card_Pool to cardPool.
Variable and method names should start with a lower-case. Eg PlayerTwo, and Start_Game.
Methods that check for the presence of something and returns a boolean usually begin with 'is', eg 'isWar' instead of 'Check_For_War'.
Program to interfaces. Eg use List<String> tempDeck = ArrayList<>();
Used Random class instead of Math.random.
Change randNum = (int)(Math.random()*deck.size()); to randNum = rand.nextInt( deck.size() );
Line 161, variable name of 'i' is usually reserved for integers. Perhaps 's' would be better.
Nice indenting.
 
BrandonA Brown
Greenhorn
Posts: 7
2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Carey Brown wrote:Some comments:
'suits' and 'cards' should be in all upper-case because it's a static final, ie a constant.
Too many static members, they should be non-static fields.
Only use under-scores (_) in constant names. Use camel-case for everything else. Eg change Card_Pool to cardPool.
Variable and method names should start with a lower-case. Eg PlayerTwo, and Start_Game.
Methods that check for the presence of something and returns a boolean usually begin with 'is', eg 'isWar' instead of 'Check_For_War'.
Program to interfaces. Eg use List<String> tempDeck = ArrayList<>();
Used Random class instead of Math.random.
Change randNum = (int)(Math.random()*deck.size()); to randNum = rand.nextInt( deck.size() );
Line 161, variable name of 'i' is usually reserved for integers. Perhaps 's' would be better.
Nice indenting.



Thanks for the notes (wow, quicker than I expected).
I haven't gotten to List's yet, but now that I see it for the 'For' loop, i should use 's' instead for string or 'c' for char. thanks. I just read about the Final and all-caps today, didn't change it but I should have, thanks for pointing that out. Will learn about Random soon I hope and will use it from hence forth and much appreciated the comments on variable and method names, I drather use the convention than my own personal tastes. The boolean thing = 100% sound reasoning.
Will learn about List and Random tomorrow and try to update code where needed.

THANK YOU this was educational and will post back tomorrow.

 
Carey Brown
Saloon Keeper
Posts: 10687
85
Eclipse IDE Firefox Browser MySQL Database VI Editor Java Windows ChatGPT
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
shuffelDeck() could be written as:


Don't call shuffleDeck() twice.
 
Carey Brown
Saloon Keeper
Posts: 10687
85
Eclipse IDE Firefox Browser MySQL Database VI Editor Java Windows ChatGPT
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Constants should be in all caps.
'final' is not necessarily a constant.
'static final' is always a constant.
 
Bartender
Posts: 2407
36
Scala Python Oracle Postgres Database Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Welcome to the ranch, Brandon!

You might be interested in the online Java Programming course from Duke University, which starts this week on Coursera. There is a small fee for the course, but I think you can opt to audit the course for free if you're happy to do without a course certificate. I've done several free courses via Coursera and I've found most of them to be pretty good.

Alternatively, there is another free online course in Java from Udacity which you can study at your own pace. The course team includes Cay Horstman who has written several excellent books on Java.

I really like the "Head First" books, but it can be helpful to try different learning materials to see what works best for you. Some of these online courses are really good, well-structured, clearly presented, with a good mix of materials and practical exercises.

Good luck!
 
lowercase baba
Posts: 13089
67
Chrome Java Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Anyone who uses code tags on their very fist post deserves a cow!!!
 
Bartender
Posts: 10780
71
Hibernate Eclipse IDE Ubuntu
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

BrandonA Brown wrote:I am just now getting ready to learn the GUI part of it but before I get into all of that, I wanted to write a simple card game.


Well that's a really good thing to do right there.

Java GUIs - in fact, GUIs in general - are fiddly, verbose and distracting; so anything you can do to delay writing them (IMO) is good.
(You may, perhaps, have gathered from the advice above that I'm not a big fan of them )

Remember: A GUI is concerned with two things, and two things ONLY:
  • Displaying content.
  • Getting response from a user.
  • It cannot play a game for you. For that, you need a class that knows the rules and logic of the game.
    And you should try as hard as you possibly can to keep the two things separate.

    And on that subject, I have one basic criticism of your Deck class: It does things that I don't associate with a deck of cards; specifically: Start_Game() and (arguably) Deal_PlayerCards(). And that's because you're already thinking about what you're going to DO with it.

    It may well work, but it'll be a "one trick pony"; and if you decide to write a program to play another game like Poker, or Canasta (or Snap), you'll have to rewrite your Deck class from scratch - even though you'll probably find that you copy a lot of code between them because they all use the same type of deck (4 suits, 52 (or 54) cards, A, K, Q, J, 10... etc).

    So you need to separate what a deck IS from the game it's used in - and to do that you need to think about what a Deck actually is.

    And basically, it boils down to a bunch of "cards", each of which has a suit and a name, which suggests that you might also want a Card class...
    You'll certainly want to be able to shuffle it, but dealing is likely to depend entirely on the game it's used in.

    Now I don't want to inundate you with design here, or re-write your class; but do you see how you might want to break up some of the stuff that's currently in your Deck and put it in your War class? Because that's the class that knows how to USE the Deck. Without the game, a Deck is just a pile of cards.

    HIH

    Winston
     
    BrandonA Brown
    Greenhorn
    Posts: 7
    2
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    Thank you so much for the feedback.
    Carey -> I still need to learn about Lists. I tried just putting it in there and it broke. Will def be taking a look into it and also thanks for the Collections.shuffle() method. Nice to have that built in. I am not sure what you mean by to many static members. I think i add that keyword in there because that is all I know so far. Static makes it readable by the whole class? or I probably read that wrong.

    Chris -> I enrolled to audit a class, thanks for the link. Will let you know how it goes.

    Fred -> thanks for the Cow I read several threads on here before registering and one thread someone had mentioned to another to always put code in Code tags, so learned that before posting

    Winston -> Yeppers, I don't like the GUI side of programming. I took several classes in Visual Studio and it took away from the coding big time. As for using different classes. this was my third attempt at the program, and originally I had several classes, but I couldn't get them to work together for some reason and I finally got Cards and Deck to work together, but then I couldn't figure out how to deal and assign players cards. The last attempt, just the one class was to get it working. I hope to divide these up into several classes soon. I found an situation where the game broke anyway, so I need to look at closer. It breaks if the last hand before a winner and there is isWar();

    Again, just wanted to say thanks and below is the modded code with your suggestions. I will def try to keep up with naming conventions. VB had me under scoring and doing other things. Old habit.
    Thanks my new java friends



     
    Carey Brown
    Saloon Keeper
    Posts: 10687
    85
    Eclipse IDE Firefox Browser MySQL Database VI Editor Java Windows ChatGPT
    • Likes 2
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    You're welcome Brandon. Nice job of cleanup. If you do come back to refactor this at some point bear in mind Winston's comments about OO design.

    Regarding 'List':

    'List' is an interface. An interface defines what methods a Class that implements it should support. It does not do the implementing itself.

    An 'ArrayList' implements the 'List' interface. The 'ArrayList' provides the actual working code and implements the methods specified by the interface.

    When you do some thing like:
    you limit how var can be used - it can only be used as an ArrayList.
    If you decide later that a LinkedList would have been a better implementation you will have to go back through your entire project and replace the affected ArrayList's with LinkedLists's.

    If, on the other hand you did this:
    you are in a much more flexible position and can easily swap in a LinkedList in place of your ArrayList without impacting your code. Eg.
    Project code will not have to change its use of var.

    Note that you will also need to change you method declarations. E.g.
    would then become


    Note that:
    does not work. This is because you can't instantiate an interface.

     
    Bartender
    Posts: 1845
    10
    • Likes 1
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    Just some feedback for shorter/tidier code.

    The variable "faceDownCards" is only referred to - its value never changes. It should therefore be a constant (and then get renamed to FACE_DOWN_CARDS)



    The if expression here is irrelevant. "While" effectively has an implicit if. You could lose the if statement entirely, and it will work exactly the same.


    Calling "remove" on a list returns you the item you removed.
    So these two lines:


    could become one:


    And to me at least it makes more sense: remove a card from a player's hand, and add it to the pool.

    cardPool.add(parseCardToInt(playerOne.playFirstCardInHand()))l

    and actually what I would really want to see in the final product is:

    cardPool.play(playerOne.firstCardFromHand()) or something along those lines anyway.
     
    BrandonA Brown
    Greenhorn
    Posts: 7
    2
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Carey Brown wrote:You're welcome Brandon. Nice job of cleanup. If you do come back to refactor this at some point bear in mind Winston's comments about OO design.
    ...Regarding 'List':



    Thanks for the info, I am java'd out for the night, but thanks to everyone's responses, I am going to use this program as a learning tool. Since this is version 3 of the program

    I want to get it back to my first instinct of having at least 4 classes and make them work together. So when I have time, I hope to make this more of an OOP. Many thanks to everyone and tomorrow, I am going to learn a bit more about Lists so I can implement it and see what the benefits and other are going to be.
     
    Saloon Keeper
    Posts: 15484
    363
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    Don't put your classes in the default package. It's a good habit to always put your classes in a named package. You'll avoid a bunch of problems down the road. For instance, you can use abbreviations for the name of the class you're enrolled in and the name of the assignment.

    BrandonA Brown wrote:I am not sure what you mean by to many static members. I think i add that keyword in there because that is all I know so far. Static makes it readable by the whole class? or I probably read that wrong.


    A static member is a member that's part of the class, not of an object of that class. That means that you don't need an instance of Deck to access cardPool. cardPool is essentially something that floats free in empty space. While that may make sense at first, it goes against the spirit of OOP. Always think to yourself, what is this conceptually a part of? In this case, a pool of cards is part of a game, and so are the players. The pool of cards and the players make no sense outside of the context of a game, so make them non-static fields of Game.

    Keep this guideline in mind: The static modifier should only be used for nested classes, pure functions and constants.

    Another thing, fields should almost always be private. If you need a field to be static or non-private, you probably need to redesign your classes.

    Fred -> thanks for the Cow I read several threads on here before registering and one thread someone had mentioned to another to always put code in Code tags, so learned that before posting


    I agree with fred, and another cow for keeping us updated on your progress.

    Winston -> Yeppers, I don't like the GUI side of programming. I took several classes in Visual Studio and it took away from the coding big time.


    I've come to enjoy GUI coding myself, but only because I've taken the time to really understand how to code them, instead of using a visual designer. Whenever you feel ready to move on to the GUI part, I strongly recommend you code them by hand, and only start using visual designers when you're confident with the type of code they'll generate for you. It may seem onerous at first, but you'll be happy you did afterwards.
     
    Stephan van Hulst
    Saloon Keeper
    Posts: 15484
    363
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Another comment. String is a weak data type. Strings contain data, but don't say anything about what the data means. "5" may refer to the rank of a card, but it may also refer to the floor of a building, or the day of the month. Try to maximize the use of "strong types". A strong type is a type that says something about what the data means. In this particular case you can use enums. An enum is a strong type that has a fixed number of values, for example:


    Each of the constants declared in the enum is an object of the type DayOfWeek. Therefore, you can do the following:

    Like Winston, I suggest you make a Card class that represents a playing card. I further suggest you add enums Card.Rank and Card.Suit.
     
    Winston Gutkowski
    Bartender
    Posts: 10780
    71
    Hibernate Eclipse IDE Ubuntu
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Stephan van Hulst wrote:Like Winston, I suggest you make a Card class that represents a playing card. I further suggest you add enums Card.Rank and Card.Suit.


    Again, I don't want to blind Brandon with design here, but I'd say that the enums belong in Deck, because it's the deck (or type of deck) that defines what suits and ranks are used. They're certainly attributes of a Card though.

    You also have two jokers, so what's the "suit" of a joker...or is "joker" itself a suit? And the same question applies to rank.
    In order to keep to the rule that suit + rank uniquely identifies a card you can't just make both "joker". My solution is to add a "jokers" suit, and two extra ranks: "red" and "black"; but you could just as easily swap them around. The game determines whether they're actually used or not.

    And on that point, it's worth remembering that games use different sets of cards. Many use the 52 non-joker cards, but some, like Sechsundsechzig (66), use a subset (in its case, A through 9, and they have a different order from normal); Canasta uses two decks of 54, and Blackjack can use anything from 1 to 8 52-card decks. So it might be an idea to add a 'Shoe' or CardSource class.

    In some games - eg, Blackjack and Texas Hold 'em - there is also significance in whether a card is visible or "hidden" (either face-down or in a player's hand).

    So, Brandon: quite a lot to think about.
    I doubt you have to worry about all the issues raised by us, but I'd certainly consider Deck, Card and possibly Shoe, along with the Suit and Rank enums suggested by Stephan.

    HIH

    Winston
     
    BrandonA Brown
    Greenhorn
    Posts: 7
    2
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    Thank you all

    So, I only have a little time here and there, but I was excited about re-creating this. Randomly watched some videos and picked up on some new conventions. I have 5 classes and they are all working with each other. I am stopping for the night, but wanted to post what I have so far. This was a big learning venture in that I can start to see how the classes work together. I don't see or haven't benefited from it yet, but it is starting to seep into the logic of it. This took me about 2 hours tonight. I won't be able to spend much time the couple of days, but if you see anything worth commenting on PLEASE do



    I took everyone's note to heart although I didn't get into ENUM, for now I am going to stick with List and hard code a lot of it as a learning tool. This is not a working program, it only does the basics, but man - I am super happy with what I have so far

    My questions would be: What does 'Packaging' have to do with anything? I assume it is so I can re-use code. I took a snippet of what I think it means. I didn't start a GoFish.java project, just for illustration and attached.

    Main:

    Card Class

    Deck Class

    Player Class

    Dealer Class
    newWar.JPG
    [Thumbnail for newWar.JPG]
     
    Marshal
    Posts: 79151
    377
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    I am not happy with the method which parses the cards to numbers. That is te sort of thing you could do more easily if the faces were in an enumerated type. You can read about enumerated types in the Java® Language Specification (=JLS). The JLS can be difficult to read, but the examples will be easier to understand.
     
    Ranch Hand
    Posts: 165
    12
    • Likes 1
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    Getting your head around basic OO design is definitely one of the trickiest parts of learning Java. Personally I would not worry too much about packages at this point because it just adds another layer of learning that you could probably do without at this point - keep everything in your War package by all means but focus on the Object stuff for now.

    Enums on the other hand I would say are worth your time as you don't want to get into bad habits by using Strings for everything. At a basic level they are very straightforward. This is probably all you need to know for now to declare, assign and display an enum:


    Easy enough, yes? Do something similar for Suit.

    I would focus on building your game one step at a time instead of trying to build the whole thing in one go. Most developers build applications piece by piece and test each class before moving on to the next. Thats one of the great benefits of OO in fact.

    As you have realised your most fundamental object/class is a Card. So if I were doing this exercise I would start by creating a Card class and a class containing the main() method, you have called it your War class, thats fine. I would forget the rest of your code for now.

    You already know the card object needs to have suit and a rank (because each individual card has a suit and a rank) so you need a field in your Card class to store each of these two attributes. Your Card Constructor method should create one card object, so it will need to be passed a suit and a rank so it can initialise those two fields. The other essential element of the class is a toString() method so that you can display the value of this card object in terms of suit and rank, eg "THREE of HEARTS". Your main() method at this point will just create and display card objects, ie create a card (eg four of diamonds), display it, create another one (eg king of hearts), display it, etc.

    Once you have your Card class working you can move on to develop a Deck class. A Deck class will store up to 52 cards and will need a constructor method to generate those cards and store them. And I would suggest it has a method such as takeACard() which returns one card object which it gets from the top of its deck and removes it from the deck. You could optionally write a display method to display the entire deck (for debugging/testing use). Having written the Deck class you can then test it from your main() method as you did previously for your Card class, create a deck, call takeACard(), print the card's value, take another card, display it, etc etc.

    And onwards you go from there.
     
    Ranch Hand
    Posts: 704
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    So far you have been given some great advice and pointers to help you on your way.
    If you do the exercises in the Java Tutorial you'll create a deck of cards.
    Also I would create a Test class to ensure that you test the code as you go along. Here's an example of a really simple one
     
    BrandonA Brown
    Greenhorn
    Posts: 7
    2
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    I feel a little embarrassed because I had to put learning Java on the back burner for a few weeks and I plan on picking up where I left off, but yeppers, a lot of community support and advise. Hopefully next week, my days and nights go back to normal. I clicked that link Nigel, but it was a 404 page. When I get back to it, I will read this thread again carefully and try to implement a lot of what was advised and post some more developments.

    Thanks guys
     
    Nigel Browne
    Ranch Hand
    Posts: 704
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    Heres the link in plain text.

    https://docs.oracle.com/javase/tutorial/java/index.html
    The card/deck exercises are in the tutorial:
    Lesson classes and objects.
     
    permaculture is giving a gift to your future self. After reading this tiny ad:
    a bit of art, as a gift, that will fit in a stocking
    https://gardener-gift.com
    reply
      Bookmark Topic Watch Topic
    • New Topic