• Najnowsze pytania
  • Bez odpowiedzi
  • Zadaj pytanie
  • Kategorie
  • Tagi
  • Zdobyte punkty
  • Ekipa ninja
  • IRC
  • FAQ
  • Regulamin
  • Książki warte uwagi

Code review - prosty program obiektowy "Rodzina"

0 głosów
152 wizyt
pytanie zadane 26 czerwca 2021 w Java przez ITshnyk Obywatel (1,720 p.)

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.

1
komentarz 26 czerwca 2021 przez tkz Nałogowiec (41,900 p.)
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ść.
1
komentarz 26 czerwca 2021 przez Oscar Nałogowiec (27,250 p.)

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.

1
komentarz 26 czerwca 2021 przez Wiciorny Ekspert (230,930 p.)

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

komentarz 26 czerwca 2021 przez ITshnyk Obywatel (1,720 p.)

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

1
komentarz 26 czerwca 2021 przez tkz Nałogowiec (41,900 p.)
Zwróć uwagę na to, co napisał Oscar.
komentarz 26 czerwca 2021 przez Oscar Nałogowiec (27,250 p.)
edycja 26 czerwca 2021 przez 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.

komentarz 27 czerwca 2021 przez Wiciorny Ekspert (230,930 p.)

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 

komentarz 2 lipca 2021 przez ITshnyk Obywatel (1,720 p.)

@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.

Zaloguj lub zarejestruj się, aby odpowiedzieć na to pytanie.

Podobne pytania

+1 głos
2 odpowiedzi 920 wizyt
pytanie zadane 6 sierpnia 2017 w Nasze projekty przez Cyborek Użytkownik (860 p.)
0 głosów
0 odpowiedzi 95 wizyt
pytanie zadane 17 sierpnia 2020 w Java przez DaraS Nowicjusz (120 p.)
0 głosów
1 odpowiedź 365 wizyt

89,082 zapytań

137,669 odpowiedzi

307,605 komentarzy

59,140 pasjonatów

Motyw:

Akcja Pajacyk

Pajacyk od wielu lat dożywia dzieci. Pomóż klikając w zielony brzuszek na stronie. Dziękujemy! ♡

Sklep oferujący ćwiczenia JavaScript, PHP, rozmowy rekrutacyjne dla programistów i inne materiały

Oto dwie polecane książki warte uwagi. Pełną listę znajdziesz tutaj.

...