• 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

I wrote a 2048 clone, looking for feedback

 
Greenhorn
Posts: 3
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Hey,
I'm Philipp and I've been learning Java for the last few weeks, coming from a Python Background.
I just finished my first "bigger" project, a clone of the game 2048. There are a few things left to add, but I am mostly finished with it.
I am still a bit confused by access modifiers, mostly I just followed the rule of thumb to keep everything as private as possible. Is it a bad habbit to change my methods from private to default for easier testing?
It would be awesome if you could give me constructive criticism for my code, I'm open to anything

https://github.com/mogl0768/2048-java
 
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

Philipp Horn wrote:I am still a bit confused by access modifiers, mostly I just followed the rule of thumb to keep everything as private as possible. Is it a bad habbit to change my methods from private to default for easier testing?


Yes.

It would be awesome if you could give me constructive criticism for my code, I'm open to anything


Well, a few things I see, from your Game class (I'm not qualified to advise on GUI code):
1. Why is direction an int[]? My assumption is that it would be some sort of value like "UP"/"DOWN"/"LEFT"/"RIGHT".
2. Don't use Math.random(). Use Random.nextInt() instead.
3. You don't have to fill an int array with 0's because that's the default.
4. Your program looks very "procedural" to me. In particular, I suspect that you could cut down considerably on the number of parameters you pass around by adding a Grid class that encapsulates your grid and all its actions, leaving your Game class simply as a "manager" or "translator" of user moves and arbiter of when a Player has won or lost.

Also: We generally prefer that you post your code here, as some people don't like to go to sites like github; so you will get more responses if you do.
And before you do, please read the UseCodeTags page.

Oh, and welcome to JavaRanch, Philipp.

Winston
 
Philipp Horn
Greenhorn
Posts: 3
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Winston Gutkowski wrote:
1. Why is direction an int[]? My assumption is that it would be some sort of value like "UP"/"DOWN"/"LEFT"/"RIGHT".


I chose to represent the direction by a vector in the form of {x, y}. So {-1, 0} would be left for example. I thought about using a string or constant, but since I am using the vector directly in the functions and it was still pretty understandable to me, I went with that.

Thanks a lot for your feedback, especially the part about my program being procedural kind of opened my eyes. Towards the end of the program I realized myself that my code got pretty hard to extend.
I'll post my GUI code here, in case anyone has a few points to improve on it:

 
Saloon Keeper
Posts: 15484
363
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Don't let GUIs implement listeners. This is a bad technique that sadly is taught in way too many tutorials and books. Use a nested class, or even better, instantiate your listeners with a lambda expression. The reason is that by implementing KeyListener, you're essentially saying that your GUI can be used as an all purpose keystroke handler, even in other classes that it has nothing to do with. You should also try to avoid extending JFrame. Your GUI uses a frame, it isn't necessarily a frame itself. I see you already have a frame member field which is unused right now. Use it.

Your frame, panel and keyToDirection members can be final. keyToDirection should be a Map, not a HashMap. If the mappings don't change between GUI instances, you can make the keyToDirection static, and initialize its entries in a static intializer block.

I don't think you should be using an int[] to represent a direction. Create a Direction enum instead.

Don't use magic values in your key mapping. Use the constants defined in KeyEvent instead.

Interaction with Swing components should be done on the Event Dispatch Thread:

Try to avoid creating new components after the GUI has been made visible. Instantiate your labels when you create the GUI, and just change their text value during the course of a game. You can then get rid of the call to validate(). Even if you don't go this route, you should call revalidate() instead of validate().

If you post more of your code instead of linking to it, we can offer some more advice on that as well.
 
Philipp Horn
Greenhorn
Posts: 3
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Thanks, that helped a lot. I didn't look into threads yet, but I think i understood the rest of your points and I already implemented the enum for directions (and it turned out to be pretty useful ).
These are my updated Grid and Game classes, this time it should be more object oriented.
Direction enum:

Grid class:

Game class:
 
Marshal
Posts: 28177
95
Eclipse IDE Firefox Browser MySQL Database
  • Likes 2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
An alternate way for assigning attributes to Enum elements (unrelated code omitted in this example):



You may find this more or less convenient; it's more self-documenting but on the other hand there's a bit more code necessary to support it.
reply
    Bookmark Topic Watch Topic
  • New Topic