Poprawność kodu prostej gry konsolowej

Witajcie.
Z nudów pisze sobie takie coś na kształt prostej gierki konsolowej. Staram się na podstawie inicjatywy ustalić kolejność. Chodzi o to jak da się to zrobić łatwiej i ładniej ponieważ mam wrażenie, że to jak zrobiłem nie jest dobrze.

	private RoundOrder initiativeCoparision(Creature c1, Creature c2) {
		RoundOrder order = new RoundOrder();
		//porównuje inicjatywe a wypadku takiej samej ustala losowo
		if(c1.getInitiative()==c2.getInitiative()){
			if(r.nextBoolean()){  
				order.first=c1;
				order.second=c2;
			}else{
				order.first=c2;
				order.second=c1;
			}
		}
		else if(c1.getInitiative()>c2.getInitiative()){
			order.first=c1;
			order.second=c2;
		}else{
			order.first=c2;
			order.second=c1;
		}
		
		// Mam wrazenie ze te ify nie wygladaja ladnie
		
		return order;
	}
	//Oraz nie wiem czy nie kombinuje zabardzo z osobnym obiektem który ma w sobie trzymać kolejnosc
	private class RoundOrder{
		private Creature first;
		private Creature second;
	}

Pozdrawiam ;)

poprawienie tytułu wątku - furious programming

No ale co to ma robić?

Skrócić można np.tak:

int initiative1 = c1.getInitiative(), initiative2 = c2.getInitiative();

if (initative1 == initiative2)
 order.first = r.nextBoolean()?c1:c2; else

if (initiative1 > iniatitive2)
 order.first = c1;

/* inne branche - także modyfikujące tylko "first" */

if (order.first == c1)
 order.second = c2; else
 order.second = c1;

if (((c1.getInitiative() == c2.getInitiative()) && r.nextBoolean()) || (c1.getInitiative() > c2.getInitiative())) {
  order.first = c1;
  order.second = c2;
} else {
  order.first = c2;
  order.second = c1;
}

Względnie, jeśli getInitiative() zwraca coś mniejszego niż 2^30 to można zrobić:

if (c1.getInitiative() * 2 + r.next(1) > c2.getInitiative() * 2) {
  order.first = c1;
  order.second = c2;
} else {
  order.first = c2;
  order.second = c1;
}

Ale to już jest trikowe.

Jeśli r jest typu Random, a wersja Javy >= 7 to można rozważyć zmianę na ThreadLocalRandom.

Ale dlaczego w ten sposób?

class RoundOrder{

	private final Creature[] creatures;

	public RoundOrder(Creature...creatures){
		// kopia defensywna, niekonieczna, ale 
                this.creatures  = creatures;
		Arrays.sort(this.creatures, (a, b) -> {
			if(a.initiative == b.initiative) {
				// jeżeli równa to losujemy z zakresu [0, 2) i odejmujemy 1 
				// by mieć zakres [-1, 1)
				return ThreadLocalRandom.current().nextInt(2) - 1;
			}
			return a.initiative - b.initiative;
		});
	}

}

Wersja dla Javy 8 oczywiście. Jeżeli inicjatywy są równe to wybieramy stan losowy. Inaczej porządkujemy je malejąco.

// edit

Zastanawia mnie dlaczego RoundOrder u ciebie jest prywatny. Nie ma to sensu. Lepiej trzymać go jako niezmienny osobny obiekt.

Koziołek napisał(a):

		Arrays.sort(this.creatures, (a, b) -> {
			if(a.initiative == b.initiative) {
				// jeżeli równa to losujemy z zakresu [0, 2) i odejmujemy 1 
				// by mieć zakres [-1, 1)
				return ThreadLocalRandom.current().nextInt(2) - 1;
			}
			return a.initiative - b.initiative;
		});

to bardzo zły algorytm mieszania
polecam http://bost.ocks.org/mike/algorithms/#shuffling

Niedeterministyczny komparator to nie jest dobre rozwiązanie. Nie zachowuje kontraktu.

Komparator jest ad hoc robiony i nie jest ujawniany. Zresztą to co chcemy tu uzyskać to takie niedeterministyczne zachowanie w pewnych przypakach. Skoro klasa RoundOrder będzie krótkożyjąca to i komparator umrze wraz z nią. W ramach pojedynczej rundy wszystko jest spójne, bo kolejność ustalana jest jednorazowo na samym początku w konstruktorze.
Można jeszcze pobawić się w stworzenie metod z niejawnym konstruktorem kopiującym:

public RoundOrder nextRound(){
     return new RoundOrder(this);
}

private RoundOrder(RoundOrder prev){
    this.creatures = prev.creatures; // kopia defensywna w pewnych przypadkach!
}

Wtedy kolejność jest spójna w całej rozgrywce.

Oczywiście klasa Creature może implementować domyśłny komparator. Tu będziemy go nadpisywać.

// edit: Zresztą nie chodzi tu o komparator, bo to jest tylko użycie interfejsu, który jest pod ręką w celu uogólnienia przypadku. Przy N >2 kreatur, ifologia rozrośnie się do przerażajacych rozmiarów.

Przy komparatorze nie spełniającym kontraktu możesz dostać błędy typu: http://stackoverflow.com/questions/7849539/comparison-method-violates-its-general-contract-java-7-only

OK. Zgoda. Tyle tylko, że tu takiego problemu raczej nie będzie, bo zbiór jest bardzo mały (2 elementy). Pytanie czy samodzielne implementowanie sortowania i komparatora jest sensowne. Bo do tego sprowadza się ten wątek.

// edit i zawsze zostaje ustawić parametry wyłączające sprawdzenie kontraktu.

Dzięki wielkie wam za pomoc ;) jak tylko wrócę to postaram się to zrozumieć ;) co do liczby to zawsze będą to dwaj przeciwnicy ale w sumie z tym sortowaniem lepiej to wygląda ;)

O i jeszcze jedno. Można do klasy losującej podać jakiś rozsądny seed (iloczyn hashCode-ów na ten przykład). Tak by mieć kontrolę nad losowością zachowania.

Jeśli ograniczysz losowość to gdy przeciwnicy będą w kółko walczyć ze sobą to jeden będzie cały czas wygrywał.

Myślę, że moja pierwsza propozycja jest w miarę czytelna, zwięzła i ogólnie wystarczająca :]