Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link
  • private final Your fields dateStart, dateStop and the others should be marked as private final. Other classes don't need direct access to them (and normally shouldn't have direct access even if they needed it), and as they never change once the class has been initialized it's good practice to mark them as final.

  • I'm not a big fan of the lack of line breaks in your switch statements. Also, by returning boolean as has been suggested in other answers, you can simplify it to: (Note the improved formatting)

     switch (repetitionType) {
     case WEEKLY: 
     return checkWeekRepetition(dateToCheck);
     case MONTHLY: 
     return checkMonthRepetition(dateToCheck);
     }
    

    I also removed the if (repetitionType != null) check as that's not needed, if it would be null the default case would apply, and as your default case was just empty there's no need in having it at all.

  • catch (IllegalArgumentException ex) {

    You catch the IllegalArgumentException to use it as flow-control by returning false if it occurs. Don't do that. Don't use Exceptions for flow-control in the first place, and I also have to point out that some exceptions are not meant to be caught. That includes IllegalArgumentException See another answer of mine here here regarding such exceptions.

  • private final Your fields dateStart, dateStop and the others should be marked as private final. Other classes don't need direct access to them (and normally shouldn't have direct access even if they needed it), and as they never change once the class has been initialized it's good practice to mark them as final.

  • I'm not a big fan of the lack of line breaks in your switch statements. Also, by returning boolean as has been suggested in other answers, you can simplify it to: (Note the improved formatting)

     switch (repetitionType) {
     case WEEKLY: 
     return checkWeekRepetition(dateToCheck);
     case MONTHLY: 
     return checkMonthRepetition(dateToCheck);
     }
    

    I also removed the if (repetitionType != null) check as that's not needed, if it would be null the default case would apply, and as your default case was just empty there's no need in having it at all.

  • catch (IllegalArgumentException ex) {

    You catch the IllegalArgumentException to use it as flow-control by returning false if it occurs. Don't do that. Don't use Exceptions for flow-control in the first place, and I also have to point out that some exceptions are not meant to be caught. That includes IllegalArgumentException See another answer of mine here regarding such exceptions.

  • private final Your fields dateStart, dateStop and the others should be marked as private final. Other classes don't need direct access to them (and normally shouldn't have direct access even if they needed it), and as they never change once the class has been initialized it's good practice to mark them as final.

  • I'm not a big fan of the lack of line breaks in your switch statements. Also, by returning boolean as has been suggested in other answers, you can simplify it to: (Note the improved formatting)

     switch (repetitionType) {
     case WEEKLY: 
     return checkWeekRepetition(dateToCheck);
     case MONTHLY: 
     return checkMonthRepetition(dateToCheck);
     }
    

    I also removed the if (repetitionType != null) check as that's not needed, if it would be null the default case would apply, and as your default case was just empty there's no need in having it at all.

  • catch (IllegalArgumentException ex) {

    You catch the IllegalArgumentException to use it as flow-control by returning false if it occurs. Don't do that. Don't use Exceptions for flow-control in the first place, and I also have to point out that some exceptions are not meant to be caught. That includes IllegalArgumentException See another answer of mine here regarding such exceptions.

Source Link
Simon Forsberg
  • 59.7k
  • 9
  • 157
  • 311
  • private final Your fields dateStart, dateStop and the others should be marked as private final. Other classes don't need direct access to them (and normally shouldn't have direct access even if they needed it), and as they never change once the class has been initialized it's good practice to mark them as final.

  • I'm not a big fan of the lack of line breaks in your switch statements. Also, by returning boolean as has been suggested in other answers, you can simplify it to: (Note the improved formatting)

     switch (repetitionType) {
     case WEEKLY: 
     return checkWeekRepetition(dateToCheck);
     case MONTHLY: 
     return checkMonthRepetition(dateToCheck);
     }
    

    I also removed the if (repetitionType != null) check as that's not needed, if it would be null the default case would apply, and as your default case was just empty there's no need in having it at all.

  • catch (IllegalArgumentException ex) {

    You catch the IllegalArgumentException to use it as flow-control by returning false if it occurs. Don't do that. Don't use Exceptions for flow-control in the first place, and I also have to point out that some exceptions are not meant to be caught. That includes IllegalArgumentException See another answer of mine here regarding such exceptions.

lang-java

AltStyle によって変換されたページ (->オリジナル) /