C++ Klasse - stil und fehler

.filmor

blan schrieb:

1. kann mit man c++ streams auch pipes öffnen und wann ja wie?

Wenn du die Kommandozeilen-Pipes meinst, die leiten in den stdin (== std::cin) um. Wenn du Pipes im Dateisystem meinst, das sind für dein Programm einfach Dateien, geht also mit fstream.

blan schrieb:

2. wie schreibt man sowas in c++... ich hab mit www.cppreference.com und ähnliches angeschaut aber der stream lief so nie

Mit Filestreams gehst du genauso um wie mit std::cout bzw. std::cin. Du musst nur bei der Definition (oder später mit der open-Methode) den Dateisystempfad angeben.

blan

also ich meinte eine alternative in c++ zu der c-funktion

FILE *popen(...)

damit kann man ein programm starten und den output einlesen.

mfg blan

tntnet

Mich wundert, daß hier noch niemand auf den gravierendsten Fehler aufmerksam gemacht hat: den fehlenden Kopierkonstruktor und Zuweisungsoperator. Definierst Du keinen, macht der Compiler einen. Und der ist in Deinem Fall falsch, da der Kopierkonstruktor eine 2. Instanz anlegt ohne shout_new() aufzurufen. Bei löschen der beiden Instanzen wird 2 mal shout_free() aufgerufen. Das gleiche gilt für den Zuweisungsoperator.

Am einfachsten lässt sich das lösen, indem Du beide private deklarierst und nicht definierst. Dann ist die Klasse nicht kopierbar.

Ansonsten wie hier bereits angedeutet wurde: Alle getter sollten const deklariert werden.

Standard sollte sein, daß Methoden nicht virtuell sind. Mache sie nur virtuell, wenn es notwendig ist. Erst mal alles virtuell (ausser ausgerechnet dem Destruktor) ist nicht so sinnvoll.

Man sollte nicht vermeiden, Referenzen auf lokale Variablen zurück zu liefern, sondern das ist VERBOTEN. Eine "const std::string&" liefert man dann zurück, wenn in der Klasse ein std::string deklariert ist. Muß man im Getter erst eine Instanz erzeugen, dann einfach "std::string" liefern.

"shout_open()" und "shout_close()" sind mir auch Kandidaten für den Konstruktor und Destruktor. Oder macht es Sinn, mehrmals shout_open() aufzurufen? Dann wäre das doch eine eigene Klasse.

Was ist mit Fehlerbehandlung? Ich empfehle die Verwendung von Exceptions. Definiere eine Exception ShoutException abgeleitet von std::runtime_error und werfe diese bei Fehlern (aber nicht im Destuktor!).

Übrigens für pipes empfehle ich red::pstreams (http://sourceforge.net/projects/pstreams/)

Tntnet

blan

ich versteh nicht was du mit dem kopierkonstruktor meinst

zu dem "open" und "close" in konstruktor / dekonstruktor... erstmal muss man alle einstellungen tätigen... macht wenig sinn wenn er schon beim erstellen connectet oder?

mfg blan

.filmor

Wenn du keinen Kopierkonstruktor und keinen Zuweisungsoperator angibst, dann werden beide Methoden vom Compiler generiert. Die generierten Methoden tuen das, was sie für C structs tuen würden, nämlich einfach elementweise Kopie/Zuweisung. Das hebelt aber deinen Referenzzähler aus. Am besten deklarierst du beides im private-Bereich, eine Definition gibst du nicht an:

class Shout
{
public:
// ...
private:
// ...
    Shout (const Shout&);
    Shout& operator= (const Shout&);
};

blan

arrrr... sone klasse is ja komplizierter als ich dachte o.O

also ich hab mein kopierkonstruktor nun so implementiert (public):

Shout::Shout(Shout &shout)
{
  if(m_count == 0)
    shout_init();

  m_count++;

  m_shout = shout_new();

  set_host(shout.get_host());
  set_port(shout.get_port());

  set_user(shout.get_user());
  set_password(shout.get_password());

  set_mount(shout.get_mount());

  set_protocol(shout.get_protocol());
  set_format(shout.get_format());

  set_name(shout.get_name());
  set_url(shout.get_url());
  set_genre(shout.get_genre());

  set_public(shout.get_public());
  set_agent(shout.get_agent());
  set_description(shout.get_description());
  set_dumpfile(shout.get_dumpfile());

  set_audio_info(SHOUT_AI_BITRATE, shout.get_audio_info(SHOUT_AI_BITRATE));
  set_audio_info(SHOUT_AI_SAMPLERATE, shout.get_audio_info(SHOUT_AI_SAMPLERATE));
  set_audio_info(SHOUT_AI_CHANNELS, shout.get_audio_info(SHOUT_AI_CHANNELS));
  set_audio_info(SHOUT_AI_QUALITY, shout.get_audio_info(SHOUT_AI_QUALITY));

  set_nonblocking(shout.get_nonblocking());
}

mfg blan

.filmor

Hat denn das Kopieren des Objektes irgendeine sinnvolle Anwendung? Wenn nicht, dann spar dir die Arbeit und schalte es ab.

blan

muss ich mal schaun, ich denk schon - vorläufig hab ich es nun so gemacht wie du gesagt hast

private:
    Shout(const Shout &shout);
    Shout &operator= (const Shout &shout);

    shout_t *m_shout;
    static unsigned short m_count;

wieso bringt der compiler beim kompilieren kein fehler wenn ich die methoden nicht implementier - sollte ich ein es trotzdem tun und einfach nichts reinschreiben?

mfg blan

.filmor

Solltest du sie von außen aufrufen sagt dir der Compiler, dass sie private sind. Solltest du sie innerhalb der Klasse aufrufen (was du ja auch mit privaten Methoden darfst) wird er meckern, sie seien nicht definiert. Somit hast du sämtliche Verwendungsmöglichkeiten ausgeschlossen.

/Edit: Das &-Zeichen sollte IMHO beim Typ stehen. Bei Zeigern ist das noch grenzwertig (wegen des C-Erbes), aber bei Referenzen muss klargestellt werden, dass der Typ verändert wird.
Da du die Methoden eh nicht benutzen kannst, musst du den Parametern keine Namen geben.

blan

okay danke.. ich dachte der macht dann irgendwie wieder sein eigenes ding und implementiert selber irgendwas, wie wenn ich den kopierkonstruktor garnicht setzte.

mfg blan

kartoffelsack

man sollte sehr wohl const std::string zurückgeben statt eines simplen string. In C++ sollte man Klassen einsetzen, dass sie sich wie ein int verhalten.

Während

int foo(){...}

foo() += 3;

nicht funzt, würde

string bar(){...}

bar() += "bla";

gehen, aber keinen Sinn machen.

Anders

const string foobar(){...}

foobar() += bla;

denselben Fehler wie der int Pendant