Konstruktiv kritikk av mitt første program i Java

Nordmoen · 31. januar 2009

Heisann godt å få prøvd seg litt på java, endelig.

Jeg ønsker, hvis noen gidder å ta seg tid, å få litt konstruktiv kritikk på et program jeg har laget. Har programmert litt i python før og litt i C#, men ikke i nærheten av sånn som nå og i tillegg så er det første gang med klasser osv. Det jeg ønsker å høre er hva som kan forbedres av absolutt hele programmet(kanskje uten om grammatikken selv om den sikkert er like dårlig som programmet), vet at jeg burde ha kommentert men det er sent så det får bli morgen dagens prosjekt.

Her er koden:

import java.util.*;


public class blackJack {

/**
 * @param args
 */
public static void main(String[] args) {
	System.out.println("Welcome to this wonderfull Blackjack game!");
	System.out.println("------------------------------------------");
	System.out.println("Please choose an option below:");
	choice menu = new choice();
	menu.newGame();

}

}
class choice{
void newGame(){
	printMenu();
	game newGame = new game();
	Scanner in = new Scanner(System.in);
	int choice = in.nextInt();
	while(choice!=5){
		if(choice==1){
			newGame.empty();
			choice = in.nextInt();
		}
		else if(choice==2){
			newGame.sumPick("Player");
			choice = in.nextInt();
		}
		else if(choice==3){
			newGame.printScore();
			choice = in.nextInt();
		}
		else if(choice==4){
			printMenu();
			choice = in.nextInt();
		}
		else{
			System.out.println("That was not an option =)");
			choice = in.nextInt();
		}
	}
		System.out.println("Quiting now");
		System.exit(0);

}
private void printMenu(){
	System.out.println("Menu:");
	System.out.println("Press 1 for new Game");
	System.out.println("Press 2 for new card");
	System.out.println("Press 3 to stay with the card you have");
	System.out.println("Press 4 for menu");
	System.out.println("Press 5 to quit");
}
}
class game{
private int[][] deck = new int [13][4];
private double playerSum = 0;
private double dealerSum = 0;

private void newDeck(){
	for(int i =0; i<4;i++){
		for(int k =0; k<13;k++){
			deck[k][i]=1;
		}
	}
}
private String cardOver10(int card){
	if(card==11){
		return "Jack ";
	}
	else if(card==12){
		return "Queen ";
	}
	else if(card==13){
		return "King ";
	}
	else if(card==1){
		return "Ace ";
	}
	else{
		return card+"";
	}
}
void sumPick(String player){
	double card = pick(player);
	if(card>=10){
		playerSum +=10;
	}
	else if(card==1){
		aceChoice();
	}
	else if(card<10){
		playerSum +=card;
	}
	else{
		errorMessage("sumPick");
	}
	System.out.println("You got "+ playerSum);
}
private void aceChoice(){
	System.out.println("Ace's can be either 1 or 11 choose(by typing either 1 or 11 for you choice):");
	Scanner in = new Scanner(System.in);
	int aceChoice = in.nextInt();
	if(aceChoice==1){
		playerSum +=1;
	}
	else if(aceChoice==11){
		playerSum += 11;
	}
	else{
		errorMessage("aceChoice");
	}
}
private double dealerSuperPick(String dealer){
	double card = pick("Dealer");
	if(card>=10){
		return 10;
	}
	else if(card<10){
		return card;
	}
	else{
		errorMessage("dealerSuperPick");
		return 0;
	}
}
private double pick(String player){
	Random randomGenerator= new Random();

	int card = randomGenerator.nextInt(13);
	int colour = randomGenerator.nextInt(4);

	while(deck[card][colour]!=0){
		if(colour==0){
			System.out.println(player+ " got "+ cardOver10(card+1)+ " of hearts" );
			return (card+1);
		}
		else if(colour==1){
			System.out.println(player+ " got "+ cardOver10(card+1)+ " of spades" );
			return (card+1);
		}
		else if(colour==2){
			System.out.println(player+ " got "+ cardOver10(card+1)+ " of diamonds" );
			return (card+1);
		}
		else if(colour==3){
			System.out.println(player+ " got "+ cardOver10(card+1)+ " of clubs" );
			return (card+1);
		}
		else{
			errorMessage("pick()");
			return 0;
		}		
	}
	return 0;
}
void printScore(){
	dealer();

	if(playerSum>21 && dealerSum<=21){
		System.out.println("You got "+playerSum +" which is over 21, dealer got "+dealerSum+ " and wins");
	}
	else if(playerSum==21 && dealerSum!=21){
		System.out.println("You got Blackjack, dealer didn't stand a chance!");
	}
	else if(dealerSum>21 && playerSum<=21){
		System.out.println("Dealer got "+dealerSum+" which is over 21, you won with "+playerSum);
	}
	else if(playerSum>dealerSum){
		System.out.println("You won with "+ playerSum+" which is higher than dealer with his "+dealerSum);
	}
	else if(dealerSum>playerSum){
		System.out.println("You lost with "+ playerSum+" which is lower than dealer with his "+dealerSum);
	}
	else if(dealerSum==playerSum){
		System.out.println("It was a tie, you both got "+playerSum);
	}
	else{
		errorMessage("printScore");
		System.out.println("You got "+ playerSum +" dealer got "+ dealerSum);
	}
	empty();
}
void empty(){
	newDeck();
	playerSum=0;
	dealerSum=0;
}
private double dealer(){
	while(dealerSum<17){
		if(dealerSum<=13 && dealerSum<playerSum){
			dealerSum +=dealerSuperPick("Dealer");
		}
		else if(dealerSum>13 && dealerSum<17 || dealerSum<playerSum){
			dealerSum +=dealerSuperPick("Dealer");
		}
		else if(dealerSum>17 && dealerSum<20 || dealerSum<playerSum){
			Random randomGenerator= new Random();
			int shouldDealerGetNewCard = randomGenerator.nextInt(5);
			if(shouldDealerGetNewCard==2){
				dealerSum +=dealerSuperPick("Dealer");
			}
			else{
				break;
			}
		}
		else{
			break;
		}
	}
	return dealerSum;
}
private void errorMessage(String error){
	System.out.println("An error was registred, the error was in "+error);
}
}

Kan også ved java filen hvis det er enklere, hvis det er noen spørsmål så skal jeg selvsagt prøve å svare så godt jeg kan på de

RavnTM · 31. januar 2009

Jeg er litt trøtt så jeg kan ta feil, men her er de tingene jeg ville ha endret:

Følg Java konvensjonen! Du finner den her
F.eks skal klassenavn alltid starte med stor bokstav.

Bruk switch-statements. (her)
Når du skal sjekke om en variabel har spesifikke verdier, er det lettere å bruke switch istedenfor mange if/else if/else.

switch (value) {
 case 1: actionOne(); break;
 case 2: actionTwo(); break;
 case 3: actionThree(); break;
 default: noneOfTheAbove(); break;
}

ser mye bedre ut enn

if (value == 1) {
 actionOne();
} else if (value == 2) {
 actionTwo();
} ... else {
 noneOfTheAbove();
}

Bruk mer mellomrom.
if (val <= 4 && val != 2)
er lettere å lese enn
if(val<=4&&val!=2)

Jeg ser også at du hele tiden lager nye instanser av Random-klassen, hvorfor? Du burde bare ha én instans av den som en klasse-variabel du kan bruke rundt om i hele programmet.

Ellers ser det veldig fint ut til å være ditt første program!

Nordmoen · 31. januar 2009

Tusen takk for tilbakemeldingen

Visste ikke om den navngivingen, men skal selvsagt ta den til etterretning fra nå av. Skal også se på switch. Ble bare litt seint mot slutten av programmet, så har også funnet ut at det er enkelte ting jeg ville forandre på.

En annen ting jeg er veldig usikker på er om class game, burde vært oppdelt i flere klasser eller blir det riktig? Skjønner at class choice ikke hadde trengt å være der, noe jeg skal endre på.

edit:

En liten ting til, mener du å bruke switch f.eks i printScore, eller mener du at jeg skal bruke det når det kommer til menyen?

Endret 31. januar 2009 av Spec-ops-j

___ · 31. januar 2009

Her er det så mye å ta tak i at jeg ikke vet hvor jeg skal begynne, så jeg lar heller være. Jeg vil anbefale deg å kjøpe en god Java-bok, og starte på kapittel 1. Det er en fordel om boka er av nyere dato, og tar for seg generics, osv.

Lykke til...

Werner

Cyberfrog · 31. januar 2009

Switch-blokker er ikke noe som alltid kan erstatte en rekke med if-setninger. I dette tilfellet kan det muligens gjøre menykoden mer lesbar/konsis, mens switch egner seg dårlig til bruk i printScore() slik koden er nå (den har litt mer innfløkt logikk enn "hvis 1 eller hvis 2 eller hvis 3 eller...").

Men dette er ikke noe fasitsvar. Du må selv vurdere hvorvidt det er hensiktsmessig å bruke switch eller if i koden (og hvorvidt gevinsten ved å bruke det ene istedet for det andre gjør det verdt å skrive om kode).

Når det gjelder inndeling i klasser er dette også en vurderingssak. Hvor hører en funksjonalitet hjemme? Trenger programmet ny funksjonalitet som ikke logisk hører hjemme i noen av de eksisterende klassene? Utfører en eksisterende klasse 2 eller flere oppgaver som ikke logisk hører sammen? Er f.eks. enig i valget om å fjerne choice-klassen, men jeg synes ikke menyen i sin helhet hører hjemme i game heller (presiserer at dette er min mening, og at det er lov å være uenig ). Uten å ha tenkt grundig over det, kanskje jeg ville gått for "new card" og "stay" i game, og "start new game" og "quit game" eller noe slikt i main()-metoden.

Kanskje jeg ser litt nøyere på koden senere, men ved første øyekast ser det ganske bra ut.

steingrim · 31. januar 2009

Her er det så mye å ta tak i at jeg ikke vet hvor jeg skal begynne, så jeg lar heller være. Jeg vil anbefale deg å kjøpe en god Java-bok, og starte på kapittel 1. Det er en fordel om boka er av nyere dato, og tar for seg generics, osv.

Lykke til...

Jøsses, du har tydeligvis ikke sett særlig mye nybegynner-kode. Dette var slettes ikke verst til å være et første forsøk i Java.

Hilsen en som har sett og rettet mange, mange obliger

Nordmoen · 31. januar 2009

steingrim:

Hva ville du endret på tatt i betraktning forutsetningene, evt. hva burde jeg øve på i neste program?

Cyberfrog:

Takk for "pointerene", skal ta det i betraktning

x871kx6167ss7 · 31. januar 2009

God start dette her!

Diverse ting av ulik "viktighet"...

For det første så skal klasser ha stor forbokstav. Altså BlackJack, Choice, Game osv. Ikke det at kompilatoren bryr seg, men andre gjør det.

Som nevnt så kunne koden med fordel vært mer luftig, altså mellomrom mellom operatorer som +, -, =, osv (med untak av ++ og --).

Se på denne kodebiten:

if (choice == 1) {
....
choice = in.nextInt();
} else if (...) {
...
choice = in.nextInt();
} else {
System.out.println("That was not an option =)");
choice = in.nextInt();
}

Her skriver du linjen «choice = in.nextInt();» mer enn nødvendig. Den skal jo uansett bli kjørt en gang i løpet av løkken, så du kan uansett bare legge den på slutten.

Også i pick-metoden lurer jeg på hvorfor du har en while-løkke. Uansett hvilken gren av if-setningen den går inn i vil den jo returnere. Skulle kanskje vært en if-settning i stede? Og som du også ser, så gjør den nesten det samme i alle grenene. Også tror jeg du kan stole på at randomGenerator.nextInt(4) er mellom 0 og 3.

Random randomGenerator = new Random();

int card = randomGenerator.nextInt(13);
int colour = randomGenerator.nextInt(4);
String [] colourNames = {"hearts", "spades", "diamonds", "clubs"};

if (deck[card][colour] == 0) {
return 0;
} else {
System.out.println(player + " got " + cardOver10(card + 1) + " of " + colourNames[colour]);
return card + 1;
}

Cyberfrog · 31. januar 2009

OK, jeg har sett litt mer på koden. Finner ikke stort å legge til det andre har sagt, men føler for å tipse deg om enums (selv om du selvfølgelig kan ha vært borti dem i C# allerede). Her er f.eks. en alternativ måte å representere kort på:

enum Value {
ACE, DEUCE, THREE, FOUR, FIVE, SIX, SEVEN, EIGHT, 
NINE, TEN, JACK, QUEEN, KING
}

enum Suit {
CLUBS, HEARTS, SPADES, DIAMONDS
}

class Card {
private Value value;
private Suit suit;

Card(Value value, Suit suit) {
	this.value = value;
	this.suit = suit;
}


public String toString() {
	return value + " of " + suit;
}
}

Ironisk nok skrev jeg dette for hånd før jeg lette etter en side som forklarer enums bedre enn jeg kunne gjort (og oppdaget bl.a. et bedre navn for kortverdier): http://java.sun.com/j2se/1.5.0/docs/guide/...uage/enums.html

For evt. fremtidige prosjekter kan det også være nyttig å leke litt med Collections-rammeverket (http://java.sun.com/docs/books/tutorial/co...ions/index.html). Og for å gjøre det, kan det ikke skade å først vite litt om generics (http://java.sun.com/docs/books/tutorial/ja...rics/index.html).

Endret 31. januar 2009 av Cyberfrog

Nordmoen · 31. januar 2009

Kjempe mye bra og ta tak i, takk skal dere ha

Blackslash:

Grunnen til at det er en While løkke er at den jeg glemte å gjøre om verdien av deck[card][colour]=0; når while løkken ble ferdig, sånn at den ikke skulle ta det samme korte flere ganger.

pgdx · 1. februar 2009

class Card {
private final Value value;
private final Suit suit;

Card(Value value, Suit suit) {
	this.value = value;
	this.suit = suit;
}

public Suit getSuit() {
	return suit;
}

public Value getValue() {
	return value;
}

@Override
public String toString() {
	return value + " of " + suit;
}
}

La til noe greier, du MÅ ha equals og hashCode implementert også. I tillegg kunne (burde?) Suit ha en konstruktør som tok inn verdien på kortet.

Endret 1. februar 2009 av pgdx

Cyberfrog · 1. februar 2009

Det siste er jeg uenig i. Men uten equals og hashCode vil Collections-klasser ikke alltid oppføre seg som forventet, så det var et viktig innspill. Samtidig som man ser på equals/hashCode kan det også være greit å se på compareTo (som kan brukes til å definere en "naturlig orden" blant objekter av klassen) - http://java.sun.com/javase/6/docs/api/java...Comparable.html.

Forøvrig har jeg aldri påstått at Card-klassen var fullstendig. Eneste poenget var gi et enkelt eksempel med bruk av enums. Har hatt en foreleser som krevde at en haug med metoder ALLTID MÅTTE implementeres, uansett om det var behov for dem for å løse oppgaven eller ikke (dette førte til at alle tok snarveier, som å implementere compareTo() for så bare å skrive "return 0"), så posten din vekker litt ekle minner.

steingrim · 1. februar 2009

steingrim:
Hva ville du endret på tatt i betraktning forutsetningene, evt. hva burde jeg øve på i neste program?

Objekt-orientering. Jeg ser det er kommet noen forslag til å representere kort og kortstokker. Til ditt neste prosjekt bør objekt-orientering være et tema! Et kortspill er i høyeste grad noe som elegant kan implementeres med en domene-modell adskilt fra resten av programmet.

En annen ting er at du bør unngå å gjenta deg selv. Det finnes en trebokstavers-forkortelse for dette: DRY = Don't Repeat Yourself. Hver gang du gjentar deg selv så bør du tenke "hm, hvordan kan jeg gjøre dette uten gjentakelser?" -- resultatet er ofte at koden får et enklere design.

Ved å tenke DRY konsekvent vil du også ofte merke at du skriver mange små metoder. Det er bra! Metodene dine trenger ikke være så lange.

Men fokusér først og fremst på objekt-orientering.

Logg inn

Konstruktiv kritikk av mitt første program i Java

Anbefalte innlegg

Nordmoen

Lenke til kommentar

Videoannonse

RavnTM

Lenke til kommentar

Nordmoen

Lenke til kommentar

___

Lenke til kommentar

Cyberfrog

Lenke til kommentar

steingrim

Lenke til kommentar

Nordmoen

Lenke til kommentar

x871kx6167ss7

Lenke til kommentar

Cyberfrog

Lenke til kommentar

Nordmoen

Lenke til kommentar

pgdx

Lenke til kommentar

Cyberfrog

Lenke til kommentar

steingrim

Lenke til kommentar

Opprett en konto eller logg inn for å kommentere

Opprett konto

Logg inn

Hvem er aktive 0 medlemmer