java Calendarクラス

質問

javaで月、日を入力してカレンダーを作成したのですが
年と月のsetでmonth-1はマジックナンバーなので直したいのですが
どなたかわかる方教えてください。

package sample;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.*;

public class Test {

private final static int firstday = 1;

public static void main(String[] args) {
//カレンダーのインスタンスを取得します
Calendar cal = Calendar.getInstance();
//文字入力
BufferedReader input = new BufferedReader(new InputStreamReader(System.in));
//年を取得
int year =0;
//月を取得
int month =0;
//最後の日付
int lastDay =0;

//月初めの曜日を取得
int week =0;
//年妥当性チェック
boolean CheckYear = true;
//月妥当性チェック
boolean CheckMonth = true;

try {
//年妥当性チェック
while(CheckYear){
System.out.println("年を入力してください");
//年を入力します
year = Integer.parseInt(input.readLine());
//年が４桁の場合
if(String.valueOf(year).length()==4){
CheckYear = false;
}else{
System.out.println("年は4桁で入力してください");
}
}
//月妥当性チェック
while(CheckMonth){
System.out.println("月を入力してください");
//月を入力します
month = Integer.parseInt(input.readLine());
//月が１～１２の場合
if(month>=1&&month<=12){
CheckMonth = false;
}else{
System.out.println("月１～１２を入力してください");
}
}
}catch(IOException e){
System.out.println("数字以外は入力しないでください");
System.out.println("処理を中断します");
return;

}catch (Exception a) {
System.out.println("数字以外は入力しないでください");
System.out.println("処理を中断します");
return;
}
//年、月をセットします
cal.set(year,month-1,cal.getActualMinimum(Calendar.DATE));
//月初めの曜日を取得
week = cal.get(Calendar.DAY_OF_WEEK);
//年月を出力する
System.out.println(String.valueOf(year)+"年"+String.valueOf(month)+"月");
//曜日を出力する
System.out.println("日　月　 火　 水　 木　 金　 土");
//最後の日付を取得する
lastDay = cal.getActualMaximum(Calendar.DAY_OF_MONTH);
/*
* 最後の日付を取得する
*/
if(month==1||month==3||month==7||month==8||month==10||month==12) {
lastDay = cal.getActualMaximum(Calendar.DAY_OF_MONTH);
}else if(month==4||month==6||month==9||month==11){
lastDay = cal.getActualMaximum(Calendar.DAY_OF_MONTH);
}else if(year%4==0&&month==2){
lastDay = cal.getActualMaximum(Calendar.DAY_OF_MONTH);
}else if(year%4!=0&&month==2){
lastDay = cal.getActualMaximum(Calendar.DAY_OF_MONTH);
}
/*
* カレンダーを出力する
*/
//最後の日付まで繰り返す
// 最後の日付まで繰り返す
for (int i = 1; i <= lastDay; i++) {
// 1日とそれ以外で分岐する
if (i == 1) {
// 1日の曜日位置まで移動する
for (int j = 1; j < cal.get(Calendar.DAY_OF_WEEK); j++) {
System.out.print(" ");
}
} else {
// 日付を増やす
cal.add(Calendar.DAY_OF_MONTH, firstday);
}
// 1～9と10～で表示を変える
if (i < 10) {
System.out.print(" " + i);
} else {
System.out.print(" " + i);
}
// 土曜日になったら改行する
if (cal.get(Calendar.DAY_OF_WEEK) == Calendar.SATURDAY) {
System.out.println("");
}
}
}
}

回答No.5

ssr-y6

Calendarクラスの定数フィールドに、JANUARYやSUNDAYなどの定数が定義されているので、
直接数値を使わずにそれらを使ってCalendarクラスへはアクセスしたほうがよいと思います。
　以下のように対応付けをすれば見栄えもよくなると思います。

import java.util.*;

public class calen {

private static final int WeekValue[] = { Calendar.SUNDAY, Calendar.MONDAY, Calendar.TUESDAY,
Calendar.WEDNESDAY, Calendar.THURSDAY, Calendar.FRIDAY, Calendar.SATURDAY};
private static final String WeekName[] = {" 日", " 月", " 火", " 水", " 木", " 金", " 土"};

public static void main(String args[]) {

int year, month, week, lastDay, Int1, Int2;
Calendar cal = Calendar.getInstance();

int MonthValue[] = new int[13]; String MonthName[] = new String[13];
MonthValue[1] = Calendar.JANUARY; MonthName[1] = new String("一月");//入力された1をJUNUARYと識別して一月と表示する
MonthValue[2] = Calendar.FEBRUARY; MonthName[2] = new String("二月");//以下同じ
MonthValue[3] = Calendar.MARCH; MonthName[3] = new String("三月");
MonthValue[4] = Calendar.APRIL; MonthName[4] = new String("四月");
MonthValue[5] = Calendar.MAY; MonthName[5] = new String("五月");
MonthValue[6] = Calendar.JUNE; MonthName[6] = new String("六月");
MonthValue[7] = Calendar.JULY; MonthName[7] = new String("七月");
MonthValue[8] = Calendar.AUGUST; MonthName[8] = new String("八月");
MonthValue[9] = Calendar.SEPTEMBER; MonthName[9] = new String("九月");
MonthValue[10] = Calendar.OCTOBER; MonthName[10] = new String("十月");
MonthValue[11] = Calendar.NOVEMBER; MonthName[11] = new String("十一月");
MonthValue[12] = Calendar.DECEMBER; MonthName[12] = new String("十二月");

try { year = Integer.parseInt(args[0]);
if(String.valueOf(year).length() != 4) throw new Exception("年が4桁でない");
month = Integer.parseInt(args[1]);
if((month < 1) || (month > 12)) throw new Exception("月が1～12でない");
}

catch (NumberFormatException ex) {
System.out.println("入力した数値が異常");
return; }

catch (Exception ex) {
System.out.println(ex.toString());
return; };

cal.set(year, MonthValue[month], cal.getActualMinimum(Calendar.DATE));
week = cal.get(Calendar.DAY_OF_WEEK);
System.out.println(String.valueOf(year) + "年" + MonthName[month]);
for (Int1 = 0; Int1 < 7; Int1 ++) System.out.print(WeekName[Int1]); System.out.println("");
lastDay = cal.getActualMaximum(Calendar.DAY_OF_MONTH);

for (Int1 = 1; Int1 <= lastDay; Int1 ++) {
if (Int1 == 1) {
for (Int2 = 0; WeekValue[Int2] != cal.get(Calendar.DAY_OF_WEEK); Int2 ++)
System.out.print(" ");
};
if (Int1 < 10) System.out.print(" " + Integer.toString(Int1));
else System.out.print(" " + Integer.toString(Int1));
if (cal.get(Calendar.DAY_OF_WEEK) == Calendar.SATURDAY) System.out.println("");
cal.add(Calendar.DAY_OF_MONTH, 1);
};

};

}

　MonthValueとMonthNameは、入力された数値と月を表す定数と表示する文字を結び付けています。
　WeekValueとWeekNameは、週を表す定数と表示する文字とその順番を結び付けています。

回答No.4

hirusagari

>hirusagariさんが提示されたメソッドだと結局「-1」を使っているので意味がないと思うので

>この程度の簡単なプログラムなので無理に分ける必要もないかな、と思います。

結局「-1」を使っているのに、わざわざあのようなメソッドを定義するのは、
Calender#set()の呼び出しが一箇所とは限らないので「-1」をソースコード中にバラまかないようにするためです。
それは、マジックナンバーが避けられるべき理由のひとつが、
そのようなコーディングは仕様変更に耐えられないから、ということだからです。

>switch構文を使ってCalendar.JANUARYを呼び出す

どのようなコードが可読性が高いか、ということは人によってことなるとは思いますが、
「-1」を避けるためにswitchを使って十数行増やすのは自分には明らかに可読性が低下しているように見えます。
なぜなら、既存のコードは、1月→0、2月→1……という対応が「-1」のたった２文字に表されているのに対して、
switchを使うとそれを確かめるのに12行すべてを確認しなければならず、あきらかに読み解くための手間が増えているからです。
また、テストをするときもすべての分岐に対してテストしなければならず、書くコードを増やすことはバグが入る恐れも大きくします(たとえば8月→8と誤って書いてしまうなど)。
「-1」を使わない代償としては損な取引だと思います。

しかも、

case　1:cal.set(year,Calendar.【違いはここだけ】,cal.getActualMinimum(Calendar.DATE));break

という似たようなコードが12行増えるわけです。どう見ても冗長です。

やはり「-1」を無理やり避けようとするのが余計にコードをわかりにくくしています。
「//Month 値は 0 から始まるため -1 する」と書くのが一番わかりやすいと思いますよ。

回答No.3

foggi47

十数行という数字は決して長いものではないと思いますよ。
「-1」を使わない代償、くらいに思っておくべきです。
メソッドに分けると確かに分かりやすくなりますが、結局全体としての行数は長くなりますし、この程度の簡単なプログラムなので無理に分ける必要もないかな、と思います。

hirusagariさんが提示されたメソッドだと結局「-1」を使っているので意味がないと思うので前に自分が示した方法のメソッドを提示しておきます。ただ、見れば分かりますが結局同じことをしてるんですよね…
これ以上ソースを短くする方法がちょっと思いつかないので。(そもそもあるのかな？)

static void setMonth(Calender calender, int year, int month){
switch(month){
case 1:cal.set(year,Calendar.JANUARY,cal.getActualMinimum(Calendar.DATE));break;
case 2:　　　　……　}
}

回答No.2

foggi47

前の質問で先ほど回答させていただいたものです。
窓表示だと思っていたら、DOS窓だったんですね　すいません。
見当違いな答えを提示してしまいました。

回答ですが、もしMonthを-1するのが嫌でしたら、switch構文を使ってCalendar.JANUARYを呼び出す、位のことしかできないでしょう。
例えば以下のようです

switch(month){
case 1:cal.set(year,Calendar.JANUARY,cal.getActualMinimum(Calendar.DATE));;break;
case 2:　　　　……　}

このようにすればたとえJANUARYが1になったとしても(ならないと思いますが)、対応できると思います。

また、hirusagariさんの指摘にもあるように、

>if(month==1||month==3||month==7||month==8||month==10||month==12) {
>lastDay = cal.getActualMaximum(Calendar.DAY_OF_MONTH);
>}else if(month==4||month==6||month==9||month==11){
>lastDay = cal.getActualMaximum(Calendar.DAY_OF_MONTH);
>}else if(year%4==0&&month==2){
>lastDay = cal.getActualMaximum(Calendar.DAY_OF_MONTH);
>}else if(year%4!=0&&month==2){
>lastDay = cal.getActualMaximum(Calendar.DAY_OF_MONTH);
>}

の部分はどの分岐でも同じことをしていますので、分岐の必要はないでしょう。
カレンダークラスはセットされた月の日数を返してくれるので月ごとに分岐する必要はありません。

お礼

色々ありがとうございます。
確かにこのようなやり方もあるいますが、
switch(month){
case 1:cal.set(year,Calendar.JANUARY,cal.getActualMinimum(Calendar.DATE));;break;
case 2:　　　　……　}
ちょっとソースが長くなりすぎてしまうので、メソッドを使ったやり方を教えてください。

回答No.1

hirusagari

Calender#set()の仕様が「Month 値は 0 から始まる (1 月 は 0 になる)」である以上、
-1して値を渡すのは避けられません。
自分はこのような「-1」が「マジックナンバー」であるとは考えていません。
どうしても気になるのであれば、つねに

static void setMonth(Calender calender, int year, int month){
calender.set(year,month-1,cal.getActualMinimum(Calendar.DATE));
}

のようなメソッドを介してCalender#set()を呼び出すことになろうかと思いますが、
これが必ずしも直接Calender#set()を呼び出すより可読性が高いとまでは言えないでしょう。
また、将来的にCalender#set()の仕様が「Month 値は 1 から始まる (1 月 は 1 になる)」に
変更される恐れはまずありませんし、
必要なら「Month 値は 0 から始まるため」というコメントを書いておけば十分でしょう。
従って「month-1」として呼び出すことはとくに問題ないと思います。

そもそもそのような些細なことを気に病むより、

>if(month==1||month==3||month==7||month==8||month==10||month==12) {
>lastDay = cal.getActualMaximum(Calendar.DAY_OF_MONTH);
>}else if(month==4||month==6||month==9||month==11){
>lastDay = cal.getActualMaximum(Calendar.DAY_OF_MONTH);
>}else if(year%4==0&&month==2){
>lastDay = cal.getActualMaximum(Calendar.DAY_OF_MONTH);
>}else if(year%4!=0&&month==2){
>lastDay = cal.getActualMaximum(Calendar.DAY_OF_MONTH);
>}

の部分で、どの分岐でもまったく同じ処理を行うという冗長さのほうが
よほど気になります。
「マジックナンバー」とは何なのか、もう一度考えてみてください。

参考URL：

http://www.geocities.jp/bleis_tift/cpp/badmagicnum.html

お礼

マジックナンバーではないと解釈するかメッソドを作ってみます。参考になりました。ありがとうございます。
それと下のソースですが、
>if(month==1||month==3||month==7||month==8||month==10||month==12) {
>lastDay = cal.getActualMaximum(Calendar.DAY_OF_MONTH);
>}else if(month==4||month==6||month==9||month==11){
>lastDay = cal.getActualMaximum(Calendar.DAY_OF_MONTH);
>}else if(year%4==0&&month==2){
>lastDay = cal.getActualMaximum(Calendar.DAY_OF_MONTH);
>}else if(year%4!=0&&month==2){
>lastDay = cal.getActualMaximum(Calendar.DAY_OF_MONTH);
>}
このソースではいけないのでしょうか？
初心者なのでいまいちよくわからなくてこうしたのですが？
これでも一応うまくいったのですが他に良い方法があるのでしょうか？