C++ Schiffe Versenken

jkhsjdhjs

Hi,

ich hab einfach so aus Spaß an der Freude Schiffe Versenken im C++ Konsolenfenster programmiert und wollte fragen, ob jemand vielleicht wenn er Zeit hat mal über den Code drüberschaun könnte, und mir sagen könnte, was ich da noch besser bzw. kürzer oder anders machen sollte, da ich noch ein ziemlicher Anfänger bin.

Hab den Code auf pastebin gepostet, da der zu lang fürs Forum wäre.
http://pastebin.com/uHWqBe8B

Das Programm funktionert auch soweit, außer das mit den Schiffe anzeigen lassen, wenn man verloren hat.
Und das Programm kann bis jetzt auch nur Schiffe, die ein Feld groß sind.

LG

jkhsjdhjs

Skym0sh0

Teil den Code in viel mehr Funktionen auf.
Teil den Code auf verschiedene Dateien auf.

Globale Variablen sind designtechnisch schlecht.
Rohe Arrays wie du sie ntuzt sind suboptimal, nimm std::array oder std::vector.
Teil deine Funktionalität auf in Logik und Anzeige, du hast beides verschmolzen.

Der nächste Schritt wäre Objektorientierung.

jkhsjdhjs

Ok, Danke für die Antwort.
Wie kann ich denn den Code auf mehrere Dateien aufteilen?
Habe sowas noch nie gemacht.

Ja, das Globale Variablen schlecht sind, weiß ich, aber in den Fällen fand ich das einfacher. Es sind ja auch nicht alle Variablen global.

Was ist denn der Unterschied zwischen std::array oder std::vector zwischen den normalen arrays wie ich sie nehme?

Nathan

jkhsjdhjs schrieb:

Was ist denn der Unterschied zwischen std::array oder std::vector zwischen den normalen arrays wie ich sie nehme?

std::vector wächst dynamisch, std::array hat diese dämliche Array-to-Pointer-Decay nicht drin und ist somit einfacher zu handeln (übergib mal ein rohes Array by Value an eine Funktion).

zeile 5: kenne den header net
zeile 7: nicht standardkonform -> <clocale>
zeile 8: nicht standardkonform -> <ctime>
zeile 9: achtung, plattform-anhängig
zeile 11: kenne den header net
zeile 16 - 31: hätte man viel schöner mit enumatoren lösen können
zeile 33 - 36: da lässt sich sicher vieles in klassen kapseln und muss auch nicht global sein
zeile 38 - 41: gibt effizientere methoden, z.b. "vec(2); vec[] = ...", dann musst du nicht unnötig alloziieren und rumkopieren obwohl du die grösse eh kennst
zeile 45 - 47: -> std::pair<int, int>
zeile 49 - 57: sehr gut dass du das in eine funkion getan hast kann zum code selbst aber nix sagen weil ich ka habe...
zeile 59 - 61: wäre ein wenig schöner wenn du das alles in eine klasse als static member oder in einen namespace packen würdest. (also auch die farben dann als enumatoren oder als statische konstante member)
zeile 63 - 110: da blicke ich nicht wirklich durch aber ich sehe schon dass da gewisse dinge redunant sind, die kann man bestimmt mit besseren strukturen vermeiden. allgemein sagt man auch dass compiler switches besser optimieren können als solche if-ketten, ist aber für dich hier kein argument, ich weiss...
zeile 112 - 255: riiiiesen funktion, hätte man auch einfach so schreiben können:

Pair Koordinatenverarbeitung(int x, int y) {
        Pair Rückgabewert;
		if(std::tolower(x) >= 'a' && std::tolower(x) <= 'q')
			Rückgabewert.Eins = 20 + (std::tolower(x) - 'a') * 10;
		else
			Rückgabewert.Eins = 0;
        if(y >= '1' && y <= '8')
			Rückgabewert.Zwei = 10 + (y - '1') * 6;
		else
			Rückgabewert.Zwei = 0;
        return Rückgabewert;
}

so bitte, 12 zeilen. du musst üben, muster zu erkennen damit du algorithmen schreiben kannst statt tabellen zu nutzen
zeile 264 - 276: statt das fünf mal zu schreiben hättest du doch eine for-schleife machen können:

Farbe(farbe);
        for(int i = -2; i <= 2; i++)
        {
                gotoxy(x - 4, y + i);
                cout<<"XXXXXXXXX";
        }

ist wieder sowas was ich beim letzten punkt angesprochen habe

an dieser stelle höre ich mal auf, mit den tipps die ich dir gegeben habe dürftest du genug zu tun haben wenn du sie auf dein ganzes programm anwendest.

mfg

jkhsjdhjs

Ok, Danke! Hab jetzt mal was gegooglet und komme jedoch noch nicht so ganz damit klar. Könnte mir vielleicht jemand mal anhand von:

int Schiffe[4][2]

zeigen, wie das geht?

Ich käme ja auf hier das:

array<int, 4, 2> Schiffe;

Bekomme aber Fehler. Gehen bei std::array zwei Dimensionen überhaupt?

Und @asdfsdf:

Danke, dass du dir den Code genauer angeschaut hast . Werde deine Ratschläge befolgen!

SeppJ

jkhsjdhjs schrieb:

Gehen bei std::array zwei Dimensionen überhaupt?

Das wäre dann ein Array von Arrays. Das hast du bei int[4][2] auch, aber du merkst es vielleicht nicht.

array<array<int, 2>, 4>

Ein typedef bietet sich an.

Skym0sh0

Mach dich auf jedenfall drauf gefasst, dass hier viele Dinge noch genannt werden, die du noch nie gehört hast. Auch wird dir dein Code früher oder später komplett auseinander genommen werden. Dem ein oder anderen geht das persönlich sehr nahe, was es eigentlich nicht braucht.

Denn:

die 10-Programmierer Gebote schrieb:

Understand and accept that you will make mistakes. The point is to find them early, before they make it into production. Fortunately, except for the few of us developing rocket guidance software at JPL, mistakes are rarely fatal in our industry, so we can, and should, learn, laugh, and move on.

You are not your code. Remember that the entire point of a review is to find problems, and problems will be found. Don't take it personally when one is uncovered.

No matter how much "karate" you know, someone else will always know more. Such an individual can teach you some new moves if you ask. Seek and accept input from others, especially when you think it's not needed.

Don't rewrite code without consultation. There's a fine line between "fixing code" and "rewriting code." Know the difference, and pursue stylistic changes within the framework of a code review, not as a lone enforcer.

Treat people who know less than you with respect, deference, and patience. Nontechnical people who deal with developers on a regular basis almost universally hold the opinion that we are prima donnas at best and crybabies at worst. Don't reinforce this stereotype with anger and impatience.

The only constant in the world is change. Be open to it and accept it with a smile. Look at each change to your requirements, platform, or tool as a new challenge, not as some serious inconvenience to be fought.

The only true authority stems from knowledge, not from position. Knowledge engenders authority, and authority engenders respect – so if you want respect in an egoless environment, cultivate knowledge.

Fight for what you believe, but gracefully accept defeat. Understand that sometimes your ideas will be overruled. Even if you do turn out to be right, don't take revenge or say, "I told you so" more than a few times at most, and don't make your dearly departed idea a martyr or rallying cry.

Don't be "the guy in the room." Don't be the guy coding in the dark office emerging only to buy cola. The guy in the room is out of touch, out of sight, and out of control and has no place in an open, collaborative environment.

Critique code instead of people – be kind to the coder, not to the code. As much as possible, make all of your comments positive and oriented to improving the code. Relate comments to local standards, program specs, increased performance, etc.

nein das würde man so machen:

std::array<int, 4 * 2> Schiffe;

und weil es mühsam ist, jedes mal wenn man darauf zugreifen möchte auch noch die indices zu berechnen, kann man das auch kapseln:

template<class T, ::std::size_t Width_, ::std::size_t Height_>
class Field
{
public:
	typedef T ValueType;
	enum
	{
		Width = Width_,
		Height = Heigth_
	};
private:
	::std::array<ValueType, Width * Height> Underlying;
	::std::size_t TransformIndex(::std::size_t IndexX, ::std::size_t IndexY) const
	{
		return IndexX + IndexY * this->Width;
	}
public:
	ValueType const& operator() (::std::size_t IndexX, ::std::size_t IndexY) const
	{
		return this->Underlying[this->TransformIndex()];
	}
	ValueType& operator() (::std::size_t IndexX, ::std::size_t IndexY)
	{
		return this->Underlying[this->TransformIndex()];
	}
};

also das nein ging an jkhsjdhjs, nicht an dich seppj...

ups, die funktionsparameter bei den "TransformIndex"-aufrufen vergessen.

ValueType const& operator() (::std::size_t IndexX, ::std::size_t IndexY) const 
    { 
        return this->Underlying[this->TransformIndex(IndexX, IndexY)]; 
    } 
    ValueType& operator() (::std::size_t IndexX, ::std::size_t IndexY) 
    { 
        return this->Underlying[this->TransformIndex(IndexX, IndexY)]; 
    }

kann ein mod bitte meine drei posts zu einem zusammenheften?
jaja ich registrier mich irgendwann mal...

jkhsjdhjs

@asdfsf:

Okay...
Ich hab echt nicht die geringste Ahnung was du da geschrieben hast, hab zwar schonmal von Klassen gehört, weiß aber nicht was das ist. Könntest du vllt mal mit Kommentaren versehen, was du geschrieben hast?
Und was ist das Ziel von dem was du da geschrieben hast?

SeppJ

Ich denke, das was asdfsf vorschlägt, ist hier wohl etwas zu hoch für den vorhandenen Kenntnisstand. Ich sehe auch nicht, wozu das hier überhaupt nötig sein sollte, denn es hat keinen Vorteil gegenüber dem verschachtelten Array (siehe mein vorheriger Beitrag), es ist bloß viel umständlicher. Daher würde ich das verschachtelte Array nehmen.

jkhsjdhjs

Ok, dann werde ich die nehmen. Danke
Würde ich das hier:

#define SCHWARZ			0x0000
#define DUNKELBLAU		0x0001
#define DUNKELGRÜN		0x0002
#define DUNKELTÜRKIS	0x0003
#define DUNKELROT		0x0004
#define LILA			0x0005
#define GOLD			0x0006
#define GRAU			0x0007
#define DUNKELGRAU		0x0008
#define BLAU			0x0009
#define GRÜN			0x000a
#define TÜRKIS			0x000b
#define ROT				0x000c
#define PINK			0x000d
#define GELB			0x000e
#define WEIß			0x000f

dann so schreiben?:

enum Farben {
	SCHWARZ,
	DUNKELBLAU,
	DUNKELGRÜN,
	DUNKELTÜRKIS,
	DUNKELROT,
	LILA,
	GOLD,
	GRAU,
	DUNKELGRAU,
	BLAU,
	GRÜN,
	TÜRKIS,
	ROT,
	PINK,
	GELB,
	WEIß
}

SeppJ

Bezüglich Farbenenum: Ist jedenfalls ein Fortschritt gegenüber Makros. Allgemein würde ich Umlaute weglassen, da es auch viele Compiler gibt, die streng auf ASCII für Bezeichner bestehen. Und wenn man schon dabei ist, würde ich auf Englisch programmieren, sofern es keine große Mühe ist. Ist sicherlich gut, sich frühzeitig da dran zu gewöhnen, früher oder später wird man ohnehin auf englisch programmieren.

jkhsjdhjs

Ja, ich wollte eigentlich auf Englisch programmieren, aber da ich die Ausgaben auch auf Deutsch habe, dachte ich mir mach ich doch alles auf Deutsch. Aber ok, werde es mir merken!

EDIT: Und wie würde ich die Farben dann verwenden? Weil denen wurde doch jetzt kein Wert zugewiesen, oder?

Nathan

jkhsjdhjs schrieb:

EDIT: Und wie würde ich die Farben dann verwenden? Weil denen wurde doch jetzt kein Wert zugewiesen, oder?

Doch.
Der erste Wert eines Enums erhält die Nummer 0, alle anderen dann aufsteigend.
Du kannst ihnen aber auch manuell Werte zuweisen.

SeppJ

jkhsjdhjs schrieb:

EDIT: Und wie würde ich die Farben dann verwenden?

Wenn du es so wie hier definierst ( enum name { ... } ), dann gelten die Bezeichner im umgebenden Namensraum (also in der Regel überall, wenn du keine Namensräume benutzt). Das heißt, du kannst wirklich "SCHWARZ" im Programm schreiben und es ist wie eine Zahl mit Wert 0.

Ab C++11 gibt es neu die "strongly-typed" enums, die mittels enum class name { ... }; definiert werden. Diese unterliegen einer strengen Typprüfung (es ist also nicht mehr möglich, ein Wert aus einem enum dort zu benutzen, wo ein anderes enum erwartet wird) und die Elemente sind nun ebenfalls im jeweiligen Scope des enums. Bei enum Farben {SCHWARZ, ...}; also zum Beispiel Farben::SCHWARZ .

Was davon besser geeignet ist, kommt auf den Einzelfall an. Die alte Methode provoziert Namenskonflikte. Manchmal sollen die Bezeichner aber auch wirklich überall gelten. Farbwerte sind ein bisschen ein Grenzfall. Man kann leicht Beispiele konstruieren, wo die alte Methode unpassend ist, aber hier würde ich sagen, passt keiner dieser Fälle und ich würde es so machen, wie du es hier vorschlägst.

Weil denen wurde doch jetzt kein Wert zugewiesen, oder?

Wenn man nichts anderes angibt, dann gehen die Werte bei 0 los und sind dann jeweils um 1 aufsteigend. Hier passt also alles schon, sofern du keine Farben vergessen hast. Besserer Stil wäre es daher wohl (um solche Fehler zu vermeiden) allen Farben einen Wert zu zu weisen:

enum foo
{
  hundert = 100, 
  tausend = 1000, 
  million = 1000000
};

P.S.: In deinem Codebeispiel fehlen übrigens die Kommas zwischen den Werten, aber das hast du sicherlich schon bemerkt.

jkhsjdhjs

@Nathan:
Aber ich brauche ja den Wert 0x0001 und nicht 1 oder ist das egal?

@SeppJ:
Danke, muss jetzt erstmal deinen Post komplett lesen, und nein, hatte es noch nicht gemerkt, hatte die komplett vergessen, ich editiers grad

SeppJ

Wie viel ist denn 0x0001, wenn du es als Dezimalzahl schreibst?

jkhsjdhjs

Ok, ja das wird mir jetzt auch klar ^^

Dann könnte ich doch auch schon vorher als ich noch #define verwendet habe, nur 1 schreiben. Habe das mit den Farben von irgendner Seite, die auf Google war. Da stand das mit dem 0x0001

EDIT: Und seit wann gibt es C++ 11? Ich nutze Visual Studio 2008

EDIT2: Und kommen bei enumeratoren dann nach 9 auch a,b,c usw?

EDIT3: Und wie würde ein typedef für ein 2 Dimensionales Array aussehen?
Habe das eben gegoogled, kam aber auf Schwachsinn, der nicht funktioniert hat
:

typedef array<array<int, d_array;

So leicht geht das dann doch nicht, oder? ^^
Ich habe auch von typedef mit Parameter gelesen, ich muss warscheinlich solche nehmen, oder?

Und wie teile ich Programmcode auf mehrere Dateien auf?
Das wurde mir ja auch am Anfang geraten.

Könnte mir bitte jemand all diese Fragen beantworten? Danke