C++ Klasse - stil und fehler

[blan]

jo hab ich bemerkt, mein compiler hat eine warnung ausgegeben.

also seh ich das richtigt, die referenz bei einer methode bringt nur was um den wert einer variable die dauerhaft exisiert zurückzugeben?

mfg blan

[akari]

Hallo

also seh ich das richtigt, die referenz bei einer methode bringt nur was um den wert einer variable die dauerhaft exisiert zurückzugeben?

Ja du solltest unbedingt vermeiden, temporäre Werte per Referenz oder Pointer zu übergeben.

bis bald
akari

[blan]

also ich bin wieder ein stück weiter und atm sieht die klasse wie folgt aus (das streamen funktioniert schon)

#ifndef _SHOUT_H_
#define _SHOUT_H_

#include <iostream>
#include <string>
#include <shout/shout.h>

class Shout
{
  private:
    shout_t *m_shout;
    static unsigned short m_count;

  protected:

  public:
     Shout();
    ~Shout();

    static void get_version(int &major,
                             int &minor,
                             int &patch);

    static std::string get_version();

    std::string get_error() const;
    int get_errno() const;

    bool get_connected() const;

    int open();
    int close();

    int set_host(const std::string &host);
    std::string get_host() const;

    int set_port(const unsigned short &port);
    unsigned short get_port() const;

    int set_user(const std::string &user);
    std::string get_user() const;

    int set_password(const std::string &password);
    std::string get_password() const;

    int set_mount(const std::string &mount);
    std::string get_mount() const;

    int set_protocol(const unsigned int &portocol);
    unsigned int get_protocol() const;

	int set_format(const unsigned int &format);
	unsigned int get_format() const;

    int set_name(const std::string &name);
    std::string get_name() const;

    int set_url(const std::string &url);
    std::string get_url() const;

    int set_genre(const std::string &genre);
    std::string get_genre() const;

	int set_public(const unsigned int &make_public);
	unsigned int get_public() const;

    int set_agent(const std::string &agent);
    std::string get_agent() const;

    int set_description(const std::string &description);
    std::string get_description() const;

    int set_dumpfile(const std::string &dumpfile);
    std::string get_dumpfile() const;

    int set_audio_info(const std::string &name,
                       const std::string &value);

    std::string get_audio_info(const std::string &name);

	int set_nonblocking(const unsigned int &nonblocking);
	unsigned int get_nonblocking() const;

	int send(const unsigned char *data,
             const size_t &len);

	ssize_t send_raw(const unsigned char *data,
                     const size_t &len);

	void sync();
};

#endif /* _SHOUT_H_ */

#include "shout.h"

unsigned short Shout::m_count = 0;

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

  m_count++;

  m_shout = shout_new();
}

Shout::~Shout()
{
  m_count--;

  if(m_count == 0)
    shout_shutdown();

  shout_free(m_shout);
}

void Shout::get_version(int &major,
                        int &minor,
                        int &patch)
{
  shout_version(&major, &minor, &patch);
}

std::string Shout::get_version()
{
  return shout_version(0, 0, 0);
}

std::string Shout::get_error() const
{
  return shout_get_error(m_shout);
}

int Shout::get_errno() const 
{
  return shout_get_errno(m_shout);
}

bool Shout::get_connected() const
{
  if(shout_get_connected(m_shout) == SHOUTERR_CONNECTED)
    return true;
  else
    return false;
}

int Shout::open()
{
  return shout_open(m_shout);
}

int Shout::close()
{
  return shout_close(m_shout);
}

int Shout::set_host(const std::string &host)
{
  return shout_set_host(m_shout, host.c_str());
}

std::string Shout::get_host() const
{
  return shout_get_host(m_shout);
}

int Shout::set_port(const unsigned short &port)
{
  return shout_set_port(m_shout, port);
}

unsigned short Shout::get_port() const
{
  return shout_get_port(m_shout);
}

int Shout::set_user(const std::string &user)
{
  return shout_set_user(m_shout, user.c_str());
}

std::string Shout::get_user() const
{
  return shout_get_user(m_shout);
}

int Shout::set_password(const std::string &password)
{
  return shout_set_password(m_shout, password.c_str());
}

std::string Shout::get_password() const
{
  return shout_get_password(m_shout);
}

int Shout::set_mount(const std::string &mount)
{
  return shout_set_mount(m_shout, mount.c_str());
}

std::string Shout::get_mount() const
{
  return shout_get_mount(m_shout);
}

int Shout::set_protocol(const unsigned int &protocol)
{
  return shout_set_protocol(m_shout, protocol);
}

unsigned int Shout::get_protocol() const
{
  return shout_get_protocol(m_shout);
}

int Shout::set_name(const std::string &name)
{
  return shout_set_name(m_shout, name.c_str());
}

int Shout::set_format(const unsigned int &format)
{
  return shout_set_format(m_shout, format);
}

unsigned int Shout::get_format() const
{
  return shout_get_format(m_shout);
}

std::string Shout::get_name() const
{
  return shout_get_name(m_shout);
}

int Shout::set_url(const std::string &url)
{
  return shout_set_url(m_shout, url.c_str());
}

std::string Shout::get_url() const
{
  return shout_get_url(m_shout);
}

int Shout::set_genre(const std::string &genre)
{
  return shout_set_genre(m_shout, genre.c_str());
}

std::string Shout::get_genre() const
{
  return shout_get_genre(m_shout);
}

int Shout::set_public(const unsigned int &make_public)
{
  return shout_set_public(m_shout, make_public);
}

unsigned int Shout::get_public() const
{
  return shout_get_public(m_shout);
}

int Shout::set_agent(const std::string &agent)
{
  return shout_set_agent(m_shout, agent.c_str());
}

std::string Shout::get_agent() const
{
  return shout_get_agent(m_shout);
}

int Shout::set_description(const std::string &description)
{
  return shout_set_description(m_shout, description.c_str());
}

std::string Shout::get_description() const
{
  return shout_get_description(m_shout);
}

int Shout::set_dumpfile(const std::string &dumpfile)
{
  return shout_set_dumpfile(m_shout, dumpfile.c_str());
}

std::string Shout::get_dumpfile() const
{
  return shout_get_dumpfile(m_shout);
}

int Shout::set_audio_info(const std::string &name,
                          const std::string &value)
{
  return shout_set_audio_info(m_shout, name.c_str(), value.c_str());
}

std::string Shout::get_audio_info(const std::string &name)
{
  return shout_get_audio_info(m_shout, name.c_str());
}

int Shout::set_nonblocking(const unsigned int &nonblocking)
{
  return shout_set_nonblocking(m_shout, nonblocking);
}

unsigned int Shout::get_nonblocking() const
{
  return shout_get_nonblocking(m_shout);
}

int Shout::send(const unsigned char *data,
                const size_t &len)
{
  return shout_send(m_shout, data, len);
}

ssize_t Shout::send_raw(const unsigned char *data,
                        const size_t &len)
{
  return shout_send_raw(m_shout, data, len);
}

void Shout::sync()
{
  shout_sync(m_shout);
}

(falles es jemand intressiert :D)

kleine frage hätt ich da noch:

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

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

unsigned char buffer[4096];
	  size_t read;
	  FILE *hFile;

		hFile = fopen("05-schritt_fuer_schritt_feat._gnom-blz.ogg", "r");

		  while(1)
		  {
			read = fread(buffer, 1, sizeof(buffer), hFile);

			if(read > 0)
			{
			  if(shout.send(buffer, read) != SHOUTERR_SUCCESS)
			  {
//				printf("[DEBUG] Send error: %s (%d)", shout.get_error(), shout.get_errno());
				break;
			  }
			}
			else
			{
			  break;
			}

			shout.sync();
		  }

		fclose(hFile);

mfg blan

[.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