Hot answers tagged

39

What if I add a class Kelvin without modifying the other classes? When I convert Celsius to Kelvin then, your code says that 20 °C = 20 K, which is simply wrong. Your whole approach is doomed to fail. What you should do instead is to define one reference temperature scale. I would take Kelvin for that. Then, for every other temperature scale, have methods ...


27

Let's look for some patterns in your code: First of all, we notice that you have three parameters with similar names. condition, condition2 and condition3. These names indicates a code smell, that we should use an array or list instead. Taking one step at a time, we see this code is repeated multiple times: if (StringUtils.isNotEmpty(condition.toString()))...


26

The algorithm This algorithm creates every combination of ships and tests them one by one, including combinations that could be seen to be useless by the first choice made. For example, if the first choice is to assign a unit of weight 60 to a ship with capacity 50, then it no longer matters what the rest of the content of arrayWithGeneratedNumbers will be, ...


21

Let's take this from the top down: /** * Title: GameFunctions * Author: Dev * Date: 6/1/2019 * Purpose: This class contains - * -all the functions for the game * */ public class GameFunctions This comment doesn't tell me anything new: Title is already mentioned in the class declaration that follows Author and Date are already kept in version control ...


21

Usability I have a remark about defining recursive functions. You don't want consumers of your API to think about the intermediate variables and their initial value. The consumer does not care you are using recursion internally. Make your recursive function private. private static int maxPlayer1(int[] a, int currSum, int sum, int start, int len) And ...


20

Using 2 classes here doesn't make sense. Every time you add another class, you'll have to modify every classes conversion method. There is a tight coupling between Celsius & Fahrenheit. You are using an instance of Fahrenheit to determine the type you want to convert to. This isn't necessary. Note: In programming you want low coupling, high cohesion. ...


19

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)...


18

Other answers correctly point out that using primitive types and not mixing them with Objects (=int instead of Integer), reducing visibility wherever possible (=adding private modifier to position and list) and preventing popping from empty stack are good practices. There are a few more subtleties which can be improved: Rename INCREMENTSIZE to ...


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 ...


16

Next time you're in such a scenario, ask them to clarify what "academic" means. For me it means a solution that concentrates on the essence of data structure, not an implementation of a specific API. But we probably never know so let's not concentrate on that. Instead, let's look at the red flags... (I have ignored the comments as I do not understand the ...


14

Write useful comments carryingCapacities = new ArrayList<Integer>(); // create arraylist for the carrying capacities of ships strengths = new ArrayList<Integer>(); //create arraylists for strengths and weights of units weights = new ArrayList<Integer>(); //create arraylists for weights sc = new Scanner(new File(input)); // load my input ...


13

size++ should be extracted from both branches and called only once there is no need to cache instance variables where local method variables should be used instead: newNode,oldNode,displayNode prefer while(currentNode != null) over while(true) and currentNode should start with firstNode and be chained inside the while-loop as currentNode = currentNode.next ...


13

Design The problem with getName is already mentioned in the comments. firstName + lastName is at least missing a space. Note that there are multiple ways of creating a full name - you may want to leave the construction of a full name to the caller. For instance, Germans are a bit more formal and may prefer "lastname, firstname". Usually initials are also an ...


13

First, a Code Review Anti-pattern Please don't do this: record = new String(); This is (barring compiler heroics), creating a brand new empty string literal, when a perfectly good interned string literal is available: record = ""; Using this interned literal "" prevents the creation of a number of identical tiny objects, and will improve the ...


13

To answer your comment, yes there are equivalent of PEP8 in java. I suggest checkstyle, this plugin works in lots of IDE / text editors and can be configured with your code style. Code review reverse_string method In java, we try to use the upper-case version of Snake case only on constants / enums. For the methods and variables, I suggest that you use ...


12

This looks good, However I suggest you properly indent the code. Further suggestions: I would change following private final int INCREMENTSIZE = 1024; to private static final int INCREMENTSIZE = 1024; Since you are not changing this in the constructor (to a new value) we might as well make it unique for the whole class. I would change following ...


12

Positive Changes getReader() is only called once, while called multiple times before. There are more early exits as before, which results in less nested and lesser deepened nested statements. Method getPdfStreamUf hides the fallback method away from the main method, this functionality deserves its own method. Negative Changes I agree with the developer ...


12

iA and iB can only take the values 0 and 1, so that if ((1 == iA || 1 == iB) && (1 != iA || 1 != iB)) { ... } can be simplified to if (iA != iB) { ... } Boolean variables would be more appropriate for this purpose, and the variable names can be improved, e.g. boolean containsA = false; boolean containsB = false;


11

Use your brain, not brute force. How would you solve the problem if it were presented in a math test, and you couldn't use a computer? Your solution requires \$O(x)\$ time; it should be possible in \$O(1)\$.


11

Your class is small and does what you said it should, that's good. However, there are a few things you could do better. 1. Use primitive types unless the boxed ones are specifically needed: You are using the Integer type for counting the position and storing/returning data. If you actually used the fact, that it could be null, this would be acceptable use. ...


11

BinarySearch The BinarySearch class should be public since it contains utility methods that are generally useful. To do this, write public class BinarySearch instead of class BinarySearch. The binSearch method should be static since it does not access any fields from the BinarySearch class. After all, that class doesn't have any fields that could be ...


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 ...


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

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 ...


10

No and no. This is a very inefficient way to reverse, for example, a dynamic array based list (it's a different story with a linked list). With a dynamic array based (e.g. ArrayList) The remove(0) operation will require moving every other element in the list (linear in the length), so the whole method is quadratic in the total length of the list. It also ...


10

Java doesn't handle recursion very well. And by not very well I mean it should be avoided almost every time where you cannot limit the depth to something small. To show you how bad it is let's take a look at this simple program: public static void main(String[] args) { for(int n = 0; n < 100_000; n+=100) { List<Integer> list = new ...


10

You might follow the example of java.util.Date. The internal representation of a Date is always in UTC, and it's converted to other time zones and date formats as needed. Similarly, you could define a Temperature class that is always in Kelvin, and a TemperatureScale interface with as many implementations as you like to perform conversions and formatting.


10

In java is the preferred way of curly brace placing on the same line like you can see in code snippets in Oracles Style Guide. reverse_string has a type embedded in its name which leads to a code smell. The disadvantage is that if you want to change the type of the parameter you have to change the method name too. Additional the invocation looks messy and ...


10

Are you searching for letters ('A' and 'B') or strings ("A" and "B")? You title says "pair of Strings in a List", but your code's variables are letter and letters. You have hard-coded the search for the strings "A" and "B". Are these the only two you will look for? Might you want to search for "C" and "D"? Is the condition "all" or "none", in which case,...


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