A message from our CEO about the future of Stack Overflow and Stack Exchange. Read now.

Hot answers tagged

15

Start using New IO The java.io packages have been replaced for a reason. They don't integrate well into what the language tries to become and are at best unwieldy. Instead you should rely on the new IO or nio packages, that work on Paths instead of Strings and have some other advantages. Loading your properties can be simplified to: try { ...


13

Cards public void setSuit(Suit suit) { this.suit = suit; } No! No! No! Just drop the setters. With them, you can never return a Card from any method without the risk that someone changes it. Cards are immutable in all games I know and must be immutable in the code, too. Moreover, never write methods you don't need (unless you know you'll need them ...


12

Server private static String names() { StringBuilder nameList = new StringBuilder(); for (String name : userNames) { nameList.append(", ").append(name); } return "In lobby: " + nameList.substring(2); } The above can be simplified by streaming and collecting with Collectors.joining(CharSequence, CharSequence, CharSequence) using ...


11

Empty Lists if (list.isEmpty()) { throw new IllegalArgumentException("list cannot be empty"); } Yuck ... you should be able to handle an empty list more gracefully than that. Creating a special EmptyCircularListCursor instance perhaps. Random Access I like that you differentiate between RandomAccess and other lists. This is a good thing. ...


11

You can have more classes Your start(Stage stage) is doing a bit too much for what I like to see. You're creating an Anonymous Class that encompass the logic of reading the socket and the presentation of to the client. I would create a separate class that would extract that logic. This would need to pass the socket and the textbox, but I'm sure there is an ...


11

Naming Manifest constants typically use _ for space in Java. So NANOSPERSECOND for example should be NANOS_PER_SECOND. Also I believe that the name MainLoop is quite misleading as a name because the class isn't actually your main loop but rather a timing helper. Avoid unnecessary conversions You don't need the double precision here: public final static ...


11

Firstly, I'm not familiar with the JavaFX nor PostgreSQL technologies, so my comments will be aimed more at Java-specific code practices. See also the answers by Vogel612 and Antot. Use "try-with-resources" Java makes memory management simpler, because it is a garbage-collected language. However, main memory is not the only resource your program will use....


10

A List, a list! My kingdom for a list! List<Champion> champions = new ArrayList<>(); champions.add(new Champion.Generator( "Legolas' Lover", // name new Image("/Resources/Ranger.gif"), // icon new Image("/Resources/Ranger.jpg")) // portrait .attack(4) .armor(2) .health(8) .description("Has ...


10

Use interfaces as much as you can. private List<Card> deck = new ArrayList<>(); Checked Exceptions are bad, don't use them public class EmptyDeckException extends RuntimeException{ .. } public Card draw(){ // no throws } You don't need to write a shuffle method yourself, there is a one in the JDK already List<Integer> numbers = new ...


10

Naming private static final String BACK_IMAGE_DIR = ("image/back_image.png"); You call this BACK_IMAGE_DIR which suggests that it is a directory, but it's actually a file path. Just BACK_IMAGE might be a better name. Are you sure that you want this to be a property of a Card? It seems more common to make all cards in a deck share the same back. ...


10

Server I think you are over-using the static keyword. Don't make each and every function static, create an object of the class and use the object instead. You have some catch parts that are either empty or print very little information. I suggest you investigate a bit more about the different reasons those exceptions can occur and add some comments about ...


10

I totally agree with Vogel612's observations. Since there is a lot of code for this review, I'd like to add a couple of items. DatabaseAdapter Class In getConnnection() method, there are three issues, two of them are quite serious: There is a careless call of System.exit(0). I'll just recall a sentence from the good old Hardcore Java about it: If you ...


8

Pretty neat. I think this: Label label = name.isEmpty() ? new Label("Hello user!") : new Label("How's it going, " + name + "?"); Could be neater like this: Label label = name.isEmpty() ? new Label("Hello user!") : new Label("How's it going, " + name + "?"); Actually... what changes depending on the result name.isEmpty() isn't the label itself, ...


7

As someone who's been working with your project and your Console Gui here's my 2 cents: protected final TextArea textArea = new TextArea(); protected final TextField textField = new TextField(); these could use a rename. The names do not contain any information and in all the unit tests you wrote, this was confusing me very much: Which is in, which is out? ...


7

Naming Your class is named circularText, it should be named CircularText as all classes in Java should start with a capital character. Declared fields You have declared two fields as private but what about the rest? double textSize = 30; String string = ""; String fontName = ""; Font font = new Font("Times Roman", textSize); All these should be marked ...


7

No, this is not the correct way to do it. I would recommend that you read JavaFX Transitions documentation. I personally could not get your code to work properly (Using Java 8, but that should not matter as Java is always backwards compatible). The rectangle showed on the screen, but it did not move at all. I got this exception: Exception in thread "Timer-...


7

Your JavaFXGame class can use javadoc /** * This class just loads the FXML document which initializes its DocumentController */ public class JavaFXGame extends Application { The start method can be shortened and cleaned up: @Override public void start(Stage stage) throws Exception { Parent root = FXMLLoader.load(getClass().getResource("...


7

Constructor CircleInfo(String circleLabel, double centerX, double centerY, double radius) can be simplified to CircleInfo(String circleLabel, CircleObject obj) because the constructor can get x, y and radius from CircleObject. The same for checkIntersection(...). This method can belong to CircleObject, such that you can call it from the object itself, i....


7

All your fields can be declared private. There is really no need to expose these more than necessary. @FXML private Label systemOut; @FXML private GridPane gridPane; private final static int ROWS = 3; private final static int COLUMNS = 3; private int[][] grid; private boolean turn; Your initialize and newGame methods looks very much the same to me, you ...


7

As already said, a kingdom for a list and small one for a CSV: IconName;Name;Attack;Armor;Health;Description Ranger;Legolas' Lover;4;2;8;Has less health than most, but decent armor, and strong attacks! Dragon;Scion of Daenerys;4;2;8;Is a motherloving dragon Angel;Full Metal Bitch;4;2;8;Really likes rock music, tons of defense Spirit;Forlorn Phantasm;4;2;8;...


7

I don't believe that this is exactly what you were looking for in a review, but I thought I'd point it out: In the Card class's toString method, it looks like you are starting to create a switch statement which will check each possible numerical value of the enum property suit and, depending on the number, log out what the card is. However, I think a ...


7

Code-wise I can't really see any major problems. The only glaring issue I see is that scene should be private. The remainder of the code is neat, and consistent. I like that. The rest of my review is about the usability. It irritated me that I had to click on the text entry to add my name. I realized that you forced the focus on to the hello button in ...


7

2 male and 2 female superheros... Very politically correct ;-) I suggest to try to avoid repeated code as much as possible. For example, Desktop.getDesktop().browse(...) is used in most of the case statements, only the URL parameter is different. I would refactor this in a way that the case statements set a URL variable, so that you can write the Desktop....


7

View It was a lucky choice to implement tic-tac-toa and not e.g. go. With circleOne to circleThreeHundredAndSixtyOne it could get rather lengthy. You can see what I dislike most. Starting with the XML abuse(*), the repetitiveness propagates everywhere. Maybe FXML allows arrays and loops, which could shorten your XML. But then we get to another point, ...


7

There's not a lot to review here, the only thing that's jumping at me is that this: @Override public void start(Stage primaryStage) { System.out.println("Hello world"); Scanner scanner = new Scanner(System.in); scanner.nextLine(); Platform.exit(); } Should be indented like this: @Override public void start(Stage primaryStage) { ...


7

public class DateFormatter { static String date1 = ""; static String date2 = ""; static String date3 = ""; static String date4 = ""; static String date5 = ""; static String date6 = ""; static String date7 = ""; public static void dateFormatter(NumberAxis xAxis){ loadDates(); xAxis.setLowerBound(0.0); ...


7

I'm really sorry, but after starting this review I have to tell you: Go back, read some more tutorials and try again This code has so many issues, that I'm not able to point them out reasonably without saying something stupid and / or offensive, so I'll give you a few tips that should help you, if you rewrite this from scratch: Avoid static Like The ...


7

A few recommendations per class: Food.java What's the point in this class? It just's just a Point with a constant colour. I would just have a FOOD_COLOUR constant in your Painter and then you can scrap this. GameLoop.java frameRate and interval should be final. Actually you don't need frameRate at all because you don't use it for anything other than the ...


6

It's hard to pick on such simple and straightforward code. The one thing that stands out to me is the distance between the line where you declare the root variable, and the lines where you actually use it. Let me quote an interesting paragraph from Code Complete: The code between references to a variable is a “window of vulnerability.” In the window, new ...


6

Do not use underscores on Class names : public class Project16_8 extends Application { ... change this to public class InterceptingCirclesApplication extends Application { also the start() is like the main method, keep it simple. There is too much going on in your start method. change it to only contain 4-6 lines Node mainView = new MainView(); ...


Only top voted, non community-wiki answers of a minimum length are eligible