Code review - prosty program obiektowy "Rodzina"

Question

Witam!

Proszę o ocenę krótkiego programu obiektowego w Java. Program w przyszłości chcę rozwinąć aby można było zrobić plik w którym zapisywani są członkowie rodziny, na razie program tylko tworzy nowego członka. Poniżej kod

import java.time.*;
import java.util.Calendar;
import java.util.Scanner;

public class Family {
	public static void main(String[] args) {
		Scanner input = new Scanner(System.in);
		
		int actualYear = Calendar.getInstance().get(Calendar.YEAR);

		String name;
		int year;
		int month;
		int day;
		
		System.out.println("Podaj imię i nazwisko nowego członka w następującej postaci: Jan Kowalski");
		name = input.nextLine();
			
		System.out.println("Podaj rok urodzenia");
		year = input.nextInt();
				
		while(year < 1 || year > actualYear) {
			System.out.println("Podałeś nieprawidłowy rok, spróbuj jeszcze raz");
			year = input.nextInt();
		}
							
			
		System.out.println("Podaj miesiąc urodzenia");
		int lengthOfMonth;
		month = input.nextInt();
		
		while(month > 12 || month < 1) {
			System.out.println("Podałeś nieprawidłowy miesiąc, spróbuj jeszcze raz");
			month = input.nextInt();
		}
			
		if(month == 1) {
			Month birthMonth = Month.JANUARY;
			lengthOfMonth = birthMonth.length(false);
		}else if(month == 2) {
			Month birthMonth = Month.FEBRUARY;
			lengthOfMonth = birthMonth.length(false);
		}else if(month == 3) {
			Month birthMonth = Month.MARCH;
			lengthOfMonth = birthMonth.length(false);
		}else if(month == 4) {
			Month birthMonth = Month.APRIL;
			lengthOfMonth = birthMonth.length(false);
		}else if(month == 5) {
			Month birthMonth = Month.MAY;
			lengthOfMonth = birthMonth.length(false);
		}else if(month == 6) {
			Month birthMonth = Month.JUNE;
			lengthOfMonth = birthMonth.length(false);
		}else if(month == 7) {
			Month birthMonth = Month.JULY;
			lengthOfMonth = birthMonth.length(false);
		}else if(month == 8) {
			Month birthMonth = Month.AUGUST;
			lengthOfMonth = birthMonth.length(false);
		}else if(month == 9) {
			Month birthMonth = Month.SEPTEMBER;
			lengthOfMonth = birthMonth.length(false);
		}else if(month == 10) {
			Month birthMonth = Month.OCTOBER;
			lengthOfMonth = birthMonth.length(false);
		}else if(month == 11) {
			Month birthMonth = Month.NOVEMBER;
			lengthOfMonth = birthMonth.length(false);
		}else{
			Month birthMonth = Month.DECEMBER;
			lengthOfMonth = birthMonth.length(false);
		}
			
		System.out.println("Podaj dzień urodzenia");
		day = input.nextInt();
			
		while(day > lengthOfMonth || day < 1) {
			System.out.println("Podałeś nieprawidłowy dzień, spróbuj jeszcze raz");
			day = input.nextInt();
		}
			
		Member member = new Member(name, year, month, day);
		System.out.println("Imię i nazwisko: " + member.getName() + ", data urodzenia: " + member.getHireDay());
	}
}

class Member{
	private String name;
	private LocalDate hireDay;
	
	public Member(String n, int year, int month, int day) {
		name = n;
		hireDay = LocalDate.of(year, month, day);
	}
	
	public String getName() {
		return name;
	}
	
	public LocalDate getHireDay() {
		return hireDay;
	}
}

Rady i uwagi mile widziane.

Comments

switch zamiast drabinki if'ów byłby lepszym wyborem. Nie piszesz w C, że musisz deklarować zmienne na samym początku. Deklaracja-użycie. Nie deklaracja-długo, długo nic-użycie.

Osobiście if'y, czy switcha, którego zaproponowałem zamieniłbym na mape/tablice. Kluczem byłby numer miesiąca, a wartością jego długość. Analogicznie z tablicą, indeks=numer miesiąca, wartość pod indeksem=dlugość.

---

Na razie program odrzuci kogoś urodzonego w roku przestępnym 29 lutego.

A jeśli chodzi o pomysł z użyciem tablic - bardzo dobry, w dodatku taka tablica już jest:

static Month[]

values() Returns an array containing the constants of this enum type, in the order they are declared.

---

@tkz,nawet zamiast switcha zrobiłbym w tym wypadku jakąś strategie wzorcową, albo w stream :) filtrer wpakował 

---

Dziękuję za wskazówki!

 

switch zamiast drabinki if'ów byłby lepszym wyborem. Nie piszesz w C, że musisz deklarować zmienne na samym początku. 

Zmieniłem 

Na razie program odrzuci kogoś urodzonego w roku przestępnym 29 lutego.

poprawiłem to 

Teraz lepiej?

import java.time.*;
import java.util.Calendar;
import java.util.Scanner;

public class Family {
	public static void main(String[] args) {
		Scanner input = new Scanner(System.in);
		
		int actualYear = Calendar.getInstance().get(Calendar.YEAR);
		
		System.out.println("Podaj imię i nazwisko nowego członka w następującej postaci: Jan Kowalski");
		String name = input.nextLine();
			
		System.out.println("Podaj rok urodzenia");
		int year = input.nextInt();
				
		while(year < 1 || year > actualYear) {
			System.out.println("Podałeś nieprawidłowy rok, spróbuj jeszcze raz");
			year = input.nextInt();
		}
							
			
		System.out.println("Podaj miesiąc urodzenia");
		int lengthOfMonth;
		int month = input.nextInt();
		
		while(month > 12 || month < 1) {
			System.out.println("Podałeś nieprawidłowy miesiąc, spróbuj jeszcze raz");
			month = input.nextInt();
		}
		
		Month birthMonth;
		
		switch(month) {
		case 1:
			birthMonth = Month.JANUARY;
			lengthOfMonth = birthMonth.length(false);
			break;
		case 2:
			birthMonth = Month.FEBRUARY;
			lengthOfMonth = birthMonth.length(false);
			if(year%4 == 0) {
				lengthOfMonth++;
			}
			break;
		case 3:
			birthMonth = Month.MARCH;
			lengthOfMonth = birthMonth.length(false);
			break;
		case 4:
			birthMonth = Month.APRIL;
			lengthOfMonth = birthMonth.length(false);
			break;
		case 5:
			birthMonth = Month.MAY;
			lengthOfMonth = birthMonth.length(false);
			break;
		case 6:
			birthMonth = Month.JUNE;
			lengthOfMonth = birthMonth.length(false);
			break;
		case 7:
			birthMonth = Month.JULY;
			lengthOfMonth = birthMonth.length(false);
			break;
		case 8:
			birthMonth = Month.AUGUST;
			lengthOfMonth = birthMonth.length(false);
			break;
		case 9:
			birthMonth = Month.SEPTEMBER;
			lengthOfMonth = birthMonth.length(false);
			break;
		case 10:
			birthMonth = Month.OCTOBER;
			lengthOfMonth = birthMonth.length(false);
			break;
		case 11:
			birthMonth = Month.NOVEMBER;
			lengthOfMonth = birthMonth.length(false);
			break;
		case 12:
			birthMonth = Month.DECEMBER;
			lengthOfMonth = birthMonth.length(false);
			break;
		default:
			birthMonth = null;			
			lengthOfMonth = 0;
		}
			
		System.out.println("Podaj dzień urodzenia");
		int day = input.nextInt();
			
		while(day > lengthOfMonth || day < 1) {
			System.out.println("Podałeś nieprawidłowy dzień, spróbuj jeszcze raz");
			day = input.nextInt();
		}
			
		Member member = new Member(name, year, month, day);
		System.out.println("Imię i nazwisko: " + member.getName() + ", data urodzenia: " + member.getHireDay());
	}
}

class Member{
	private String name;
	private LocalDate hireDay;
	
	public Member(String n, int year, int month, int day) {
		name = n;
		hireDay = LocalDate.of(year, month, day);
	}
	
	public String getName() {
		return name;
	}
	
	public LocalDate getHireDay() {
		return hireDay;
	}
}

P.S oczekuję jeszcze więcej uwag, ale na razie jeszcze nie chcę wykorzystywać kolekcji

---

Zwróć uwagę na to, co napisał Oscar.

---

Metoda Month.length(boolean)  dostaje parametr, który określa czy chodzi o rok przestępny (hasło leap year), więc nie musisz sprawdzać warunku i modyfikować wyniku tylko ten warunek daj jako argument length(). Możesz go śmiało przekazać do każdego wywołania - poza lutym jest bez znaczenia, nic nie zmieni, a kod będzie bardziej czytelny - raz obliczysz, zapamiętasz w zmiennej boolean i podasz do wywołania.

Sam warunek na rok przestępny jest nieco bardziej złożony niż to co napisałeś, ale przyjmijmy takie uproszczenie.

 

Tam w klasie od razu jest funkcja zwracająca obiekt miesiąca po numerze (1-styczeń), więc napisałbym (kod bez sprawdzenia):

            birthMonth = Month.of(month);
            lengthOfMonth = birthMonth.length(year%4 == 0);

Java to jest taki dość prosty język, posiadający jednak bardzo zaawansowaną bibliotekę standardową. Może ona nie obsługuje wszystkiego, ale jak już coś ma to zwykle na tyle przemyślane, że wszelkie możliwości są obsłużone.

---

Java to jest taki dość prosty język, posiadający jednak bardzo zaawansowaną bibliotekę standardową

no to teraz poleciałeś po bandzie. Szczególnie w zakresie algorytmów GCC, oraz  kontroli pamieci JVM :) która sama sobie przeczy 

---

@Oscar,

Dziękuję za wyjaśnienie, mam nadzieję, że prawidłowo to zastosowałem. Poniżej uaktualniony kod:

import java.time.*;
import java.util.Calendar;
import java.util.Scanner;

public class Family {
	public static void main(String[] args) {
		Scanner input = new Scanner(System.in);
		
		int actualYear = Calendar.getInstance().get(Calendar.YEAR);
		
		System.out.println("Podaj imię i nazwisko nowego członka w następującej postaci: Jan Kowalski");
		String name = input.nextLine();
			
		System.out.println("Podaj rok urodzenia");
		int year = input.nextInt();
				
		while(year < 1 || year > actualYear) {
			System.out.println("Podałeś nieprawidłowy rok, spróbuj jeszcze raz");
			year = input.nextInt();
		}
									
		System.out.println("Podaj miesiąc urodzenia");
		int lengthOfMonth;
		int month = input.nextInt();
		
		while(month > 12 || month < 1) {
			System.out.println("Podałeś nieprawidłowy miesiąc, spróbuj jeszcze raz");
			month = input.nextInt();
		}
		
		Month birthMonth;
		birthMonth = Month.of(month);
		lengthOfMonth = birthMonth.length(year%4 == 0);
			
		System.out.println("Podaj dzień urodzenia");
		int day = input.nextInt();
			
		while(day > lengthOfMonth || day < 1) {
			System.out.println("Podałeś nieprawidłowy dzień, spróbuj jeszcze raz");
			day = input.nextInt();
		}
			
		Member member = new Member(name, year, month, day);
		System.out.println("Imię i nazwisko: " + member.getName() + ", data urodzenia: " + member.getBirthDate());
	}
}

class Member{
	private String name;
	private LocalDate birthDate;
	
	public Member(String n, int year, int month, int day) {
		name = n;
		birthDate = LocalDate.of(year, month, day);
	}
	
	public String getName() {
		return name;
	}
	
	public LocalDate getBirthDate() {
		return birthDate;
	}
}

Zmieniłem też nazwy pola i metody w klasie Member na birthDate i getBirthDate.