C++ Anfägertipps für Codeoptimierung?

[Jinuga1]

Hallo liebe Community!

Ich bin, was Programmieren angeht, ein absoluter Anfänger, habe mich die letzten Tage etwas mit der C++ Programmierung auseinandergesetzt und mit dem gewonnenen Wissen ein Programm geschrieben. Jedoch wäre mein Ziel, in Zukunft auch darauf zu achten möglichst einfach und optimiert zu programmieren. Daher bitte ich euch den Code zu lesen und eventuell auch Verbesserungsvorschläge hier zu lassen.

#include <iostream>
using namespace std;

int main() {

	float RADIUS;
	float PI = 3.14159f;
	int eingabe;
	
	cout << "1 - Kreisflaeche" << endl;
	cout << "2 - Kreisdurchmesser" << endl;
	cout << "3 - Kreisumfang" << endl;
	
	cin >> eingabe;
	
	switch(eingabe)
	{
		case 1:
			cout << "Kreisfläche" << endl;
			
			
			cout << "Diese Programm berechnet die Fläche eines Kreises." << endl;
			
			cout << "Radius: ";
	
	cin >> RADIUS;


	if(RADIUS <= 0)
		cout << "Error!" << endl;
	
	else
	cout << "Die Kreisflaeche bertaegt: " << RADIUS * RADIUS * PI << endl;
	break;
		case 2:
			cout << "Kreisdurchmesser" << endl;
			
			cout << "Dieses Programm berechnet den Durchmesser eines Kreises." << endl;
			cout << "Radius:";
			
			cin >> RADIUS;
		
		
			if(RADIUS <= 0)
			cout << "Error!" << endl;
		
			else
			cout << "Der Durchmesser betraegt: " << RADIUS * 2 << endl;
		break;
		case 3:
			cout << "Kreisumfang: " << endl;
			
			cout << "Dieses Programm berechnet den Umfang eines Kreises." << endl;
			cout << "Radius: ";
			
			cin >> RADIUS;
			
			
			if(RADIUS <= 0)
			cout << "Error!" << endl;
		
			else
			cout << "Der Umfang betraegt: " << RADIUS * 2 * PI << endl;
			break;
	}
	
	
	
	system("Pause");
	return 0;
}

MfG

 

[Mr.Smith]

wie wär es mit kommentieren^^
in dem fall isses klar was das programm macht, aber ansonsten weißt du recht schnell nicht mehr was wieso und warum gemacht hast

 

[-=:Cpt.Nemo:=-]

Hallo,

ich kann zwar kein c++ ,
Aber der Output in Zeile 33:

cout << "Die Kreisflaeche bertaegt: " << RADIUS * RADIUS * PI << endl;

müsste heissen

cout << "Die Kreisflaeche betraegt: " << RADIUS * RADIUS * PI << endl;

schönes Wochenende

 

[Spider11]

wie wär es mit kommentieren^^
in dem fall isses klar was das programm macht, aber ansonsten weißt du recht schnell nicht mehr was wieso und warum gemacht hast

Darüber kann man streiten, ich lese lieber Code ohne jegliche Kommentare. Dafür müssen die Namen (Klassen, Funktionen, Variablen, etc.) aussagekräftig sein. Siehe dazu auch Clean Code.
http://apdevblog.com/comments-in-code/

 

[schattenhueter]

Ich würde vorschlagen Funktionen zu nutzen. Das Switch ist in deinem Programm fast 50 Zeilen lang und dadurch etwas unübersichtlich. Für jeden Fall kannst du eine Funktion erstellen, die du aufrufst.
Um Strings zu erstellen, die Variablen enthalten ist es ratsam sprintf [1] zu verwenden. Bei denen Programm stehen dir Variablen immer am Ende der Strings, so dass spri tf keinen Vorteil bringt, häufig möchte man jedoch Variablen mitten in Strings einbauen. Außerdem kannst du mit sprintf zahlen formatieren ( Anzahl der Nachkommastellen).

[1] http://www.cplusplus.com/reference/cstdio/sprintf/

 

[deineMudda]

sprintf würde ich nicht nutzen, weil es c und nicht cpp ist. Wenn ich es richtig sehe fehlt in deinem switch statement bei der Auswertung der Variable eingabe ein default wert. Wenn jemand z. b. 5 oder text eingibt solltest du damit eine Fehlerbehandlung einbauen.

Außerdem solltest du meiner meinung das Programm mal objektorientiert gestalten. Pack mal die Variablen in eine Klasse Kreis und und füge der Klasse z. B. die Methoden Berechnungsart, Kreisfläche, Kreisdurchmesser und Kreisumfang hinzu.

Außerdem solltest du Varibalen wie PI als Konstante deklarieren, also

const float PI = 3,14;

edit: du kannst Radius und eingabe als unsigned int deklarieren, weil die Werte ja positiv sein müssen. Außerdem solltest du nur Konstanten in Großbuchstaben benennen. Aus RADIUS solltest du hier radius machen, PI ist OK weil es eine Konstante ist.

edit2: Initialisiere alle deine Variablen. Also statt

float RADIUS;

float radius=0

 

[K0ng3n]

Ich bin kein Experte, aber ich würde sagen bzgl. der Performance gibt es in dem kleinen Programm rein von der Programmiersprache nicht viel zu optimieren.

Aber ich halte den Vorschlag mit den Funktionen und dem Initialisieren der Variablen für sehr sinnvoll. Ansonsten würde ich Pi konstant machen:

const float PI = 3.14159f;

oder sogar:

constexpr float PI = 3.14159f;

Der compiler kann dann entscheiden, ob er tatsächlich eine Konstante anlegt, oder einfach jedes Auftreten von PI mit dem entsprechenden Wert ersetzt. Zusätzlich verhinderst du damit ein versehentliches Zuweisen.

Das constexpr weist den Compiler darauf hin, dass PI auf ein Literal zurückzuführen ist, sprich auf 3.14159, also ein Wert, der schon vor dem Compilieren bekannt ist. Wenn du dann irgendwo Berechnungen hast, in der sich alle Operanden auf Literale zurückführen lassen, kann die Berechnung zur compile-time stattfinden. Damit kannst du Rechenzeit sparen. Bringt in deinem Beispiel aber weniger, da du mit Eingaben arbeitest.

Sinnvoll ist meiner Meinung nach noch, In if- oder while-Köpfen, ein Literal links zu schreiben. Ist aber auch nicht wirklich c++ spezifisch.
Also statt:

if(RADIUS <= 0)

if(0.f >= RADIUS)

Damit verhinderst du versehentliche Zuweisungen, die vom Compiler nicht erkannt werden würden. Z.B. wenn du dich verschreibst:

if(RADIUS = 0)

 

[Jinuga1]

Vielen Dank für die Vorschläge und Korrekturen!


Wie wäre das mit dem objektorientiert denn umzusetzen, also wie packe ich den Code in "Klassen"?

edit: Und wenn ich in dieser switch-Funktion noch einen default-Wert ergänze, wie wäre es denn umzusetzen, wenn eine ungültige Eingabe vorliegt, dass das Programm quasi wieder nach oben springt und nocheinmal nach einer Eingabe verlangt?

 

[deineMudda]

Also du machst eine Klasse, die deine Variablen enthält. Außerdem richtest du für jede Variable eine Set Funktion ein, mit der von außerhalb der Klasse die Varibalen initialisiert werden können. Die Variablen selbst können von außen nicht verändert werden.

in etwa so: (nicht getestet)

class Kreis
{
private:

const float PI = 3,14;
float radius = 0;
unsigned int eingabe=0;

public:

void setRadius(float r)
{
radius = r;
}

void setEingabe(unsigned int e)
{
eingabe = e;
}

float kreisflaeche(float r)
{
return r*r*PI;
}

}

 

[nullPtr]

if(RADIUS <= 0)

if(0.f >= RADIUS)

Das macht den Code meiner Meinung nach sehr viel komplizierter zu lesen, wenn du das an jeder einzelnen Bedingung machst. Davon halte ich überhaupt nichts.

Damit verhinderst du versehentliche Zuweisungen, die vom Compiler nicht erkannt werden würden. Z.B. wenn du dich verschreibst:

if(RADIUS = 0)

Das ist Quatsch. Ein Compiler weiß selbstverständlich was eine Zuweisung ist und was nicht. Und jeder aktuelle Mainstream-Compiler weißt dich auf diesen potenziellen Fehler hin.

Was hier teilweise vorgeschlagen wird ist haarsträubend. Die Programmauswahl "Eingabe e" in der Kreisklasse mitzuführen ist so ziemlich das schlimmste, was man tun kann. Wenn wir hier mit Klassen arbeiten wollen, dann sind Setter jedoch unnötiger Boilerplate-Code. Ein Kreis ist in diesem Programm immutable; der Radius sollte nur im Konstruktor festgelegt werden. Dann kann man das auch in einem sinnvollen Ausdruck verpacken:

std::cout << Circle(radius).diameter();

Wenn wir dann Glück haben, optimiert der Compiler für uns das temporäre Objekt sogar raus.

Die aller erste *zwingende* Optimierung, die mir hier auffällt: Jedes deiner Alternativen benötigt einen Radius und eine Fehlerbehandlung, falls der der Nutzer Unfug eingegeben hat. Diese Codeduplikation muss weg.

 

[K0ng3n]

Das macht den Code meiner Meinung nach sehr viel komplizierter zu lesen, wenn du das an jeder einzelnen Bedingung machst. Davon halte ich überhaupt nichts.

Wieso ist das komplizierter? Ist doch lediglich eine andere Reihenfolge. Nur wenn du auf ungleich 0 prüfen würdest, könntest du es kürzer schreiben.

Das ist Quatsch. Ein Compiler weiß selbstverständlich was eine Zuweisung ist und was nicht. Und jeder aktuelle Mainstream-Compiler weißt dich auf diesen potenziellen Fehler hin.

Dennoch ist es ein Unterschied einen Error zu bekommen, oder eine Warnung, die man ignorieren kann. Ist ja nur meine Meinung, aber für mich macht es durchaus Sinn.

 

[deineMudda]

die set Methoden sollten es meiner Meinung nach ermöglichen, den Radius zu ändern, der Radius ist doch beim nächsten Aufruf veränderlich.

 

[VikingGe]

Wieso ist das komplizierter? Ist doch lediglich eine andere Reihenfolge.

Naja, weil man das, was man das Objekt, das man untersuchen möchte, intuitiv in der Regel als erstes hinschreibt. Ich will ja nicht wissen, ob es die Eigenschaft der Zahl 0 ist, größer als irgendwas anderes zu sein, sondern ob dieses etwas kleiner ist als 0. Ist ein bisschen so wie die Sätze "Der Mann aß den Apfel" vs "Den Apfel aß der Mann". Mathematisch identisch, aber für mich persönlich schwer zu lesen.

die set Methoden sollten es meiner Meinung nach ermöglichen, den Radius zu ändern, der Radius ist doch beim nächsten Aufruf veränderlich.

Es ist aber unnötig. Wenn ich eine Kreis-Klasse schreiben müsste, sähe die in etwa so aus:

class Circle {

public:
  
  Circle() { }
  explicit Circle(float radius)
  : _radius(radius) { }
  
  float radius() const {
    return this->_radius;
  }
  
  float diameter() const {
    return 2.0f * this->_radius;
  }
  
  float surfaceArea() const {
    return pi * this->_radius * this->_radius;
  }
  
private:
  
  float _radius = 0.0f;

};

Ich habe also einen Kreis. Den kann ich erstellen und bin glücklich. Der Kreis hat immer denselben Radius.
Was mache ich also, wenn ich einen Kreis mit einem anderen Radius haben möchte? Ganz einfach, ich erstelle einen neuen Kreis:

while (running) {
  float radius = 0.0f;
  cin >> radius;
  const Circle circle = Circle(radius);
  // Mach was mit dem Kreis
}

Klar, in dem Fall könnte man dasselbe auch mit nem Setter erreichen. Es hat aber nur Nachteile:
- Du musst ihn manuell implementieren. Die direkte Zuweisung erledigt dasselbe ganz ohne Code.
- Du musst darauf achten, dass das Objekt jederzeit in einem gültigen Zustand ist. Das ist bei einem Kreis mit einer Variablen trivial, bei komplexeren Objekten wird das schwieriger.
Das betrifft nebenbei nicht nur reine Setter, sondern alle Methoden, die das Objekt verändern. Je weniger davon existieren, desto weniger Bugs hat am Ende das Programm.
- Die Klasse ist nicht mehr automatisch threadsicher, wenn mehrere Threads auf dieselbe Instanz zugreifen. Beim Kreis ist das wie immer egal, das ist vor allem bei Objekten interessant, die auf dem Heap erzeugt werden.
- Da bei deiner Klasse auch noch die direkte Initialisierung fehlt, ist sie aufwändiger zu benutzen (2-3 Zeilen statt einer).

 

[K0ng3n]

Naja, weil man das, was man das Objekt, das man untersuchen möchte, intuitiv in der Regel als erstes hinschreibt. Ich will ja nicht wissen, ob es die Eigenschaft der Zahl 0 ist, größer als irgendwas anderes zu sein, sondern ob dieses etwas kleiner ist als 0. Ist ein bisschen so wie die Sätze "Der Mann aß den Apfel" vs "Den Apfel aß der Mann". Mathematisch identisch, aber für mich persönlich schwer zu lesen.

Ok, vielen Dank für die Erläuterung!
Hab das mit dieser Art der Vergleiche irgendwann mal irgendwo als Tipp gelesen. Schien und scheint mir persönlich sinnvoll. Von daher werde ich das bei mir so beibehalten. Bisher hatte ich damit überhaupt keine Verständnisprobleme. Als "viel komplizierter" würde ich es jedenfalls nicht bezeichnen. Aber kann den Punkt zumindest jetzt nachvollziehen. Danke :-)

 

[NemesisFS]

Eine Anmerkung ohne konkret auf deinen Code einzugehen:
Versuche simple, und lesbaren Code zu schreiben. Es sollte dir (und anderen) möglich sein, auch in zwei Jahren um ein Uhr Nachts schnell zu verstehen was passiert. Optimierung auf Codeeffizienz ist erstmal Aufgabe des Compilers! Erst wenn du tatsächlich Probleme hast, solltest du anfangen zu schauen, wo der Code effizienter sein könnte. Stichwort: premature optimization

 

[Miuwa]

Dennoch ist es ein Unterschied einen Error zu bekommen, oder eine Warnung, die man ignorieren kann. Ist ja nur meine Meinung, aber für mich macht es durchaus Sinn.

Das sollte dank -Werror ("treat warnings as errors!") eigentlich keinen Unterschied machen. Ich bin auch der Meinung, dass die Variable eher nach links gehört, weil das dem natürlichen Lesefluss entspricht, aber das kann jeder machen wie er will.

Ich würde vorschlagen Funktionen zu nutzen. Das Switch ist in deinem Programm fast 50 Zeilen lang und dadurch etwas unübersichtlich.

Die aller erste *zwingende* Optimierung, die mir hier auffällt: Jedes deiner Alternativen benötigt einen Radius und eine Fehlerbehandlung, falls der der Nutzer Unfug eingegeben hat. Diese Codeduplikation muss weg.

Das ist der eine entscheidende Hinweis in dieser ganzen Diskussion. Das würde die switch Struktur dann auch auf ein paar übersichtliche Zeilen Code zusammenstreichen. Ob du deinen Code dann noch weiter in Funktionen oder Klassen unterteilen willst ist dir überlassen (hier fände ich es übertrieben).

Generell würde ich übrigens ein paar weniger Leerzeilen empfeheln und sofern du keinen besonderen Grund dafür hast würde ich auf dem Desktop immer mit double statt float rechenen.

Ergänzung (11. April 2016)

Und falls du wirklich die Performance optimieren möchtests (auch wenn das hier völlig sinnlos wäre), dann solltest du alle std::endl, die nicht vor einem std::cin stehen durch "\n" ersetzen.

std::cout << std::endl

Ist (nach dem unvermeidlichen Einlesen von Nutzereingaben) die mit Abstand langsamte Operation in deinem ganzen Code.