C++ Klasse - stil und fehler

blan

okay, aus sicherheitsgründen sollte ich wohl wirklich init und shutdown nicht in construktor / deconstructor bauen oder?

hab mir ein paar andere Quellcodes angeschaut und da ham die ihre Klasse so genannt, aber Shout ist kein problem.

das mit der version ist auch eine gute idee..

was stimmt mit den "const" net?

danke!

mfg blan

THX 1138

blan schrieb:

okay, aus sicherheitsgründen sollte ich wohl wirklich init und shutdown nicht in construktor / deconstructor bauen oder?

Kannst Du schon. Wenn Du mehrere Objekte hast, hilft Dir vielleicht ein Reference Counter.

LordJaxom

virtual const string get_user(void);

Wozu ne konstante Kopie zurückgeben? Ändern interessiert Deine Klasse eh nicht. Und das Shout-Objekt wird auch nicht geändert, im Getter.

virtual const string& get_user() const;

Oder, wenn Du eine Kopie zurückgeben magst (warum auch immer), dann die wenigstens nicht konstant

virtual string get_user() const;

EDIT: (void) auch noch entschlackt

blan

okay, ich hab jetzt mal alles so umgesetzt wie ichs verstanden habe, wenn ihr noch was findet sagt bescheid.

#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();

    virtual int open();
    virtual int close();

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

    static std::string get_version();

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

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

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

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

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

#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);
}

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

int Shout::close()
{
  return shout_close(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);
}

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

std::string Shout::get_host()
{
  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()
{
  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()
{
  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()
{
  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()
{
  return shout_get_mount(m_shout);
}

mfg blan

.filmor

blan schrieb:

sollte ich eigentlich als extension von headern *.hpp benutzten?

IMHO ja. Das stellt nochmal eindeutig klar, dass dies ein C++- und kein C-Header ist (der "echte" libshout-Header scheint ja auch shout.h zu heißen). Die Alternativen, die ich kenne sind nicht so doll (.H macht unter Windows Probleme, ein doppeltes H ist in Deutschland negativ besetzt).

Namen die mit Unterstrich anfangen gehören auch dem Compiler. Mach einfach SHOUT_HPP.

Ich würde bezüglich der Zugriffsbereiche die Ordnung public, private vorziehen (protected benutzt du eh nicht!), da der Anwender nur das Interface braucht und es deshalb direkt sehen sollte. Außerdem würde ich an deiner Stelle die Methoden noch passend kommentieren.

Desweiteren sind alle getter als const zu kennzeichnen und sollten (für Nicht-Integrale Typen) eine const Referenz zurückliefern.

Einzeiler sollten entweder als inline gekennzeichnet werden oder direkt im Header stehen.

Wenn dein Wrapper mehr als diese eine Klasse enthält gehört er in einen eigenen Namensraum.

Du hast immernoch keine Antwort darauf geliefert, warum alle Methoden virtuell ist und (falls dies nötig sein sollte) hast noch immer keinen virtuellen Destruktor.

Frag nach, wenn du etwas nicht verstehst, aber übergeh bitte nicht einfach Hinweise.

blan

okay, aber da die klasse noch nicht fertig ist weisst du garnicht ob ich protected benutzen werde oder nicht ich habe die einfach alle mal virtuell gemacht - werd ich bei bedarf noch abändern. wenn ihr mir des mit dem const nochmal erklären könntet wär ich sehr dankbar bzw. nen tutorial wo des beschrieben ist. was macht zB ein const am anfang anders als am ende?

mfg blan

.filmor

Das const am Anfang gehört zum Rückgabetyp (der dann statt std::string z.B. const std::string& wird), während das const am Ende der Deklaration dafür sorgt, dass diese Methode erstens nur lesend auf das Objekt, über das sie aufgerufen wurde, zugreifen und zweitens kann sie deshalb auch auf const-Objekten aufgerufen werden. getter brauchen grundsätzlich keinen Schreibzugriff, deshalb sind diese Methoden immer const.

Übrigens macht man sich eigentlich bevor man die Klasse hinschreibt über deren Struktur und Verwendung Gedanken

akari

blan schrieb:

wenn ihr mir des mit dem const nochmal erklären könntet wär ich sehr dankbar bzw. nen tutorial wo des beschrieben ist.

Hier nochmal eine komplette Zusammenfassung der Const-Correctness

bis bald
akari

blan

okay, das mit dem const am ende versteh ich jetzt - die method kann dann keine klassen-variable verändern. aber was macht ein const am anfang einer methode, zB

const string getValue();

wzt soll denn das const bewirken - hab die situation in keinem tutorial gefunden?

mfg blan

THX 1138

blan schrieb:

okay, das mit dem const am ende versteh ich jetzt - die method kann dann keine klassen-variable verändern. aber was macht ein const am anfang einer methode, zB
const string getValue();
wzt soll denn das const bewirken - hab die situation in keinem tutorial gefunden?

mfg blan

Wie .filmor sagte, das gehört zum Rückgabewert.

LordJaxom

blan schrieb:

const string getValue();
wzt soll denn das const bewirken - hab die situation in keinem tutorial gefunden?

Wundert mich nicht - weil's wenig Sinn ergibt. Anders sieht das bei einer konstanten Referenz aus:

const string& getValue();

Vorteil Referenz: Es wird direkt auf den String in Deinem Objekt zugegriffen, ohne die Notwendigkeit eine Kopie zu erstellen (was oben passieren würde).
Vorteil const: Niemand kann den String aus Deinem Objekt ändern. Ohne const wäre im zweiten Fall nämlich folgendes möglich:

objekt.getValue() = "hallo, welt";

blan

okay, so ist das logisch - aber wenn ich folgendes hab

const string &getValue()
{
  string y = "hello world";
  return y;
}

string a = getValue();

dann existiert y doch garnicht mehr und es kommt zu einem fehler oder?

mfg blan

LordJaxom

Wenn Du einen guten Compiler hast kommt es zu einer Warnung a la "returning reference to temporary" - was allerdings die Compilierung nicht verhindert. Das Laufen zu lassen ist undefiniert.

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