code improvements #230
code improvements #230
Conversation
| clone.setGeoLocation((GeoLocation) getGeoLocation().clone()); | ||
| clone.setCalendar((Calendar) getCalendar().clone()); | ||
| clone.setAstronomicalCalculator((AstronomicalCalculator) getAstronomicalCalculator().clone()); | ||
| } | ||
| return clone; | ||
| } |
There was a problem hiding this comment.
This was just to get rid of a possible NullPointerException warning. I know it is not necessary, but it is still beneficial to get rid of a few warnings.
| @@ -116,13 +116,13 @@ public class HebrewDateFormatter { | |||
| * שובה,שירה,הגדול, | |||
| * חזון,נחמו"</code> | |||
| */ | |||
| private EnumMap<JewishCalendar.Parsha, String> hebrewParshaMap; | |||
| private final EnumMap<JewishCalendar.Parsha, String> hebrewParshaMap; | |||
There was a problem hiding this comment.
this variable can be final as it is setup in the constructor
|
|
||
| /** | ||
| * Default constructor sets the {@link EnumMap}s of Hebrew and default transliterated parshiyos. | ||
| */ | ||
| public HebrewDateFormatter() { | ||
| transliteratedParshaMap = new EnumMap<JewishCalendar.Parsha, String>(JewishCalendar.Parsha.class); | ||
| transliteratedParshaMap = new EnumMap<>(JewishCalendar.Parsha.class); |
There was a problem hiding this comment.
As of Java 7, the diamond operator <> does not need explicit types. See here: https://stackoverflow.com/a/5999744
| @@ -177,7 +177,7 @@ public class JewishCalendar extends JewishDate { | |||
| * @see #getSpecialShabbos() | |||
| * @see #getParshah() | |||
| */ | |||
| public static enum Parsha { | |||
| public enum Parsha { | |||
There was a problem hiding this comment.
IntelliJ said that there was no need for the enum to be static. Not sure if this is because of newer java versions though. Please double check before merging this change.
| @@ -607,6 +605,9 @@ public int getYomTovIndex() { | |||
| final int day = getJewishDayOfMonth(); | |||
| final int dayOfWeek = getDayOfWeek(); | |||
|
|
|||
| boolean fastOfEsther = ((day == 11 || day == 12) && dayOfWeek == Calendar.THURSDAY) | |||
| || (day == 13 && !(dayOfWeek == Calendar.FRIDAY || dayOfWeek == Calendar.SATURDAY)); | |||
|
|
|||
There was a problem hiding this comment.
This may not be what you want, but it removes the need to write the same code twice in both ADARs
| return CHOL_HAMOED_PESACH; | ||
| } | ||
| if ((day == 22 && inIsrael) || (day == 23 && !inIsrael)) { | ||
| if (day == 22 || day == 23 && !inIsrael) { |
There was a problem hiding this comment.
We can remove the inIsrael check here, because it will never occur since PESACH will be returned above.
| @@ -661,7 +661,7 @@ public int getYomTovIndex() { | |||
| if (day == 6 || (day == 7 && !inIsrael)) { | |||
| return SHAVUOS; | |||
| } | |||
| if ((day == 7 && inIsrael) || (day == 8 && !inIsrael)) { | |||
| if (day == 7 || day == 8 && !inIsrael) { | |||
There was a problem hiding this comment.
same here, we don't need to worry about checking inIsrael because we checked it above already
| } | ||
| return false; // keep the compiler happy | ||
| } | ||
| }// keep the compiler happy |
There was a problem hiding this comment.
This comment can be removed. I did not notice it
| @@ -1306,7 +1306,7 @@ public void forward(int field, int amount) { | |||
| } | |||
| } else if (field == Calendar.MONTH) { | |||
| forwardJewishMonth(amount); | |||
| } else if (field == Calendar.YEAR) { | |||
| } else { | |||
| setJewishYear(getJewishYear() + amount); | |||
| } | |||
There was a problem hiding this comment.
Since we throw an exception above if the field is not Calendar.YEAR, Calendar.MONTH, or Calendar.DATE. There is no need to check for the field after checking for two other cases
| if (clone != null) { | ||
| clone.setInternalGregorianDate(gregorianYear, gregorianMonth, gregorianDayOfMonth); | ||
| } | ||
| return clone; | ||
| } |
There was a problem hiding this comment.
Same as before, get rid of possible NullPointerException warning
| && (!jewishCalendar.isUseModernHolidays() | ||
| || (holidayIndex != JewishCalendar.YOM_HAATZMAUT && holidayIndex != JewishCalendar.YOM_YERUSHALAYIM)) | ||
| && (tachanunRecitedWeekOfHod || month != JewishDate.IYAR || day <= 13 || day >= 21); | ||
| } |
There was a problem hiding this comment.
This just removes the need for and if-else
There was a problem hiding this comment.
Although, now that I look at it, the code looks different, IntelliJ might have taken liberties and done some unwanted stuff here. I doubt the logic has changes, but let me know if you want to revert this.
| tomorrow.getYomTovIndex() == JewishCalendar.EREV_YOM_KIPPUR || | ||
| tomorrow.getYomTovIndex() == JewishCalendar.PESACH_SHENI) | ||
| && (tachanunRecitedMinchaErevLagBaomer || tomorrow.getYomTovIndex() != JewishCalendar.LAG_BAOMER); | ||
| } |
There was a problem hiding this comment.
The logic shouldn't have changed but it seems like intellij also took liberties here
| } | ||
| total -= i; | ||
| masechta++; | ||
| } |
There was a problem hiding this comment.
Use an enhanced for loop instead. Not critical, we can revert if unwanted
| && (this.locationName == null ? geo.locationName == null : this.locationName.equals(geo.locationName)) | ||
| && (this.timeZone == null ? geo.timeZone == null : this.timeZone.equals(geo.timeZone)); | ||
| && (Objects.equals(this.locationName, geo.locationName)) | ||
| && (Objects.equals(this.timeZone, geo.timeZone)); |
There was a problem hiding this comment.
Objects.equals will handle nulls
| @@ -418,7 +416,7 @@ public static double getSolarAzimuth(Calendar cal, double lat, double lon) { | |||
| double julianDay = getJulianDay(cal); | |||
| double julianCenturies = getJulianCenturiesFromJulianDay(julianDay); | |||
|
|
|||
| Double eot = getEquationOfTime(julianCenturies); | |||
| double eot = getEquationOfTime(julianCenturies); | |||
There was a problem hiding this comment.
Not sure why the Double wrapper class was used instead of a regular double. Let me know if this was intentional.
| return Long.valueOf(firstTime).compareTo(Long.valueOf(secondTime)); | ||
| } | ||
| }; | ||
| public static final Comparator<Zman> DATE_ORDER = (zman1, zman2) -> { |
There was a problem hiding this comment.
Use lambda instead
| public static final Comparator<Zman> DATE_ORDER = (zman1, zman2) -> { | ||
| long firstTime = (zman1 == null || zman1.getZman() == null) ? Long.MAX_VALUE : zman1.getZman().getTime(); | ||
| long secondTime = (zman2 == null || zman2.getZman() == null) ? Long.MAX_VALUE : zman2.getZman().getTime(); | ||
| return Long.compare(firstTime, secondTime); |
There was a problem hiding this comment.
Use Long.compare instead
|
|
||
| // Fix Shekalim for old cycles. | ||
| if (cycleNo <= 7) { | ||
| blattPerMasechta[4] = 13; | ||
| } else { | ||
| blattPerMasechta[4] = 22; // correct any change that may have been changed from a prior calculation | ||
| } |
There was a problem hiding this comment.
The comment says that we need to reassign the value because it might have been changed before. However, the blattPerMasechta variable is recreated every time the method is called. There should be no need for this.
|
closing this to make two separate PRs |
I will try my best to explain this PR, but it basically contains all the recommended actions to do in order to make the code less redundant and easier to read. I will add additional comments where needed in the commits themselves.