We’re rewarding the question askers & reputations are being recalculated! Read more.

Hot answers tagged

26

Things I would fix: Don't do this: import javax.swing.*; import java.awt.*; It could be problematic for the compiler to import a bunch of packages at once. If two packages provide the same type, both are imported, and the type is used in the class, a compile-time error occurs. This is described in JLS 6.5.5.1: Otherwise, if a type of that name is ...


17

General Now that you have such neat postings, the answers are going to need to be neater too. GUI Bugs When I run the GUI, it does not let me select directories from the File Browser. It also starts in the 'Documents' directory, and it would be better to do one of two things: start in the current directory start in the last directory used (use java.util....


17

Will provide a fuller review if I can later, but anyways here are some quick starters... See major edit for point 3. Encapsulate the generation of typeList JComboBox as a method so that contents can be easily swapped I definitely don't have access to your "Images/" + elements[i] + ".gif", and I don't expect them to be posted here. :D Therefore, in my ...


16

I would encourage you to produce more explicit output, particularly with the filenames. If I wanted to reverse the process and scrape the code into files on my machine, using a Python script such as the following… import json from lxml import html import re import requests FILENAME_HINT_XPATH = "../preceding-sibling::p[1]/strong/text()" def code_for_post(...


15

Overall I find your code quite good as it stands. :) You've got sensible classes with sane, short methods. However, there were a few points that you could improve. Let's look at your solution class by class: Pong class: When you turn on strict enough compiler warnings in your IDE, you'll notice it warns about the fields WIDTH and HEIGHT hiding other fields....


15

OOP Java is an object oriented language, but you don't use it like that at all. Most of your code is inside the main method, and you don't have any objects. Because of this, your program logic is coupled with the presentation of the data, which makes the code hard to read and maintain. Also, if you want to change the logic (eg add a new Type), you have to ...


15

The entirety of the if/else statements are redundent if we consider that if we divide the current location by the tile_size we get the correct tile number int tile_size = 50; int tile_x = e.getX() / tile_size; int tile_y = e.getY() / tile_size; System.out.println(String.format("square %d%d", tile_x, tile_y)); I am not a Java Developer so pardon my Java


14

Math.pow(x, 2) -> x*x A pet peeve of mine is people calling Math.pow() just to square a number. To me it seems faster and simpler to just multiply the value by itself. Shadowing variable names In interact(), you use the variables x and y. I would avoid using variable names that shadow instance variables. I would call these variables dx and dy instead. ...


14

As you have probably noticed, there is a lot of repetition which we can take advantage of. We can declare an array containing all the string representations of the months like so. String[] months = {"January", "February", "March", ..., "December"}; Once you have that, we can simply do: if (num >= 1 && num <= 12) { System.out.format("The ...


13

It's much more important to convey the code faithfully than to save a few bytes of output by replacing spaces with tabs: line = line.replaceAll(" ", "\t"); // replace four spaces with tabs, since that takes less space In fact, since you used your program to produce this question itself, I suspect that your tool is responsible for introducing the bug in ...


13

Well, your post is over a year old, but hopefully this can still be of some use to you! Javadoc No need to say the method name again - the method name is automatically part of the Javadoc. What are the parameter constraints? (To be fair, you don't enforce any constraints in your code, so it makes sense to not mention any in your Javadoc) That said, you ...


12

There are a bunch of style issues you should be aware of, a validation problem, an optimization or two, and also an alternate approach: Style: Use braces for 1-liners... these are 'no-no' statements: if (i % 3 == 0) sb.append("Fizz"); if (i % 5 == 0) sb.append("Buzz"); if (sb.toString().isEmpty()) System.out.println(i); ...


12

Model vs UI Your classes have basic separation of concerns. But as you can see by yourself some model elements like "Ball" or "Box" or "Paddle" or even "Game" have a dependency to the package java.awt.*. Try to reformulate your model not to point to UI elements. This is not just because you may provide another UI technology like JavaFX. Sure you can do that ...


11

This is more to tackle than would be easy to cover in a single answer. Small items. There are a couple of small items that are a problem. you are using capitalized names for variables.... this is against the coding style for Java. Variable names should always start with a lower-case letter. For example, your Button7 should be button7. if(SecondValue....


11

Bug! In your program, it's impossible to use any operator besides addition. As Code Review is not meant to be a code-bug-fixing site, I think you should try to fix this yourself. Hint: The bug is in your addActionButton method Now, about your question Yes, the template method is a good choice. Removing code duplication is always a good idea, which is I ...


11

Vogel612's answer already covered naming. I'd like to highlight some other things: some because of how Swing works, and some about initialisation. Initialisation Initialisation and reset take mostly the same actions but are in two different places: ButtonListener3.actionPerformed and Guess.<init>. Try taking these actions out and creating methods ...


11

Create a new Actionlistener class and add it to all Buttons: b1.addActionListener(new MyActionListener(1)) b2.addActionListener(new MyActionListener(2)) b3.addActionListener(new MyActionListener(3)) ... -- class MyActionListener implements ActionListener { private int guess ; public MyActionListener(int guess){ this.guess = guess; } ...


11

You should separate the modelling of the game, board and pieces from the display components. (google: model-view-controller, separation of concerns). You can create a Game class that models the game and the game-logic You can create a Board class that models the internal state of the chess-board. You can create a Piece class and subclasses for each Piece, e....


11

The code is damn long, so just a few notes: Use initializer expressions like private JRadioButton setRed = new JRadioButton("Red", true); Use static factory methods to save even more like private JRadioButton setRed = makeColorButton("Red", true, Color.RED); private static JRadioButton makeColorButton( String name, boolean isSet, Color color) { ...


11

Some simple pointers to get things started... Magic numbers You have hard-coded your side, limit (how is it used, and why does it default to side - 2?), the number of mines, the dimensions of your buttons and the meaning of a Cell's value to -1 when it is a mine... these should either be extracted out as constants, or documented clearly to show how they ...


11

import java.util.Scanner; import javax.swing.JOptionPane; //imports //--------------------------BEGIN PROGRAM--------------------------------------------------------// public class justForFun {public static void main(String args[]){ … It's weird that the entire program is text-based, except that the error message is shown using Swing. It is ...


10

Is it a good practice just to import everything so I don't have to go back and add stuff? In a modern IDE (e.g. NetBeans) you can just type in the class you intend to use and quick-fix the import. I usually do this so that I don't have spare imports laying around inadvertently. Also, you can use the source cleanup tool to remove unused imports when you're ...


10

Hmmm.... this is a big one. Right, let's break this review in to five sections: Code Style Threads Class Hierarchy Swing Performance.... Code Style This is a laundry-list of things. I'm not going to explain them all, but: Class names should be CamelCase, lolee is not a good name. Nor is class keyPressed. Class names like Action1 etc. are also useless for ...


10

tempNumbers isn't a very explanatory variable name. Let the variable itself describe what it is temp for. For a simple calculator though, perhaps number1 and number2 would suffice. You're using strange indentation in your constructor. It's good that you separate the fields, but no need to add an extra indentation for the lines where you call methods on the ...


10

import javax.swing.*; import javax.sound.midi.*; import java.awt.*; import java.awt.event.*; Most style guides will frown upon the wildcard import -- is it so much extra work to list the classes you actually depend on? This will also help you to avoid conflicts, where you want List to mean java.util.List rather than java.awt.List public static ArrayList&...


10

char[] pw = pg.generate(numberOfChars); for (int i = 0; i < pw.length; i++) { txtPW.append(Character.toString(pw[i])); pw[i] = 0; } What the... Have you seen the String(char[] value) constructor? There's also String.valueOf(char[] data). Those would make your loop irrelevant... unless there's a specific reason you're setting the ...


10

Your passGen class does not need to be that complicated even if you don't want to use Strings. Your starting and ending values do not need to be changed for each class so they should be final. They will also not need to be different over all classes so static would be a good idea here (remember to use Capitals and underscores, the convention for constants). ...


10

Lets start by naming: It should be clear that the frame doesn't have the name of the project, so instead of the class ChessProject you can call this class ChessFrame instead. It's all about separation of concerns, you should have seperate class that does the Chess logic and one that does the UI and use composition to use its features class ChessBoard{ .....


10

Code smell: Large class: a class that has grown too large. See God object. You have put the whole game, all it's screens, all the game functionality, all components, everything! into one single class. This has made it very hard to see which things belongs where. For example, if I look at the method getNumbersAsString or getInsets alone, I have no idea in ...


10

I would recommend: You're using a GridLayout(2, 3) -- good. I'd make it actually a GridLayout(2, 0, GAP, GAP) where GAP is a constant that allows a gap between JLabels. The first and second parameters mean 2 rows with variable number of columns. Then if you set the background color of the JPanel to yellow, make the JLabels opaque and white, give the ...


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