Ugly Java Code to Refactor

UglyPoker.java

/*
am 15. Juni sind wir von blau.de bei der FH-Wedel Firmenkontaktmesse und 
werden 5 UMTS-Surfsticks[1] verlosen!

Dazu gibt es folgende Aufgabe:
Auf github[2] findet ihr eine kleine, aber ziemlich hässliche Java 
Klasse. Deine Aufgabe ist es, den Code aufzuräumen und lesbarer zu 
machen: Bezeichner, Typen, Strukturen, Methodensignaturen alles kann 
verbessert werden. Anschließend schickst Du Deine Änderungen bis 
spätestens 16. Juni, 23:59 Uhr an [email protected]. Die schönsten 
Refactorings werden von uns ausgesucht und per Email benachrichtigt.


Was zeichnet guten Code aus?

Klar, er muss funktionieren. Wir glauben nicht an Spezifikationen, denn 
Code ist lebendig, er wird ständig erweitert, optimiert, refactored, 
etc.. Specs sind tot. Einmal geschrieben, werden sie kaum angepasst. Was 
lebendiger Code braucht sind Tests, die definieren, wie sich das 
Programm verhalten soll.


Was ist mit Effizienz?

Effizienz ist wichtig, keine Frage. Bloß was bringt es, wenn der Code zu 
sehr optimiert wurde? Er verbraucht vielleicht 5% weniger Ressourcen 
oder implementiert die coolsten Designpatterns[3]. Aber ein Bugfix oder 
ein neues Feature benötigt dafür 10mal mehr Entwicklungszeit? Gut 
lesbarer, sauberer Code ist also kein Selbstzweck, sondern für uns eine 
zwingende Notwendigkeit!


--
Schöne Grüße,
Matthias


[1] http://handyshop.blau.de/handys/blau-de-neues-surfstick-bundle.htm
[2] https://gist.github.com/1006496
[3] nix gegen Designpatterns! Aber wohl dosiert und nur dann eingesetzt, 
wenn sie wirklich sinnvoll sind!
*/

package de.blau.refactormycode;

import java.util.ArrayList;
import java.util.List;

public class UglyPoker {

	public String card1;

	public String card2;

	public String card3;

	public String card4;

	public String card5;


	public UglyPoker(String p1, String p2, String p3, String p4, String p5) {
		this.card1 = p1;
		this.card2 = p2;
		this.card3 = p3;
		this.card4 = p4;
		this.card5 = p5;
	}

	public boolean doCompareHC(String p1, String p2, String p3, String p4, String p5) {
		String hc = "s02";
		List<String> o = new ArrayList<String>();
		List<String> m = new ArrayList<String>();
		o.add(p1);
		o.add(p2);
		o.add(p3);
		o.add(p4);
		o.add(p5);
		m.add(card1);
		m.add(card2);
		m.add(card3);
		m.add(card4);
		m.add(card5);

		for (int i = 0; i < o.size(); i++) {
			String mc = m.get(i);
			for (int j = 0; j < o.size(); j++) {
				String oc = o.get(j);
				if (Integer.valueOf(oc.substring(1)) > Integer.valueOf(mc.substring(1))) {
					if (Integer.valueOf(oc.substring(1)) > Integer.valueOf(hc.substring(1))) {
						hc = oc;
					}
				} else {
					if (Integer.valueOf(mc.substring(1)) > Integer.valueOf(hc.substring(1))) {
						hc = mc;
					}
				}
			}
		}

		return m.contains(hc);
	}
}

Is there a typo in line 45? oc indicates that a card from the 'o' list should be stored, but instead a value from the 'm' list is stored.

You're right. I'll fix this.

That was one of the bugs WE should find & fix. :)

I dont think so. Refactoring is about keeping the behaviour and making clean code, not about finding bugs.

That is what i thought first too, so i asked Andre Jährling, and that is what he told me.