3

Recently I received this assignment in my Beginning Computer Science Class:

Write a program that asks the user to enter a month (1 = January, 2 = February, and so on) and then prints the number of days of the month. For February, print “28 days”. Enter a month (1-12): 5 31 days Implement a class Month with a method int getDays(). Do not use a separate if or else statement for each month. Use Boolean operators.

I am not asking for someone to complete the mentioned assignment, but rather critique my own solution. I did some basic searching before asking this, but most if not all solution use the Calendar class and calculate the number of days that way. The object of this assignment is to convey the idea of conditional statements and boolean operators.

My Solution: Season.java

/**
 * @author Jared Holley
 * Date: 4/01/14
 * Period: 3rd
 * 
 * Write a program that asks the user to enter a month (1 = January, 2 = February, and
 * so on) and then prints the number of days of the month. For February, print
 * “28 days”.
 * 
 * With a method int getDays(). Do not use a separate if or else
 * statement for each month. Use Boolean operators.
 */

 //31: 1,3,5,7,8,10,12
 //30: 2,4,6,9,11
 public class Month {
  private int month;

  /**
   * The constructor for the Month class.
   * Simply takes in a monthNumber and sets it to the 
   * class variable.
   * @param monthNumber
   */
  public Month(int monthNumber){
    month = monthNumber;
  }

  /**
   * The method that converts a month number into the number of days within that month.
   * It first checks if the month is February so that it eliminates that from the        following conditions that would produce a false positive.
   * It then goes through and uses a rather odd conditional statement.
   * The first half of the if statements increments all the months by one and checks if they are even.
   * This would turn January into 2 which does have 31 days and trun April into 5 which has 30 days.
   * The second half just checks the even months beyond 7.
   * Lastly it will just return 30 otherwise.
   * @return The numbers of days within the month.
   */
  public int getDays(){
    if(month == 2)return 28;
    if( ((month + 1) % 2 == 0 && month < 9) || ((month % 2 == 0) && month >= 8))   return 31;
    return 30;

  }
}

The solution works just fine. I would just like to know if there is a more elegant way of going about things. My conditional statement just looks very ugly. Thanks for the help!

JasonMArcher
  • 14,195
  • 22
  • 56
  • 52
Holleyj66
  • 33
  • 1
  • 4
  • 1
    It looks bad indeed but considering the assignment I'm not sure you can improve much. Note: you could make `month` final. Also, `month % 2 != 0` may be clearer than `(month + 1) % 2 == 0`... – assylias Apr 03 '14 at 00:12
  • Thanks! I was pretty confident that it could not be shortened. However, I will be making the change you suggested as it seems easier to understand my reasoning behind it. – Holleyj66 Apr 03 '14 at 00:20
  • Don't make `month` final unless you want this class to be immutable. And if you _do_ make `month` final, it should be `public final` since it's a primitive. – Jared Apr 03 '14 at 00:22
  • @jared IMO immutability should be the default for such a "value" class. I'm not sure why you think it should be public though. – assylias Apr 03 '14 at 00:26
  • @assylias Why shouldn't it be public (if it's final)? That's my reasoning. – Jared Apr 03 '14 at 00:27
  • @Jared Personally, and remember I am a beginner, I believe deciding on whether or not to set it public or private is very much based on the situation. In this particular case I will keep it as private because I do not plan to access it from anywhere else. Any flaws with my theory? – Holleyj66 Apr 03 '14 at 00:32
  • @Jared let's say (contrived example) that you realise that a much better `getDays` method would be `return daysArray[month];` but you really don't want to waste space and decide that `month` should start at index 0. You've just broken all the code that depended on `month` being a human representation of the month number. Using a private variable and a `getMonthNumber` method gives you the flexibility to change your implementation if you like without anybody noticing. – assylias Apr 03 '14 at 00:35
  • @Jared, because sub classes can override it in constructors when public or protected – thobens Apr 03 '14 at 00:37
  • @thobens No you can't, try it. The subclass must call a constructor of the super class which, if it's `final` must set the value. – Jared Apr 03 '14 at 00:43
  • Using the variable `month` is completely the calling class's responsibility for using it correctly. The purpose of declaring things private, protected, or package level is about trust. When you give access to an internal variable you trust that a calling class cannot make your object inconsistent. But if it's `final` (and a primitive or immutable object) then any other class cannot disrupt _this_ object. But directly reading variables is always preferable to calling a method. – Jared Apr 03 '14 at 00:47
  • This seems better suited to Code Review. – Azar Apr 03 '14 at 00:55
  • @Jared: you're right, it's a primitive. But then again, and call me old fashined, why the heck would you define it public then anyway? That's a bit of a "because I can" attitude, isn't it?. Assuming you refactor the code and change the type of the variable `month` and *BAM* you have a public variable where you don't want it. – thobens Apr 03 '14 at 00:56
  • @Jared except that all public members are part of your class' api: as soon as other classes start using those members you are stuck. Keeping them private gives you more flexibility. Following your advice on any non-toy project would be painful... – assylias Apr 03 '14 at 00:56
  • @assylias I totally disagree. I don't know what you mean "you are stuck". Most people write getter methods incorrectly and give direct access to private fields without even realizing it. Generally, it's easier to make fields public in a large project--that is a project that is not intended as an external library to be used by others. – Jared Apr 03 '14 at 01:07
  • @jared let's disagree then. – assylias Apr 03 '14 at 01:10
  • @assylias Fair enough. I will say though, that a programmer should be very careful about declaring fields `final`. If you have a program where you want to recycle objects, you must be very careful to make sure you can actually reset the object to something new if you want to and `final` fields restrict your ability to do that. And this is a very real issue in Java due to garbage collection (if you are creating a huge amount of temporary objects it's going to cause performance issues)...which is why I wish there was a construct in Java to place objects on the stack. – Jared Apr 03 '14 at 01:14
  • @Jared: Yes, you are right regarding the `final` modifier, but just making them public because you defined them final is just half-baked, considering you are not the only one modifying or refactoring the code. – thobens Apr 03 '14 at 01:20
  • @Jared you should be very careful about non final fields - makes multithreading a lot more complicated... – assylias Apr 03 '14 at 01:39

2 Answers2

1

All roads tend south...

But I think you reached the goal of your assignment. In my point of view, there are more elegant ways (and maybe faster, I think I need to do some measurements), like using Maps or enums, so you avoid ugly if - else statements like that.

And yes, month should be final because you don't plan on modifying the variable in this class. And yes, it is okay to set it to private (when in doubt, it's allways private)

thobens
  • 1,729
  • 1
  • 15
  • 34
  • 1
    Interesting! I was wondering if you could a little more in depth into the idea of using 'Map's rather than conditionals. I'm rather new to the field and this interests me very much. – Holleyj66 Apr 03 '14 at 00:42
  • 1
    Well, I could post something, but I have to admit that it does not really make this particular problem simpler but more complex (measured in lines of code), while it would be more extensible. (which is not needed since there always will be 12 months and not one more :)) – thobens Apr 03 '14 at 01:13
  • 1
    see http://stackoverflow.com/questions/1298672/avoiding-multiple-if-statements-in-java to get an idea of the concept. It's extremely useful when dealing with dynamic input, while there is not much benefit (or even more effort) with static input (like months) – thobens Apr 03 '14 at 01:27
1

Although this may not be an ideal answer, below one-liner uses only one Boolean operator (==) and it is concise.

int daysInMonth = 31 - ((month == 2) ? 3 : ((month - 1) % 7 % 2));

An explanation of this algorithm can be found here (Solution #3):

http://www.dispersiondesign.com/articles/time/number_of_days_in_a_month

a.z.
  • 66
  • 2
  • Wow, that is really interesting. I have never learned about the ? conditional operator. Now that I know about it I plan to use it a lot more. I too noticed the pattern that the author mentions within the article, but never thought of using month % 7 in order to restart the pattern. Thanks for the contribution! – Holleyj66 Apr 03 '14 at 01:10
  • 1
    this is a short cut for an if else: the condition comes first, then the ? (=if), then one Java statement followed by the : (=else) followed by one Java statement (see http://www.cafeaulait.org/course/week2/43.html) – thobens Apr 03 '14 at 01:26
  • FYI, the `?:` syntax is called a [ternary operator](https://en.wikipedia.org/wiki/%3F:). (Note that link to Wikipedia page) While useful in some scenarios, beware of writing overly "clever" code that becomes a nightmare to read and maintain. – Basil Bourque Apr 03 '14 at 05:08