-
Notifications
You must be signed in to change notification settings - Fork 5
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
Challenges completed #5
base: master
Are you sure you want to change the base?
Conversation
src/main/java/ChallengeStream.java
Outdated
private double calculateInternationalCost(int duration) { | ||
// If the duration is more than the base minutes, calculate the cost for the additional minutes | ||
|
||
double total = 0.00; | ||
if (duration <= BASE_MINUTES) { | ||
total += INTERNATIONAL_FIRST_3_MIN * duration; | ||
} else { | ||
total += (INTERNATIONAL_ADDITIONAL_MIN * (duration - BASE_MINUTES)) + (BASE_MINUTES * INTERNATIONAL_FIRST_3_MIN); | ||
} | ||
return total; | ||
} | ||
|
||
private double calculateNationalCost(int duration) { | ||
// If the duration is more than the base minutes, calculate the cost for the additional minutes | ||
double total = 0.00; | ||
if (duration <= BASE_MINUTES) { | ||
total += NATIONAL_FIRST_3_MIN * duration; | ||
} else { | ||
total += (NATIONAL_ADDITIONAL_MIN * (duration - BASE_MINUTES)) + (BASE_MINUTES * NATIONAL_FIRST_3_MIN); | ||
} | ||
return total; | ||
} | ||
|
||
private double calculateLocalCost(int duration) { | ||
// In local it doesn't charge for the first minutes | ||
return duration * LOCAL_PER_MIN; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This functions have very similar logic in them, perhaps you could simplify this?
int player1Max = player1.stream() | ||
// create a stream of all possible two-digit numbers | ||
.flatMap(d1 -> player1.stream() | ||
.filter(d2 -> !d2.equals(d1)) // ensure that the two digits are different |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Digits don't need to be different, a player could win with 11,22,...,99
Same for L122
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What Sebastian said, please send a fix
src/main/java/Challenges.java
Outdated
sum = sum.add(power); | ||
} | ||
|
||
return sum.toString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: if you're going to separate chained calls in multiple lines do so from the beginning so it is easy to follow.
class LocationJson { | ||
private double lon; | ||
private double lat; | ||
|
||
public LocationJson() { | ||
} | ||
|
||
public LocationJson(double lon, double lat) { | ||
this.lon = lon; | ||
this.lat = lat; | ||
} | ||
|
||
public double getLon() { | ||
return lon; | ||
} | ||
|
||
public void setLon(double lon) { | ||
this.lon = lon; | ||
} | ||
|
||
public double getLat() { | ||
return lat; | ||
} | ||
|
||
public void setLat(double lat) { | ||
this.lat = lat; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be on its own separate file
private static boolean isValidRecord(Weather record) { | ||
return record != null | ||
&& record.getAirtemp() >= 0 | ||
&& record.getAtmosphericpressure() >= 0 | ||
&& record.getGustspeed() >= 0 | ||
&& record.getPrecipitation() >= 0 | ||
&& record.getRelativehumidity() >= 0 | ||
&& record.getSolar() >= 0 | ||
&& record.getStrikedistance() >= 0 | ||
&& record.getStrikes() >= 0 | ||
&& record.getVapourpressure() >= 0 | ||
&& record.getWindspeed() >= 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be careful with this because there might be cases where the temperature is less than 0.
long startTime = System.currentTimeMillis(); | ||
analyzeWeatherData(); | ||
long endTime = System.currentTimeMillis(); | ||
System.out.println("\nExecution time: " + (endTime - startTime) + "ms"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty cool you thought of adding this. Could you post the resulting processing time? I'm curious
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I had thought of adding this I could've made a contest out of it. Too late now, but pretty cool that it is quick. Well done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job overall. Just something you should take note of, whenever we are making a PR we only put the code we actually want in a production environment. So you shouldn't have a V1 or V2 (git takes care of that!). You mentioned option2 is the final version you would like to submit, so just submit that one.
src/main/java/ChallengeStream.java
Outdated
private Double calculateCallCost(CallCostObject call) { | ||
return switch (call.getType()) { | ||
case INTERNATIONAL -> calculateCostByType(call.getDuration(), INTERNATIONAL); | ||
case NATIONAL -> calculateCostByType(call.getDuration(), NATIONAL); | ||
case LOCAL -> calculateCostByType(call.getDuration(), LOCAL); | ||
default -> 0.0; | ||
}; | ||
} | ||
|
||
private double calculateCostByType(int duration, String type) { | ||
double total = 0.00; | ||
double first3MinRate; | ||
double additionalMinRate; | ||
|
||
switch (type) { | ||
case INTERNATIONAL -> { | ||
first3MinRate = INTERNATIONAL_FIRST_3_MIN; | ||
additionalMinRate = INTERNATIONAL_ADDITIONAL_MIN; | ||
} | ||
case NATIONAL -> { | ||
first3MinRate = NATIONAL_FIRST_3_MIN; | ||
additionalMinRate = NATIONAL_ADDITIONAL_MIN; | ||
} | ||
case LOCAL -> { | ||
return duration * LOCAL_PER_MIN; | ||
} | ||
default -> throw new IllegalArgumentException("Invalid call type"); | ||
} | ||
|
||
if (duration <= BASE_MINUTES) { | ||
total = first3MinRate * duration; | ||
} else { | ||
total = (additionalMinRate * (duration - BASE_MINUTES)) + (BASE_MINUTES * first3MinRate); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it could be merged into a single function... since you're doing the same type of switch in both functions. Do you think you could simplify this?
public Location() { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: for empty constructors you can put the curly braces in the same line to make it clearer it's empty on purpose.
public Weather() { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: same comment about {}
&& record.getAtmosphericpressure() >= 0 | ||
&& record.getGustspeed() >= 0 | ||
&& record.getPrecipitation() >= 0 | ||
&& record.getRelativehumidity() >= 0 | ||
&& record.getSolar() >= 0 | ||
&& record.getStrikedistance() >= 0 | ||
&& record.getStrikes() >= 0 | ||
&& record.getVapourpressure() >= 0 | ||
&& record.getWindspeed() >= 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still wondering if this would be the correct way of checking if you have a correct record. You could so some sort of pre-processing or do a bit of research about the possible values you could get, but perhaps you could try just checking for null values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good improvement over previous version, left a couple more comments to keep you busy.
double sum = 0; | ||
double min = Double.MAX_VALUE; | ||
double max = Double.MIN_VALUE; | ||
long count = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nit: I think that an int is enough here, I doubt the file has more than 2,147,483,647 records
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
case LOCAL -> { | ||
return duration * LOCAL_PER_MIN; | ||
} | ||
default -> { | ||
return 0.0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Code smell here, part of the switch doesn't return and part of it returns, this divergent behavior and could be confusing when reading the code
I only have the comment that to read the json file you must put it in the src/main/resources/weather.json folder and also that the final solution for the additional challenge is the one that says WeatherAnalyzerOption2.java.