New answers tagged

0

I'm probably not polished enough with Java to propose new code, so here are some abstract thoughts anyone else can use: Fewer bigger writes will generally be more efficient. Disk access is heavily optimized at the OS level, and those optimizations will work best if the number of things you're doing (from the disk's perspective) is smaller. It seems likely ...


1

Seeing you hyperlinked Princeton Algorithms, 4th Ed. resources, I assume you know how to view their take of Prim's. I do not understand your concern about terseness. There is one striking difference between the code you present for review and that of edu.princeton.cs.algs4.Edge and EdgeWeightedGraph it uses: Theirs is documented using the standard mechanism -...


1

There's a lot of code here, so here's a few things to get started with... Comments Consider JavaDoc for function description comments that the clients of your library might want to know, such as // if width is 0 then the frame is maximized horizontally. There's a good chance that their IDE will pick them up and give them hints when they're calling your ...


1

Eric Stein and Azahe have covered many points already. I won't repeat those here, although some of their points may be reflected in my code. Decomposition This code is made up of one large function. The function does a lot of things, any of which we might want to pull out and used separately. For example, both the hour and minute hand angles are computed ...


1

Comments - there is too many of them, usually in places where you could extract code to the well-named method (or just remove comments), try to make the comments unnecessary by improving the code structure - leaving only the ones that explain something not obvious (not needed here) e.g. consider value of: //turning strings to doubles hour = Double....


2

Declare variables close to where they're used. hour and min are declared and then ignored for half the body of the method. They can be assigned at declaration time, rather than being set to 0 first. And consider making them final, as they should not be reassigned. sep can also be assigned later. It is unclear to me why hour and min are doubles and not ...


1

Steps to solve this problem: Get slice of array related to season Sort slice (we could use custom for loop to get min and max but sorting is quite similar in performance and takes one line instead of few) Get first and last value from sorted slice Calculate amplitude and save this result in Array or List Example implementation (C#): using System; using ...


1

Don't create Rube Goldberg machines. There is an easier way to accomplish what you want: public static <T> List<T> getCopyOfList(List<T> list) { return new ArrayList<T>(list); }


1

This code does not do what you want it to do, there is no check for the minimum distance. I'm not going to review code that does not exist. However, I will review the rest of the code. Naming conventions Variable names should be descriptive. int[] a does not pass that requirement. numbers, values or even just arr would've been better. Note that x and y are ...


10

Given that arrayCopy has exactly one line, it's not clear that there's much value in having it extracted as a method. toArray() is documented to return a safe array, so there is no need to copy it. getCopyOfList returns a fixed-length list without documenting that fact. Clients may be surprised that optional List methods such as add() and remove() are not ...


3

Mixed concerns Rather than just letting the client create instances of the SortedListMerger class, there's a private constructor and a singleton map construction mechanic. If you really need to be able to construct the merger, based around the runtime class information then consider moving this responsibility out to a different class. This would allow the ...


3

Template Type Parameter You've used NodeDataType for your LinkedList template type parameter, and DataType for your LinkedListNode template type parameter. Why are these different? Considering your code is littered with LinkedList<NodeDataType>.LinkedListNode<NodeDataType> types, it seems you want the same type used for both classes. But if you ...


2

Some improvement ideas: You could consider using java.time api, however to use DateTimeFormatter you'd have to use builder, quite heavily configure it (as it would require defaults for year and day, not to mention its case sensitive) and Month class does not seem to offer anything to help here - the benefit of this approach would be that you don't have to ...


1

int set_size = (int) Math.pow(2, N); is bound to result in zero for arr longer than Integer.size. (You use 1 << j later on: why switch?) inserting all elements of the subarray in the innerList is in dire need of explanation: "the subarray" is specified by (bits set in) i. You posted an XY-problem: The "real" problem seems to be to ...


0

Class#newInstance is deprecated as it does not handle the checked exceptions of the default constructor call. So getting the default constructor (no arguments) and using Constructor#newInstance is the correct way. Only problem: there might not exist a default constructor, only some constructor with parameters. public static Object instantiate(String ...


2

In opposition to (just posted) answer @TorbenPutkonen - I think that separating validation from data is exactly against OOP principles (cohesion and encapsulation) and leads to massive duplication of if (something.isValid) checks and ultimately (from my experience) harder to maintain and secure code. If you indeed desire to have validation of separate fields ...


2

Frankly, I have a hard time trying figuring out how the validator is supposed to work. Either the code needs a lot of documentation or it requires comprehensive redesigning. I don't understand the purpose of the toFirstError methods. The names suggest that they perform a validation action, but instead they are just static replacements for the constructors. ...


4

The concept is odd to me and I've made a comment about clarifications. In the mean time... a few pointers about code style. The method name getClass suggests that you are getting a class, when in fact you are returning an instance. To better convey the purpose of the method, you should rename it to instantiateClass or newInstance etc. The name prefx my (as ...


2

Some of my suggestions won't necessarily make sense for this specific example, but for the general idea of counting occurrences of a set of characters in a string, they might scale better to bigger workloads. public class StringUtils { private static final Set<Character> ENGLISH_VOWELS = new HashSet<>('a', 'e', 'i', 'o', 'u', 'A', 'E', 'I', '...


5

Functions... When you're writing code, even when you're just playing about, it's usually a good idea to put the code into a function. This helps you to think about what the interface to the code should be, what it's dependencies are and what it should be called. Based on the code, I'd go with a function declaration something like: Map<String, Integer>...


4

Welcome to Stack Review, the first thing I see in the code is the following: List<String> VOWELS = Arrays.asList("a", "e", "i", "o", "u"); This is correct, but from the uppercase VOWELS name you chose for the variable it seems me you meant this as a constant, so you could declare it : private static ...


1

null Nulls can be useful, however they can also be more trouble than their worth. Do you really need to use null, or could you get away with using "" instead? Consider the impact this would have on your code for simplifying statements like this: if(aliasFirstName == null || aliasFirstName.length() == 0) Multiple middle names Reading the test cases, ...


3

your code looks fine - some minor issues: test driven developement if you have clear requirements, write test to validate them. "The method should pass the following tests" i hope you have these tests and just forgot to add them on the code review. exception handling instead of returning null you should throw an excpetion, namely ...


2

We can use YearMonth in Java 8 or later like below: YearMonth .parse(yearMonth, DateTimeFormatter.ofPattern(MMYYYY)) .format(DateTimeFormatter.ofPattern(MMYY));


0

Parsing a 1GB file The program reads the input file line by line. To be able to read a line, enough memory is needed to store it in memory. If all the lines in the file are guaranteed to be no longer than what fits in the memory available to your program, then it should work fine, as far as file size is concerned. (But see "Potential bugs" below...)...


0

It's important to have fraud detection, use libraries, validate tld, etc. However, you can achieve the latter using a complicated regex found here: [a-zA-Z0-9.!#$%&’*+/=?^_`{|}~-]+@[A-Za-z0-9.-]+\.+(xn--vermgensberatung-pwb|xn--vermgensberater-ctb|xn--clchc0ea0b2g2a9gcd|xn--w4r85el8fhu5dnra|travelersinsurance|northwesternmutual|xn--xkc2dl3a5ee0h|xn--...


5

When approaching optimization problems (and it kinda is one as you want to make a call faster) its generally best to first make sure that you understand exactly what is happening so when in doubt - measure and debug. Before doing any changes in the code I'd first set up debugger and probably some logs with timestamps (assuming you are testing on separate ...


0

After some more research and thinking, it turned out that both of the above approaches are wrong ;-] The reason for both cases is that user Filters may put HttpSession object into a wrapper, so neither synchronizing on it nor using it as a key in a ConcurrentHashMap is safe :/ Furthermore, in case of "maintaining a static ConcurrentMap" solution, ...


0

I had answered the question, but my previous answer contained some factual errors. Thanks to chepner, Eric Stein and JDługosz for the corrections, and additional info about Narrative Dice which have symbols on the die faces instead of numbers. You have plenty of answers about regular expressions. Instead, I will focus on the overall object oriented design. ...


0

Since you are subtracting squares, your loop won't have to execute very many times. Every step shrinks the number of yards. I originally thought you might want to optimize it for when there are multiple values like 1 and 4, but I just did a quick test and for 1 to 10 million you never get more than 7 elements, and 3 of those are 1s. There will never be more ...


2

First Welcome to CodeReview. Some comments on your code: The code need a clear indentation, most IDE can help formatting code easily with one shortcut. You may separate sections with new line, it will ease readability, also it may help to spot new method candidate. If your see a value repeated many times in your method/class, it's preferable to declare it ...


1

A couple improvements I've made, plus I bug I didn't notice. To ease lexeme's offsets retrieval, we provide destructuring capability: interface Lexeme { ... // Provide destructuring capability operator fun component1(): Int = getStartOffset() operator fun component2(): Int = getEndOffset() } We can optimize the insertion when the inserted lexeme's ...


6

I'm no Java guy, but will comment on the "algorithm". You compute and subtract squares all the way until you reach zero. You can get away with doing just one square and very modest amount of precomputation/memory. Note that squares aren't that far apart. In the allowed range 1 to a million, the largest gap is \$1000^2-999^2=1999\$. Consequently, ...


1

About the difference between Perl and Java If this class will be used a lot, there are a couple other things you should probably do: Compile the patterns. Don't use the convenience methods in String. Use java.util.regex.Pattern explicitly and cache it in a static. Thus: private final static Pattern INPUT_VALIDATION_PATTERN = Pattern.compile(....); (...


-1

An approach to elimitate the Math.sqrt call (which is rather expensive) is using dynamic programming to calculate the squares up until you reach or overshoot the value. Afterwards, you'll have them readily available. A TreeMap looks like a good data structure as it is navigable: // something along the lines of: TreeMap<Integer, Integer> squareToRoot =...


7

A note about Math.sqrt I feel as though Math.sqrt() can be a bit slow I thought, since Math.sqrt computes a precise double, maybe if you implement a custom sqrt that computes just up to int precision, it might be an improvement. However, looking at the implementation of Math.sqrt in my IDE I see this comment: // Note that hardware sqrt instructions /...


1

Given it seems to be princeton.cs.algs4 course task I am not entirely sure what would be the best answer here. I'd assume you are suppose to learn and learning limited number of things at a time (here DFS and euler cycles?) is pretty good practice, so in terms of what purpose does this code serve if you wrote it, it works and you understand why - it seems ...


0

In addition to other comments, I would suggest a change in the way you structure this program. Your functions that perform calculations and then print the output will be difficult to write automated tests for. As a first pass, I would modify your classifications function to return a String, and then perform the println from the calling function. You can then ...


3

From the design standpoint it seems you are mixing two things - abstraction of a die (that has certain properties like number of faces) and has some behavior associated with it (as it can be rolled for certain number to be a result) AND parsing (even though you call it differently see: https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/) ...


4

As others have mentioned, there is a bug in the pattern - see their answers for assistance on fixing it ;) Apart from this, I'd like to remind you to refactor your tests as well. The repeated pattern in the tests, along with the relatively large amount of boilerplate, make it hard to distinguish if there are any cases you have missed or if there are ...


4

public void bmiCalculation(double height, double weight) { // bmi algo = BMI = weight / height ** 2 * 703 double newHeight = 1; double bmi = 0; for (int i = 0; i < 2; i ++) { newHeight *= height; } bmi = weight / newHeight * 703; println(bmi); classifications(bmi); } Seems overly complicated to me. You can one line it (and maybe ...


1

Review Your solution requires a retain and release working on a state, a private reference counting field. This code repeats and hence should not be done this way (DRY, don't repeat yourself). You could have a common base class instead. This however is a requirement that does not work nice with single-inheritance java. (Interfaces with default methods does ...


1

Take a look at almson-refcount. It pulls out Netty's reference counting facility into a separate library and adds quality-of-life refactorings. Regarding the original question, my solution is to simply have a base class that has the reference count and common methods.


6

You can use named groups in the pattern, as well, which will make the code less brittle to pattern changes. (Example: If you want to add in the feature of multiplication in the future then you could have, e.g., 3d6x2+1 meaning that the addition part would be the 4th matching group instead of the 3rd, and if you refer to groups by order you would have to ...


9

We don't know how your Die fits into the game as a whole, but the way you're rolling your results seems counter-intuitive to me. I'd expect to be able to call result and get a unique value each time. However, if I roll 5D6+2, I'll always get the same answer (22 on my system), because it's always creating a new Random with a known seed (0). Consider moving ...


12

Bug in the validation pattern The INPUT_VALIDATION_PATTERN doesn't work correctly, as it matches the invalid input "D". This is due to the pattern looking for any number of digits (*) after the D. Changing the pattern to look for at least one occurrence (+) seems to fix this. Simple solution using capture groups After some fiddling around with your ...


2

@aghast makes many good points, but I'd add a few more. Firstly, the string being printed can be simplified. In the first half of the processing, a single period character is being cleared at each step, so "\b \b" could be used. In the second half, we're just adding one period... Secondly, on Unix-like systems standard output is line-buffered, so ...


3

Write a function. You have code that is repetitious? Write a function to hide it away! private static void show_string(String s) { try { Thread.sleep(200); } catch (InterruptedException e) { e.printStackTrace(); } System.out.print(s); } In fact, write several functions! Use function names to explain what you are doing. Also, ...


4

Very nice piece of code, thank you very musch for sharing your work with us! i tried to list the issued order by severity. segregation of concerns / composition over inheritance / MVC Your displaying class GameGrid extends from Game. That violates all principles in different ways. It would be easy to increase loose coupling by defining an interface in the ...


1

The algorithm is inefficient. For each character, it checks if it's repeated by checking all following characters. The time complexity is \$O(n^2)\$, space complexity is \$O(n)\$. You can do better. Consider this alternative, and very simple algorithm: Build a map of counts from the characters in the input. Loop over the characters in the input again, and ...


Top 50 recent answers are included