Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

develop into master: Challenges | Extra Challenge completed #3

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

ferodrigop
Copy link

Completed challenges then executed the commands:

mvn test -Dtests=ChallengesTest.java

image

mvn test -Dtests=ChallengesTest.java

image

return new CardWinner();
var player1Hand = player1.stream()
.sorted(Comparator.comparing((Integer i) -> i).reversed())
.distinct()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried about this distinct, what if our hand was all the same digits? We would end up with a stream that has only one digit, which is incorrect, because all the other cards are still valid, e.x

[1,2,3,4,5] vs [5,5,5,5,5] 

The winning hand for the left player is 54, while the winning hand for the right should be 55, with distinct I'm thinking you would get only 5

Copy link
Author

@ferodrigop ferodrigop Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored on 32577dd , then tested scenario described in description and passed

Comment on lines 89 to 95
double totalCost = switch (callCostObject.getType()) {
case "International" ->
Double.parseDouble(String.format("%.2f", (duration * 7.56) + (extraMinutes * 3.03)));
case "National" ->
Double.parseDouble(String.format("%.2f", (duration * 1.20) + (extraMinutes * 0.48)));
case "Local" -> Double.parseDouble(String.format("%.2f", (duration * 0.2)));
default -> 0D;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Similar comment as here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored on 32577dd including a new method which performs the calculations

Comment on lines 76 to 78
.collect(Collectors.groupingBy(CallCostObject::getType));


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This group by operation seems a bit redundant, you could achieve the same with a map since you're just doing a flatMap later on in L79

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed and simplified on 32577dd

System.arraycopy(COUNTRY_NAMES, index, tempArray, 0, numberOfElementsToCopy);
System.arraycopy(COUNTRY_NAMES, 0, tempArray, numberOfElementsToCopy, COUNTRY_NAMES.length - numberOfElementsToCopy);

COUNTRY_NAMES = tempArray.clone();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: If COUNTRY_NAMES were to be a static variable you would be breaking this function each time you called it, returning tempArray.clone directly would be less error prone

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored to directly return it on e235614

Comment on lines 102 to 104
String result = String.valueOf(powResult);

return result.substring(result.length() - lastDigits);
Copy link
Collaborator

@quebin31 quebin31 Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Think about the solution with modulo described here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored solution and used modulo on e235614

Comment on lines 137 to 139
for (char c : digits) {
sum = sum.add(new BigDecimal(String.valueOf(c)));
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Similar comment as here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored solution and used modulo and division on e235614

Comment on lines +158 to +161
for (Integer num : ascivalues) {
number += num;
result.append((char) number);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanest one yet!

Comment on lines 2 to 11

import mocks.CallCostObject;
import mocks.CallSummary;
import mocks.CardWinner;
import mocks.TotalSummary;

import java.util.Comparator;
import java.util.List;
import java.util.stream.Collectors;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: usually imports are ordered by putting first libraries (either external or from the STL) and then local imports from other directories.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rearranged on 32577dd

Comment on lines 98 to 101
return new CallSummary(
callCostObject,
totalCost
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this return could be a single line of code, not worth splitting it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored on 32577dd

Copy link
Collaborator

@sbobadilla8 sbobadilla8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall good job, besides the comments already given I don't have much to add.

@ferodrigop
Copy link
Author

ferodrigop commented Nov 21, 2024

Files that belong to Extra Challenge Solution:

modified: pom.xml
new file: src/main/java/weather/Main.java
new file: src/main/java/weather/dtos/WeatherDataDTO.java
new file: src/main/java/weather/utils/ClassTypeUtil.java
new file: src/main/java/weather/utils/DayOfWeekDeserializer.java
new file: src/main/java/weather/utils/MonthDeserializer.java
new file: src/main/java/weather/utils/ObjectMapperUtil.java
new file: src/main/java/weather/utils/StatisticsUtil.java
new file: src/main/java/weather/utils/StringUtil.java

@ferodrigop ferodrigop changed the title develop into master: Challenges completed develop into master: Challenges | Extra Challenge completed Nov 21, 2024
Comment on lines +55 to +59
// statistics of all records
statisticsOfRecords(listWeatherDataDTO);

// statistics of records per day
statisticsOfRecordsPerDay(listWeatherDataDTO);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you think of a way of gathering in results in a single call? So that the JSON is processed only once.

Copy link
Collaborator

@sbobadilla8 sbobadilla8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very clean overall, just left one extra bit to further increase the challenge.

Comment on lines +8 to +9
public class StatisticsUtil {
public static Map<String, DoubleSummaryStatistics> calculateDoubleSummaryStatisticsOfObjectAttributes(List<?> records) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you using reflection for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants