# Tag Info

27

Name Classes properly. What is a "Back Wards Prime"? I have no idea. I can guess what a Backward Prime is; perhaps you need to change the capitalization of the class! I'm told that name is dictated by the coding challenge, so you can't change it for your challenge submission; but I'm leaving this comment stand -- the required class name is ...

20

isPrime(9223372036854775783L) incorrectly returns false. Your i * i overflows, better use i <= n / i. Not sure it matters in the context of the problem, though. At least the reverse of this number isn't prime anyway. Division might be slower than multiplication, but might produce n % i as a by-product and a smart compiler might then use that instead of ...

20

You are not assigning default values to the fields and you are splitting game logic and UI. That's a good start. public Game(int i, int j) { In a public function or constructor I expect clearly named parameters. if (i > 100 || j > 100) { throw new IllegalArgumentException("grid too big!"); } So a grid of 101 cells is too big if i = 1 and j = ...

19

Welcome to Code Review and thanks for sharing your code. Your class OfficialAddress extends your class User. This does not look right. By extending a class you declare an is a relationship. But an address never is a user. instead a user has a Address. In OOP this is expressed by giving the owner (class User) a property of the other class (OfficialAddress)...

17

Welcome to code review and thanks for sharing your code. When writing UnitTests keep in mind that they are documentation. Test code has less restrictions for identifier names. This means that method names are usually longer and you should not "shorten" your unit test methods. The test name should describe the expected behavior in detail @Test public void ...

15

Formatting Run your code through a style checker / formatter to make it more readable and follow a standardized code style. For example this one https://codebeautify.org/javaviewer Don't repeat yourself 1 Complex minusOne = new Complex(-1.0 ,0.0); You are defining this in more than one place. This should be a final member at the beginning of your class. Don'...

15

Historical Note The very first comment on the OP's question -- long before any answers were posted -- was a question asked by myself, and it very pointedly asked: Without using Integer.reverseBytes(cafeBabe)? The question was two-fold: Point out to the OP that a built-in method exists, and is likely the most optimal way of doing this. Ask if they are ...

15

Keep it like that. It is perfectly readable, easy to understand, everything is fine. Naturally you could do some lambda trickery along the lines of: public static String extractFromMessageOrQuery( Update update, Function<Update, String> messageExtractor, Function<Update, String> queryExtractor, String defaultValue) { ...

14

I don't see anything object-oriented in it. A program without a single non-static method can hardly be called OO. The main task of this piece of code is to compare PlayedMove instances, so to follow OO style, you should make that comparison a method of PlayedMove. This will make the enum a bit more complex (beyond beginner level), so you might want to switch ...

14

The implementation looks good, I just have a few suggestions. Single-responsibility principle The method encrypt seems to have a lot of responsibility: Reads the input from the user Encrypts the input Outputs the result to the console One definition of SRP is "A class should have only one reason to change". But there are many reasons for ...

13

Single Responsibility My first piece of advice is to remember the Single Responsibility Principle. Take, for example, your login method in your UserInterface class, which is currently handling the rendering of the login inputs, checking whether it was a successful login, and then a host of bank account related operations. Instead, we ought to have something ...

13

The code looks perfect, except for the variable name numbers. This variable only holds a single number, therefore its name must be number instead. Since the variable is only used in a very small scope, the names n or i may be acceptable as well. (Subject to personal preference.)

12

Allowed? Yes. There seems to be nothing technically wrong with your code. Good practice? No. Specifically, in your example code setPddocument is not actually a setter. It is a load / initialize function, that does not set a value in the object, but instead performs an operation under the assumption a certain value (bytearray) has already been set. It would ...

12

Naming The names are not wrong, but I find them very verbose, which makes the code longer and harder to read and write. private double xPosition; private double yPosition; private final double particleRadius; It would be fine with x y and radius , and I see no risk of confusion as to the meaning of those names. This spreads further through your code when ...

12

Physics is wrong. The Brownian motion is isotropic. Your model is not. The steps in major directions are xIncrement and yIncrement ($I_x$ and $I_y$ respectively), but the steps along the diagonals are larger, $\sqrt{I_x^2 + I_y^2}$. The tests hide it because of the random initial positions. Shall you follow instructions and set the particles at the ...

12

It can be shortened and "beautified" by refactoring the bounds cyhecking and mine checking into internal method: private boolean isWithinBounds(int x, int y) { return x >= 0 && y >= 0 && x < width && y < height; } private boolean isMine(int x, int y) { return field[x][y] == MINE; } Then the mine count ...

12

Imagine a variant on FizzBuzz with $n$ different say-this-syllable-if rules. Would you want to check $2^n$ cases? That will lead to either a lot of code ($O(2^n)$ lines) or deeply nested code (with $O(n)$ maximum indentation). Luckily, the problem statement does you a favour: on divisibility by 15, it asks you to print FizzBuzz, not Fizz Buzz with a ...

11

I made some modifications to your View class and created this GUI. I couldn't get the Game class to cycle through generations. Maybe I missed something. I didn't see where the Game class needed any changes. Here are the changes I made to your View class. I started the GUI on the Event Dispatch Thread (EDT) by calling the SwingUtilities invokeLater ...

11

What especially jumps at me is, that you take your real data object (the Employee), convert it to a string representation and do your checks on the string. Why? Can't you check your data object? Apart from that, I don't see a problem with 5 and-conditions. This is clearer to read than some clever stream-through-predicates-and-reduce code. Clear. Simple. ...

11

Sorry to present the antithesis to an existing answer once more: if the string is neither reusable nor public, a constant makes no sense in my book. (And yes, this question is opinion-based ;-)) I see why you use a string-variable inside the medthod (to keep it readable for longer strings) which I find totally OK. In general: aim to write the code for the ...

10

In addition to Doi9t's valuable answer, I'd like to comment on overall design. Your Vectors class defines methods for mathematical vector operations. I question wether it's a good idea to extend java.util.Vector. How many of the methods you inherit from Vector are meaningful to your interpretation as a mathematical vector? Some examples: How often have you ...

10

Code formatting You have some formatting and indentation problems. Please run your code through a formatter, for example this one https://www.tutorialspoint.com/online_java_formatter.htm to make it more readable and follow standards. Logic while (playAgain = true) { This is wrong, it's supposed to be == , not = . It works anyway since what you do is just ...

10

Test cases are not convincing. I understand that the program doesn't crash, and by visual inspection of the code it seems to do the right things. However, it is pretty much impossible to check how the potential of 5000 random charges should look like. I'd rather see at least two elementary potentials: one of a single charge, and one of a dipole. No naked ...

10

Requirements Number of words Are you guaranteed to have at least 2 words? If so, you could simplify your loop, by removing the first read; the loop will take care of the second & subsequent words: String word2 = StdIn.readString(); boolean truth = true; while (!StdIn.isEmpty()) { String word1 = StdIn.readString(); truth = ...

10

Assuming Update#getMessage() is of type Message and Update#getCallbackQuery() is of type CallbackQuery: You can make a function getUpdateAttribute that receives higher-order functions as handlers for the situation where your update has a message or callback query as follows. private static <T> T getUpdateAttribute(Update update, ...

10

isPrime() Complexity This function contains a simple loop that iterates up to Math.sqrt(number) times, so assuming Math.sqrt(...) can be computed in $O(1)$ time, the function has $O(\sqrt N)$ time complexity. Review This function is terribly inefficient. Math.sqrt(number) is computed $\lfloor \sqrt N \rfloor$ times, yet the value is a constant. ...

10

return evensum == oddsum && evensum != 0 && oddsum != 0; Why can't sums be zero? With this array ... {-5, 7, 0, 5 } ... if you remove the 7, then the even elements -5 and 5 would sum to zero, and the odd elements 0 also sum to zero, giving a "balanced" array. Move tests out of loops wherever possible. for (int i = 0; i < arr.length;...

10

Code Style Use the standard JavaDoc markup when writing comments. You will be able to generate documentation and it allows you to use ready made tools for documenting stuff like method parameters and related classes. Also people expect to see JavaDoc style comments and are used to reading them, so by rolling out your own style you're adding unnecessary ...

10

Each one of the filterBasedOnConsist* methods look like they would be individual predicates themselves. So convert each method into a Predicate class and use the default and method to chain them together into a composite predicate: Predicate<String> employeePredicate = new FilterBasedOnConsistAge() .and(new FilterBasedOnConsistGender()) ....

10

Answering your first question, yes there is a better way. Efficiency doesn't matter in your case but in this instance the more efficient thing is also easier on the eyes. The usual idea would be to make a hash set of the elements, then you have O(1) look up of whether you have already inserted an elements, making the task O(n^2). I'm not that familiar with ...

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