# Tag Info

3

You have the possibility of an infinite loop. Consider: logBase10Estimate(2147483647). Should be about 9. We need a value of $10^i$ which is greater than 1000000000. So, what values does x take on? 10 100 1000 10000 100000 1000000 10000000 100000000 1000000000 1410065408 1215752192 -727379968 1316134912 ...

2

The printing should not be there :) You don't need another variable x. You can modify the local copy n, because you won't need its original value for the entire algorithm and because it is passed by value, you won't change the value for the caller. Also check for valid input. static int logBase10Estimate(int n) { if (n <= 0) { throw new ...

3

Adding to what @dariosicily mentioned already: Performance If you just need to find out a part-of-speech of each word and do not need to build a phrase-structure tree of sentences, you need only to specify 3 annotations (without parse): props.setProperty("annotators", "tokenize,ssplit,pos"); I assume this can give you a significant boost in performance. ...

4

Welcome to Code Review, here some suggestions about your code: public class review { ... } Java classnames always begin with uppercase letter so rename it to Review. private ArrayList<String> restaurants = new ArrayList<String>(); In java language it is preferable using if possible an interface like List on the left part of the assignment so ...

2

You've got inefficiency here: // Sum of a subarray, based on B(x, i, L) -- i is one-indexing public static double sum(double[] x, int i, int L) { return IntStream.range(i, i + L) .parallel() .mapToDouble(idx -> x[idx - 1]) .sum(); } I don't know how big L is, but ...

1

I start from mathematical consideration using one of the examples you provided: Input: 10 output: 3 2,3,4,5,6,8,9,10 7 11 All the elements multiple of 2 are in the set containing 2, the other sets will always contain just one prime number like {7} and {11} : if it were not so the number would be not prime and would be contained in another previous set. ...

3

In addition to the valuable answer already given: use JavaDoc for comments describing method/class do not use float or double for monetary values (calculation inaccuracies!) use existing collection implementations of Java, as ArrayList or at least wrap them learn the advantages of Single Responsibility Principle (SRP) Here some improvements on modelling ...

1

I have some advices for you. Add the static keyword to the DEFAULT_CACHE_SIZE variable (all cap snake case = constant). Use the size of the map instead of using the capacity variable You can use java.util.HashMap#size. In my opinion, it will be less error-prone to do so (If you forget to update it, ect). Avoid using C-style array declaration In the main ...

1

After applying the suggestions the function has changed as follows. static final double ln2; static final int twoExpLimit; static final double precision; static { int k = 0; int n = Integer.MAX_VALUE; while (n > 1) { n /= 2; k++; } twoExpLimit = k; // WolframAlpha ...

6

To speed it up a fraction more add double dIteration = aIteration/cIteration; and double d = a/c; before the loop. In the loop remove a and c and insert d*= dIteration; ln += d/b; You then have one multiplication, one division and an addition in the loop, cutting out two excess multiplications in your version. Edit: you can get further ...

6

Since it's always $n^2$ in the second version, by not using java.lang.Math#pow ($n * n$), you can save computation time. It takes me ±3k nanoseconds instead of the ±20k nanoseconds with the java.lang.Math#pow. static double lnApproximationOptimized(double x, int n) { double a = x - 1; double aIteration = a * a; double b = 1d; ...

1

Hellow K.H. thanks for the answer. I rewrote the code using Dagger 2 for dependency injection and following your advice. Could you take a look at this code and answer a couple of questions: How to correctly pass name to JsonSharedPreferences (Make a setter, make a module that will provide name). Is it correct to select this code as a separate component or ...

1

Why saving reference to Context and then create SharedPreferences each time? There's duplicate code for creating SharedPreferences You could just use context in constructor to create member SharedPreferences to use in your methods. If you want to keep it as it is, at least extract duplicate code into separate method. I am missing null checks. I expect to get ...

3

My review is focused about simplification of your code using java DoubleStream and DoubleSummaryStatistics classes; I found some repeated operation in your code that can be avoided to slightly increase efficiency. You have the following methods in your code: public static double sum(double[] x) { double s = 0; for (int i = 0; i < x.length; i++) ...

3

Four things come to mind immediately: (1) You’re doing a lot of memory allocations. (2) You’re doing a lot of copying of arrays. (3) You have some unnecessary calls to pow() (which is very slow). (4) You’re actually computing a factorial. I think we can take care of (1) and (2) together. You have a lot of calls to B() that can eliminated if you ...

2

Firstly, try to make sure that code you post here compiles. Now you have missing semicolons on the lines List<String> distinct1 = Arrays.asList("1", "2", "3", "4", "5", "6", "7", "2") List<String> distinct2 = Arrays.asList["3", "3", "2", "4", "7"] and also the last of these two lines has [...] instead of (...). It was also a bit difficult to ...

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

3

There is NO heal() method in class IO...

1

Since often browsers also support backslashes, you can also consider this: public static String addTrailingSlashIfMissing(String str) { String slashChar = str.contains("\\") ? "\\" : "/"; return str.endsWith(slashChar) ? str : str + slashChar; } which can be also useful for file system paths.

3

I agree with @mtj's answer : YAGNI, no point in making the string reusable if it's not being reused at the moment. I would want to add that in certain scenarios it may be smart to factor out only part of the SQL query. For example if you had a few queries with slightly more complicated WHERE statements, something like : select * from image where (...

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

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

3

One option would be to do it with Streams. This is assuming that all different filters are in the same class as the filter method: private boolean filter(Employee employee) { Stream<Predicate<String>> filters = Stream.of( this::filterBasedOnConsistAge, this::filterBasedOnConsistGender, this::filterBasedOnConsistNationality, ...

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

4

Not many valid reasons, why would ever isRecordExists2 be better method. It is almost always good to extract reusable strings into constants and split code into smaller pieces. I definitely vote for isRecordExists. If you decide to stick with isRecordExists2, why even create sqlQuery variable? You might as well just pass the string itself into queryForObject....

1

Yes, your approach is correct. However, you parallelize various queries, which probably go to a remote backing database. As you told us nothing about the runtime of these queries, the nature of the database, the complexity of the underlying statements, I can only ask you to measure. Parallelism induces overhead - in the runtime AND in program complexity. ...

3

Prime Generation As mentioned by vnp, use the Sieve of Eratosthenese. In that implementation, use a BitSet(1_000_000) for efficient memory usage during your sieve; a sieve for primes up to one million will only take 125 KB of memory. Keep the sieve around after you've generated your prime numbers, because it makes a very efficient $O(1)$ time complexity ...

6

Generation of prime numbers is suboptimal. Use a sieve of Erathosthenes. isPrime is highly suboptimal. You already generated an array of all necessary primes, so just binary search it. Breaking the loop in if(sumFromTo(cumulativeSums,start,end)>limit) break; looks like a bug. The intention is to loop by decreasing end, yet since the ...

6

Thanks for sharing your code. As you already found out, the only difference in the "map generation blocks" is the names of the variables holding the maps. So it is quite obvious, that you could move this 3 lines into the method buildDtos. On the other hand the first line in each "map generation blocks" calls a separate method on the client object. This ...

3

Some tips: Tip 1 @Autowired AccountRepository accountRepository; @Autowired BCryptPasswordEncoder bCryptPasswordEncoder; ... Setter injection is considered as bad practice prefer constructor injection to gain more control and to avoid NullPointerExceptions. Why field injection is evil Tip 2 @Id private String id = UUID.randomUUID().toString(); ...

1

I'm not going to address what else you could test, since it really depends what you're expecting your Polynomial class to do. However, looking at your test, there's several things that could be improved. Test One Thing Having all of your assertions in the same test means that the test can fail for multiple different reasons, some of which are unrelated. ...

0

the only advice, that I can give you, is to separate the logic in multiple methods. This will make your method shorter and allow the code to be reused. Personally, I see two other methods. String readString(Reader r, Charset charset) throws IOException { ByteArrayOutputStream ostream = copyInputToStream(r); final ByteBuffer byteBuffer =...

1

As already said by greybeard you have to try to be more specific about efficiency and what you want to improve. Your code can be simplified , for example your genAttribute() method: public static int genAttribute() { Random dice = new Random(); int[] sumArray = {dice.nextInt(6) + 1, dice.nextInt(6) + 1, dice.nextInt(6) + 1, dice.nextInt(6) + ...

0

Two small, related points public void calculateShortedPath(int source) Seems like it should be calculateShortestPath (not Shorted). At the moment the method returns void and is responsible for both calculating the path and printing it out. Consider changing it to return the shortest path instead, so that the caller can decide what it wants to do with ...

2

Testing strategy seems like it might be a bit out of scope for Code Review, but having historically also found this process a bit confusing, my suggestion would be to not explicitly test your VideoService class. Test its consumers. It's not necessary (or desirable) for there to be a one-to-one mapping of test classes to implementation classes. Your entry ...

4

Some notes: If you're using mocks you are pretty much by definition doing integration rather than unit testing, because mocks enable you to test how a piece of code integrates with something else having a specific behaviour. The primary goal of tests is to gain confidence that the code does what you expect. A sub-goal of this is clarity - the test should be ...

3

Hey, I think you have pretty nice code there already. Anyways, I have some suggestions: Formatting This looks pretty good already so please don't take the points to seriously. Mostly it's my personal preferences. I don't like breaking variable definitions like so: private HashMap<GraphVertex, LinkedList<WeightedEdge>> ...

0

I'm by no means a java programmer, just some things I noticed. GraphVertex.equals This method is messy to say the least. The number of return in this method mixed with if statements with no brackets makes it hard to read. And, it looks like you only want to check one condition, anything else other than that would result in a false return. Now, you only ...

8

The idea of the test class is ok: you want to test the service, mock the repository and you check on the outcome. In the test, it would be nice to use the AAA (arrange/act/assert) pattern. In my tests, i call these setup, execute and verify. There is no logic in the service layer, and your method returns the database objects. Also you only have one test, ...

16

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

2

in my opinion, the code is good, but I have some suggestions. com.github.coderodde.RemoteFile class com.github.coderodde.RemoteFile#download(java.io.File) The URISyntaxException in the method com.github.coderodde.RemoteFile#download(java.io.File) is never thrown and can be removed. In my opinion, the logic of this method should be extracted to an external ...

-1

//Class 1, as separate Node package queue; //Developed by Arun Singh public class QueueNode<T> { private T val; private QueueNode<T> next; public QueueNode(T val) { this.val = val; this.next = null; } ...

2

Document your code. In the code. Be sure you got the specification right. Very helpful here is giving test serious consideration early on. • testing order in which each value was generated must be preserved  looks a nightmare One way to present the approach coded: 1) generate six sums 2) if this sextuple is not admissible, start over Alternatives: 2b) ...

2

It depends on what is exactly possible in your class - can you override class, can some fields be null at any point? For example retrofit creates classes using reflection and some classes can be very unexpectedly null unless set in constructor without parameters. Many IDEs have generators, they do this really well. If you override equals, you really should ...

1

My java is a bit rusty, so my answer is going to be more generic than just java. For 1., as @tieskedh points out, you want to use sets. If you don't have to available, you can emulate one by creating a list containing all the elements, sorting it and checking that none of the values has a neighbour that is identical to itself. This will be effecient and ...

0

My computer theory classes have been a couple of decades ago, so I may be wrong, but you seem to be confusing the terms "node", "vertex", "edge" and some more. Unless I'm mistaken "node" and "vertex" are the same thing, so I'd suggest to use one or the other, but not both. You also seem to sometimes call the value assigned to a vertex "node". Use a ...

2

I'll have a go at your first question, using a Set: Set<Integer> seenNumbers = new HashSet<Integer>(); for (int i = 0; i < length; ++i) { for (int j = 0; j < length; ++j) { if (!seenNumbers.add(square[i][j])) { return false; } } } ... rest of method It keeps adding elements to the Set until it finds a ...

9

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

4

Note, using Sets, is the way to go. My answer is based upon the assumption that you didn't learn about Sets and you can only use arrays. duplicated values A1 A2 A3 A4 A5 A6 A7 A8 A9 your loop: for (int i = 0; i < length; i++){ for (int j = 0; j < length; j++){ int x = square[i][j]; for (int a = 0; a < length; a++){ ...

-1

11,58,351 nanoSeconds is my execution time for this question. I used a little different Logic, First, go through the code and look at the //Marker# 01 02, etc. these are the points I have explained after code.have I have added flags considering cases like num is 1, num is completely divisible by 2, etc. import java.io.*; import java.math.*; public class ...

Top 50 recent answers are included