develop into master: Challenges methods done #2
Conversation
cmonterrosad
requested review from
quebin31,
sergiolg98,
cmonterrosad and
sbobadilla8
katopy
changed the title
|
nit: At first glance, one small peace of feedback is that should avoid pushing commented code to your pull requests (Definition of nit: https://onetwobytes.com/2021/11/09/nit-code-review/) |
|
|
||
| List<Integer> sortedNum1 = player1.stream().sorted(Comparator.reverseOrder()).limit(2).toList(); | ||
| List<Integer> sortedNum2 = player2.stream().sorted(Comparator.reverseOrder()).limit(2).toList(); | ||
|
|
There was a problem hiding this comment.
nit: You could also collect this stream of integers as a String by using map and collect with Collectors.joining
There was a problem hiding this comment.
Yes, you're right. The map() method could help me to return a stream of the results when I use sorted(). So the follow 2 lines updated are:
String sortedNum1 = player1.stream().sorted(Comparator.reverseOrder()).map(Object::toString).limit(2).collect(Collectors.joining()); String sortedNum2 = player2.stream().sorted(Comparator.reverseOrder()).map(Object::toString).limit(2).collect(Collectors.joining());
So I could delete the extra lines:
StringBuilder stringB1 = new StringBuilder(); StringBuilder stringB2 = new StringBuilder(); sortedNum1.forEach(stringB1::append); sortedNum2.forEach(stringB2::append);
| int n1 = Integer.parseInt(stringB1.toString()); | ||
| int n2 = Integer.parseInt(stringB2.toString()); |
There was a problem hiding this comment.
nit: This solution is okay because we're working with Strings, but just as food for thought, can you think of any other way we could transform a Stream of Integers to a single Integer.
Hint: reduce and base-10 basic arithmetics
Feel free to answer to this comment if you have an idea, no need to implement it, pseudocode and/or an explanation would be enough :)
| case "International" -> { | ||
| double baseTypeCost = 7.56; | ||
| double additionalCost = 3.03; | ||
| totalCost = call.getDuration() <= 3 | ||
| ? baseTypeCost * (call.getDuration()) | ||
| : (baseTypeCost * 3) + (call.getDuration() - 3) * additionalCost; | ||
| } | ||
| case "National" -> { | ||
| double baseTypeCost = 1.20; | ||
| double additionalCost = 0.48; | ||
| totalCost = call.getDuration() <= 3 | ||
| ? baseTypeCost * (call.getDuration()) | ||
| : (baseTypeCost * 3) + (call.getDuration() - 3) * additionalCost; | ||
| } | ||
| case "Local" -> { | ||
| double baseTypeCost = 0.2; | ||
| totalCost = (call.getDuration() * baseTypeCost); | ||
| } |
There was a problem hiding this comment.
nit: All of these lines of code share the same logic more or less with different parameters (you can think of "Local" as having an additionalCost of zero and zero overcharge, you could extract this logic to a separate method
| newArray.addAll(Arrays.asList(COUNTRY_NAMES).subList(greaterIndex, arrayLength)); | ||
| newArray.addAll(Arrays.asList(COUNTRY_NAMES).subList(0, greaterIndex)); | ||
|
|
||
| return newArray.toArray(new String[0]); |
There was a problem hiding this comment.
What's the new String[0] for?
| sum = sum.add(power); | ||
| } | ||
|
|
||
| // I tried to pass use mod, but mathematically the last test was deleting the 0 at the left, so I chose to cast to string |
There was a problem hiding this comment.
Good thinking here, a more efficient solution could also use the addition properties of the modulo operation to avoid overflowing integers (even though we're using BigIntegers this greatly reduces the risk for even bigger numbers).
There's still one way to follow this approach, since we know that we want lastDigits we could pan the string to add zeros in front if there's not enough digits in the resulting modulo until we have the number of digits requested as a String
| String myNumber = resultFact.toString(); | ||
| List<Integer> listToSum = myNumber.chars() | ||
| .map(Character::getNumericValue) | ||
| .boxed().toList(); | ||
|
|
||
| for(Integer l : listToSum){ | ||
| sum += l; | ||
| } | ||
| return sum; |
There was a problem hiding this comment.
nit: You could get the sum with arithmetic operations, namely division and modulo, this is solution isn't wrong, but may be more costly in terms of memory allocation, though we would need to measure performance to be sure what of the two solutions is more efficient
|
|
||
| for(int c : thisIndex){ | ||
| char myChar = (char) c; | ||
| finalString.append(myChar); | ||
| } |
There was a problem hiding this comment.
nit: Could be done in the same for where we calculate the ASCII value, not a huge issue but we're iterating over the same n (roughly O(n)) elements again
|
|
||
| */ | ||
|
|
||
| int totalCalls = 0; |
There was a problem hiding this comment.
nit: I found this variable only being used in L144 when it is assigned) and in L148 (when it is sent into the function. Since it is only being read you could skip this declaration and the assignment in L144 by directly sending callSummaries.size() into the function.
There was a problem hiding this comment.
Another comment related to code cleanliness is to avoid adding extra lines in between stuff when it is not necessary. Leaving empty lines usually represents different segments of the code, i.e when you want to separate a logic block from another one that still is part of the same function.
|
The JSON challenge could've used Jackson for less boilerplate, or even Gson too |
PR to merge my method solutions from Module 1. I added some comments.