New answers tagged

2

In addition to @Linny's answer, I can comment on the following topics: Use correct method names. getGrades method gets grade input and displays student's status. You should name it accordingly. Actually, your getGrades method does two different things which is a violation of Single Responsibility Principle. Dividing it into two methods such as getGrades and ...


5

Reduce code repetition / redundant checks Your if statements are redundant. If grade = 90, it already satisfies the grade >= 80, so just use that. No need to check if it equals 90 exactly because that's already covered. Instead of having four different lines where you show a message dialog, have a string that you assign the message to and print that at ...


1

The code as presented is poorly formatted, which makes it difficult to read. If this is how it is in your editor, then you should consider reformatting it to a standard indentation. Most IDEs / code editors can do this automatically for you. public class List { The list you've implemented is a linked list. This impacts on how it performs. Consider ...


2

For an interview, this is bad code. It is \$O(n \log n)\$. But it can be done in \$O(n)\$ time. Your code does not show your skills in that direction. Also, many interviewers (justifiably) will be unhappy for using streams. Streams may look cute but at scale like Gmail, they cause performance problems.


8

It solves the problem, and that's good. Period. But you're presenting your code here to get advice how to improve your programming skills, so here are a few remarks. Take them as a hint where to go from here, as soon as you feel ready for the "next level". Separate user interaction from computation Your main method contains both aspects in one ...


1

Naming I'm sure you already know this, but naming matters. The few keystrokes that you save using w are paid for every time somebody reads the code. Consider always using more meaningful names withdrawalAmount etc. Be cautious about reusing names of classes from the standard library. Rather than Consumer, consider giving it a more specific name to reflect ...


0

Quite often with HackerRank puzzles, there will be a check that your algorithm scales to large sizes. Here we are explicitly told that n may be up to 199 999. You have managed to come up with a homebrew sort that appears to be efficient given the data constraints. However, you are calling it in the outer loop, so your algorithm appears to be O(n^2) overall. ...


5

If I test this method using an ID number which is less than 13 digits long, I will get en exception. I think that should be explicitly handled.


2

I've been thinking about this idea of "who owns the interface?" recently; hence my finding this post. I think something that might be missing from the conversation is another diagram from Uncle Bob's Clean Architecture. You can ignore the details of the particular interfaces and classes in the below image, as nobody is required to design systems ...


3

For a first application in Java, good job! I have some suggestions for your code. When using regex, try to uses the java.util.regex.Pattern class instead of the java.lang.String#matches method. When using the java.lang.String#matches method, the regex pattern is recompiled each time the method is called. This can cause a slowdown in your method execution. ...


2

While I do agree with the others that a loop is the simpler solution in this situation, I'd nevertheless like demonstrate a Stream solution, because IMO the noted disadvantages are not due to streams and functional programming in general, but due to Java's limited concept and implementations of streams. A functional way would be extend the stream of ...


3

As mtj already mentioned in the comment, adding stream trickery to fulfill this need only makes the code less readable. Streams are nifty, exciting and popular but they are not always the right tool for the job. Sometimes a plain loop is still the best choice. The common pattern in padding a collection (well, most often it's a string that gets padded) to ...


1

It depends on your needs, but generally I would say that the second approach would be better. Why? because it allows you to run multiple tasks and that gives flexibility to your system. Now the use of synchronized may be troublesome if there are many threads running, if you don't need to handle static or shared resources/variables it would be better to not ...


2

The most expensive operation is the addToKey x that adds x to all keys in map, because substantially you have to create a new entry key, value + x in your hashmap and delete the old entry key, value. To avoid the need of caching the old entry while iterating over the map, you can distinguish two cases: x > 0, then if you have iterate over a keyset ordered ...


1

I suggest avoiding static state. It makes your code more rigid. It works for this very simple app, but the limitations become more apparent in more complex apps. You should remove the static keywords from your two lists and the runPlaylist method. You can instantiate an instance of the class to use in main() and call public methods on it. Some of your ...


2

First of all, to get that out of the way, I despise var and static imports. For me, it makes the code an unreadeable mess that looks like javascript. You would not be allowed to do that on my team. Now regarding the concrete code: It is not a CSV reader. For real csv, you'd need a quote character and a way to escape the quote. Usually this is done via ...


2

I have some suggestions for your code. Use a conventional name for the static factory method name In my opinion, the of name generally aggregate a given set of data into a container; this can cause confusion in this case. I suggest to rename the method to create or newInstance. public static <T> CSVReader<T> of(BufferedReader reader, Class<T&...


2

I have some suggestions for you. Extract some of the logic to methods. In your code, when the query is insert and get, you have two big blocks of code that are similar; you can extract to a method and reuse the method in both sections. I suggest a method that returns a boolean based on the if condition, so you will be able to set the currValue and currKey ...


1

Naming is important When you're in the flow and writing code you'll typically remember what variables refer to and you'll often be able to hold the whole code context in your head, particularly with relatively small examples like this. However, as the code gets bigger / after you've stepped away from it for a while having good names really makes the code ...


2

Just a few quick things that jump out at me (I didn't go over all of it): Using static for everything is awkward. Usually you want to avoid storing state in statics. I know this is a simple app without multiple classes so you can get away with it here. But following more typical rules of thumb, you would make all these variables and methods non-static. If ...


2

I suspect the root of your problem is that you're working at the wrong abstraction level. Specifically, looking at the Telegram Bot API documentation, it seems that an Update is just a wrapper for one of several unrelated objects that describe different kinds of events that your application might want to react to. Trying to treat all of these events as if ...


2

Code Structuring All Android tutorial code packs event listeners into anonymous inner classes, because the examples are usually short and it is convenient, from distribution point of view, to pack all code into the same file. It is however not the correct way to implement production code, as it creates large and messy compilation units that contain many many ...


4

The other solutions gave an answer which is perfect for two classes. If you get to much functions//classes beside CallBack and Message, the code below might give you a better solution. Wrapper I personally would create a wrapper with a common interface. The reason is that this is not very extendable (adding a new class requires rewriting all the common ...


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


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


1

I have some suggestions for you, good job on the code! Always add the empty diamond operator, even if defined in the left-hand side If you don’t add the diamond, Java will use the old raw types instead of the generic types; those are only kept for compatibility. Here is a good explanation with more details: https://stackoverflow.com/a/4167148/12511456 Before ...


3

A comment on the DDL Despite this using MySQL (which would never be my first choice), the schema seems pretty sane; good job. This surprised me: removed_date date default null but then, reading the documentation, MySQL does a nonsensical thing by default and uses the "zero" value for a date as its default; so what you've done is correct. For these ...


1

I have some suggestions to make the code cleaner, not faster. Always try to pass the size of the maximum size to the Collection / Map when known The ArrayList has a default size of 10 elements, if you have more elements, the list will have to resize its internal pool. By setting the size, you can prevent the resize and make your code faster. //[...] int ...


2

In my opinion, there are two ways with Spring to do that; your way is one of them (using the Spring Resource), and the other is to inject the file bytes directly in the javax.servlet.http.HttpServletResponse. Your version can be improved, since you can use the FileSystemResource instead of the InputStreamResource. The InputStreamResource is good when there ...


3

Compilation Error The first problem is to solve the compiling error. Just follow the interface: public Node<Integer> mergeAscend(Node<Integer> a, Node<Integer> b) Warnings and Code convention You should add the type information to the store_next_a_node variable (and store_next_b_node). Additionally, in Java naming conversion, variable ...


2

I see you used your custom ScraperException exception in two different modes: the first is below: private static List<String> noEmptyElseThrow(final List<String> weeks) throws ScraperException{ if (weeks.isEmpty()) { throw new ScraperException("Please provide a historical time range! Cannot rank otherwise!"); } else { ...


0

As it happens, there is already an implementation of Karatsuba multiplication in the implementation of BigInteger. Of course that's integer multiplication instead of polynomial multiplication, but they're very similar, apart from how they handle carries. You can read the source here, look for multiplyKaratsuba. It's more of a high-level implementation, ...


2

Since you are constructing a full, 32-bit word from random bits, you could skip the niceties and simply generate a 32-bit random number as your p. What is unclear is how the edge cases would behave. For instance, I would have used an unsigned int instead of an int to hold your four values (0-255), as the leftmost one (R) might mess with the sign of p. etc....


5

You can write arrays of Integers directly on the WritableRaster of the BufferedImage: public static BufferedImage createRandomImage(final int width, final int height) { final BufferedImage result = new BufferedImage(width, height, BufferedImage.TYPE_INT_ARGB); final long bytesPerPixel = 4L; final int[] pixelData = new SplittableRandom().ints(...


-1

In regards to speed, I'm not seeing a whole lot you could improve. One thing that comes to mind is the use of lower level APIs -- i.e. don't use File and BufferedImage. You could, for example, use a DataOutputStream (https://docs.oracle.com/javase/10/docs/api/java/io/DataOutputStream.html) which, from my understanding, is one of Java's lowest level APIs for ...


7

Some style issues first: Package names should be all lowercase ASCII letters. No camelCase, PascalCase, snake_case or kebab-case. So tennisproject and scanner. Local variables should never be uppercase SNAKE_CASE, but camelCase. So atpUrlPrefix instead of ATP_URL_PREFIX and so on. You probably want those to be class constants anyways, which use uppercase ...


2

Welcome to Code Review. I run your code and the game works well, probably there is something you have to adjust about strategy, because it does not always choose the best move to win the match or at least tie. In your code there are some repetitions about method to check if the player or the computer wins like your code below: private static boolean ...


2

You've got some answers to your initial questions, which you seem to be happy with, so I'm not going to address them. There are some things that I noticed about your code, however which I think are worth mentioning. Tests Naming Consider losing 'test' from the front of your test names. All the methods are annotated and in test class. Having the word test ...


2

So, you would like to know if your code is "following the Single Responsibility SOLID principle". S.O.L.I.D. stand for Single responsibility, Open-closed principle, Liskov substitution principle, Interface segregation and Dependency Inversion. Single responsibility One class have a single responsibility. But both Payroll and Employees are mixing ...


0

I would not give you the exact solution to this problem rather I would help you to get an intuition on how to solve this question. If we try to generalize count of the number of times a particular number at index i is getting added and the number of times it is being subtracted then for every index i we can use that mathematically derived formula to compute ...


1

I have some suggestions. Extract some of the logic to methods. In your code, I see at least three sections of code that could be in methods. In my opinion, those extraction will help with the reading and make the code a bit shorter. The validation of the parameters. Before if (string == null || target == null || string.length() == 0 || target.length() == 0 ...


1

First off I think your code can be adjusted: Booleans to keep track of even or odd size of matrix and even or odd numbered rows allows you to use one set of loops and simply change which set of numbers are printed. To test for odd or even I like (num & 1) if the result is 0 it's even, 1 it's odd. I think using modulus for this is inefficient. Changing ...


3

First of all, I think the algorithm is pretty smart which is, for my humble experience, not so common for a college student. So congrats if you came up with it by yourself! If you're looking for smart implementations I would recommend functional ones, e.g. in Haskell; see also Shortest game of life. Now, beware of smartness. A good code should be easy to ...


2

Many folks find that tacking on { } braces to even a single-line if body is a useful way of preventing future bugs. You wrote: array[0] + maxSubsetSumNoAdjacent(Arrays.copyOfRange(array, 2, array.length)), array[1] + maxSubsetSumNoAdjacent(Arrays.copyOfRange(array, 3, array.length)) The pair of copy statements is wasteful. You're allocating temp ...


3

The first thing I noticed. The calendar in the constructor. If you wanted to test this class it would be really difficult because you are dependent on the Calendar implementation, and you have no control over the value it will return. It would be better to inject the calendar in to the constructor, or Abstract the desired functionality in to an interface and ...


1

I have some suggestion for the Java version Always try to pass the size of the maximum size to the Collection / Map constructor when known The map has a default size of 16 elements, if you have more elements, the map will have to resize its internal cache. By setting the size, you can prevent the resize and make your code faster. In this case, you can set ...


2

I have one suggestion for the Java version. Instead of using java.util.Map#containsKey, you can use java.util.Map#get. In your code, you can make it short and faster by fetching the value and checking the nullity. Before if (map.containsKey(target - nums[index])) { //[...] indices[0] = map.get(target - nums[index]); //[...] } After Integer value = ...


2

Like the other said in the comments, it's a bit hard to do a code review without context; I have a suggestion for you based on the current code. Since you seem to use the Apache Commons - lang3 library, you can use the org.apache.commons.lang3.BooleanUtils#toBoolean method to convert the string boolean. This method will return false when empty or null. ...


3

I have some suggestions. Don’t edit the parameter values, create your own instance / copy In my opinion, this is a bad habit, since some objects in Java are not immutable (Collections, Date, ect) when passed as parameters, you will edit the original instance of the caller. In your code, since the string is immutable, you are fine, but keep this in mind. ...


3

Methods vs. Functions None of your three solutions are object-oriented. Note: there is nothing wrong with that, in fact, the problem domain is really anemic, so there is not much to model with objects anyway. However, not being object-oriented, there is no need for objects here. In all three versions you use instance methods (or member functions as C++ calls ...


Top 50 recent answers are included