Frage nach Programmierstil

Blue-Tiger

Hi!
Es gibt mittlerweile den BCC 5.5 gratis auf der Borland-Seite

Inhaltlich:
da du Klassen verwendest, betrachte ich dein Programm als C++ Programm. Allerdings ist dein Programm sehr C-lastig, d.h. du machst viele Sachen, die in C ueblich sind, in C++ aber nur ungern gesehen sind bzw. zum schlechten Stil gehoeren.

#include <dos.h>
#include <conio.h>
#include <stdio.h>
#include <alloc.h>
#include <string.h>
#include <process.h>
#include <io.h>
#include <fcntl.h>

verwendest du ziemlich viel C-Zeugs. In C++ gehts leichter, wenn du z. B. statt normalen char-Arrays die Klasse string benutzt.

VICI::cpFullName = (char*) malloc(strlen(cpValue)*sizeof(char));

malloc in C++ verwenden ist vollkommen out! in C++ verwendet man new, new[] und delete, delete[]

ansonsten haben eh andere schon das meiste gesagt was ich nochmal hervorheben moecht:

* lieber const als #define
* Klassennamen und typedefs alle in GROSSBUCHSTABEN hat nur Microsoft in der WinAPI, sonst niemand. Ist zwar Geschmackssache, hat sich in C++ aber fast schon eingebuergert das NICHT so zu machen. Grossgeschrieben werden im Normalfall Makros und Konstanten (ist wie gesagt persoenlicher Geschmack, aber sehr, sehr weit verbreitet)
* gleiches gilt fuer Ungarische Notation. Die mag in C zwar gang & gebe sein, in C++ ist sie aber verpoehnt: Eine der Neuerungen von C++ ist ja gerade, dass man "neue" Datentypen definieren und untereinander austauschen kann. Was machst du, wenn du den Typ einer Variable aenderst? Den Variablennamen ueberall mitveraendern? Unpraktisch und Fehleranfaellig IMO

Was mir sonst noch im Code auffaellt:

class VICI {
  private:
    char *cpName;
    char *cpFullName;
    char *cpVersion;
  public:
    VICI();
    void TellName(void);
    void TellVersion(void);
    void TellDateTime(void);
    void ExplainName(void);
    void Tell(char *cpWhat);
};

// ... [snip] ...

  VICI::Tell(VICI_INI_LOADED);
  VICI::TellName();
  VICI::TellDateTime();

Das kann kein C++ Compiler akzeptieren, der was auf sich haelt. Die Funktionen sind nicht static, die brauchen eine Instanz um aufgerufen zu werden funktionieren. Das duerfte gar nicht compilieren

BTW solltest du Klassendefinitionen und -deklarationen in eigene Dateien auslagern

Danke für deinen Kommentar 'net'!

net schrieb:

- die '{' an's ende einer zeile zu schreiben macht's auch nicht gerade übersichtlicher. lieber in die nächste zeile damit (ist aber auch, wie vieles andere, geschmackssache)

Ich frag' mich inzwischen, ob ich mir nicht doch eine etwas mehr professionelle version a la:

/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * */
          /*   Function: [FunctionName]                              */
         /*                                                         */
        /*   * Preconditions:                                      */
       /*      char* pString   :0: Terminated String              */
      /*       char* pResult   empty Buffer                      */
     /*        int iBuffer     size of pBuffer                  */
    /*                                                         */
   /*        * Postcondition:                                 */
  /*           pString         unchanged                     */
 /*            pResult         encoded version of pString   */
/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * */
void foobar(char *pString, char* pResult, int iBuffer)
{
  (...

  ...)
}

angewöhnen sollte

net 0

Swordfish schrieb:

Danke für deinen Kommentar 'net'!

gern gescheh'n

Swordfish schrieb:

Ich frag' mich inzwischen, ob ich mir nicht doch eine etwas mehr professionelle version a la....

lässte nur noch die ungarische notation wech und dann isses perfekt

Blue-Tiger schrieb:

Hi!
Es gibt mittlerweile den BCC 5.5 gratis auf der Borland-Seite

An neueren IDEs / Compilern hapert's nicht

Blue-Tiger schrieb:

VICI::cpFullName = (char*) malloc(strlen(cpValue)*sizeof(char));

ging mir damals recht schnell von der Hand! Die Alternative wär in BC++ gewesen, sich eine allfällige cString selbst zu bauen, was mir in hinsicht Aufwand/Nutzen nicht gerechtfertigt schien.

Blue-Tiger schrieb:

malloc in C++ verwenden ist vollkommen out! in C++ verwendet man new, new[] und delete, delete[]

<= glaub mir, _mir_ wär's symphatisch gewesen - BC jedoch nicht

Blue-Tiger schrieb:

Was mir sonst noch im Code auffaellt:
class VICI {
  private:
    [...]
  public:
    VICI();
    void TellName(void);
    void TellVersion(void);
    void TellDateTime(void);
    void ExplainName(void);
    void Tell(char *cpWhat);
};
// ... [snip] ...

  VICI::Tell(VICI_INI_LOADED);
  VICI::TellName();
  VICI::TellDateTime();
Das kann kein C++ Compiler akzeptieren, der was auf sich haelt. Die Funktionen sind nicht static, die brauchen eine Instanz um aufgerufen zu werden funktionieren. Das duerfte gar nicht compilieren

Just once to make you even more confused

#include "stdafx.h"

enum INIVALUES{INVALID, NAME, FULLNAME, VERSION, PROMPT};
const char *DAYOFWEEK[] = {"Sunday", "Monday", "Tuesday", "Wensday", "Thuesday", "Friday", "Saturday"};
const int MAX_COMMAND_LENGTH = 80;
typedef enum{TRUE=1, FALSE=0} BOOL;
typedef int HANDLE;
typedef unsigned int UINT;
class VICI {
	private:
		char *cpName;
		char *cpFullName;
		char *cpVersion;
		char *cpPrompt;
		BOOL bExit;
	public:
		VICI();
		BOOL Exit(void);
		void Prompt(char *cpCommand);
		void ParseCommand(char *cpCommand);
		void TellName(void);
		void TellVersion(void);
		void ExplainName(void);
		void Tell(char *cpWhat);
};
BOOL VICI::Exit(void) {
	return bExit;
}
void VICI::ParseCommand(char *cpCommand) {
	if(cpCommand[0]=='\0') {
		return;
	}
	if(stricmp(cpCommand, "exit")==0) {
		bExit=TRUE;
		return;
	}
	if(strcmpi(cpCommand, "who are you")==0||
		stricmp(cpCod, "who are u")==0||
		stricmp(cpCommand, "who are you?")==0||
		stricmp(cpCommand, "who are u?")==0) {
		VICI::TellName();
		return;
	}
	if(strcmpi(cpCommand, "vici")==0||
		stricmp(cpCommand, "vici?")==0) {
		VICI::ExplainName();
    return;
	}
	cprintf("Unrecognized Command!\r\n");
}
void VICI::Prompt(char *cpCommand) {
	char buffer[MAX_COMMAND_LENGHT+3];
	buffer[0] = MAX_COMMAND_LENGHT + 1;
	cprintf("%s", VICI::cpPrompt);
	strcpy(cpCommand, cgets(buffer));
	cprintf("\r\n");
}

void VICI::TellVersion(void) {
	cprintf("This is version %s!\r\n", VICI::cpVersion);
}
void VICI::Tell(char *cpWhat) {
	cprintf("%s\r\n", cpWhat);
}
void VICI::TellName(void) {
	cprintf("My name is %s.\r\n", VICI::cpName);
}
void VICI::ExplainName(void) {
	cprintf("%s means %s.\r\n", VICI::cpName, VICI::cpFullName);
}
VICI::VICI() {
	HANDLE hVICIiniFile;
	bExit = FALSE;
	char *cpTemp = NULL;
	VICI::cpName = NULL;
	VICI::cpVersion = NULL;
	VICI::cpFullName = NULL;
	VICI::cpPrompt = NULL;
	VICI::bExit = FALSE;
	hVICIiniFile = open("VICI.INI", O_RDWR|O_TEXT);
	if(hVICIiniFile==-1) {
		VICI::Tell(VICI_INI_NOT_FOUND);
		VICI::Tell(VICI_TERMINATED);
		exit(0);
	}
	cpTemp = (char*)malloc((strlen(VICI_INI_LENGTH_INFO)+CountDigits(filelength(hVICIiniFile))-2)*sizeof(char));
	sprintf(cpTemp, VICI_INI_LENGTH_INFO, filelength(hVICIiniFile));
	VICI::Tell(cpTemp);
	free(cpTemp);
	cpTemp = NULL;
	char cBuffer;
	char *cpVariable = NULL;
	char *cpValue = NULL;
	int iCounter = 0;
	do {
		read(hVICIiniFile, &cBuffer, 1);
		if(cBuffer=='#') {
			do {
				read(hVICIiniFile, &cBuffer, 1);
			} while(!(cBuffer==10||eof(hVICIiniFile)));
		} else {
			lseek(hVICIiniFile, -1, SEEK_CUR);
			iCounter = 0;
			do {
				read(hVICIiniFile, &cBuffer, 1);
				iCounter++;
			} while(!(cBuffer=='='||eof(hVICIiniFile)));
			free(cpVariable);
			cpVariable = NULL;
			cpVariable = (char*)malloc(iCounter*sizeof(char));
			lseek(hVICIiniFile, -iCounter, SEEK_CUR);
			read(hVICIiniFile, cpVariable, iCounter);
			cpVariable[iCounter-1] = '\0';
			iCounter = 0;
			do {
				read(hVICIiniFile, &cBuffer, 1);
				iCounter++;
			}while(!(cBuffer==';'||eof(hVICIiniFile)));
			free(cpValue);
			cpValue = NULL;
			cpValue = (char*)malloc(iCounter*sizeof(char));
			lseek(hVICIiniFile, -iCounter, SEEK_CUR);
			read(hVICIiniFile, cpValue, iCounter-1);
			cpValue[iCounter-1]='\0';
			read(hVICIiniFile, &cBuffer, 1);
			read(hVICIiniFile, &cBuffer, 1);
			INIVALUES iv = INVALID;
			if(stricmp(cpVariable, "Prompt")==0) {
				iv = PROMPT;
			}
			if(stricmp(cpVariable, "Name")==0) {
				iv = NAME;
			}
			if(stricmp(cpVariable, "FullName")==0) {
				iv = FULLNAME;
			}
			if(stricmp(cpVariable, "Version")==0) {
				iv = VERSION;
			}
			switch(iv) {
				case PROMPT:
					VICI::cpPrompt = (char*) malloc(strlen(cpValue)*sizeof(char));
					strcpy(VICI::cpPrompt, cpValue);
					break;
				case VERSION:
					VICI::cpVersion = (char*) malloc(strlen(cpValue)*sizeof(char));
					strcpy(VICI::cpVersion, cpValue);
					break;
				case NAME:
					VICI::cpName = (char*) malloc(strlen(cpValue)*sizeof(char));
					strcpy(VICI::cpName, cpValue);
					break;
				case FULLNAME:
					VICI::cpFullName = (char*) malloc(strlen(cpValue)*sizeof(char));
					strcpy(VICI::cpFullName, cpValue);
					break;
				case INVALID:
					cprintf("Unrecognized variable: %s!\r\n", cpVariable);
					break;
			}
		}
	} while(!eof(hVICIiniFile));
	close(hVICIiniFile);
	VICI::Tell(VICI_INI_LOADED);
	VICI::TellName();
//	VICI::TellDateTime();
}
void main(void) {
	VICI v;
	char cCommand[80];
	do {
		v.Prompt(cCommand);
		v.ParseCommand(cCommand);
	}while(!v.Exit());
}

<= compiliert sogar unter VC7!
(etwas abgeändert da eindeutig(VC!=BC)).

Blue-Tiger schrieb:

BTW solltest du Klassendefinitionen und -deklarationen in eigene Dateien auslagern

Wäre das Progrämmchen über die 'quick-and-dirty'-Phase hinausgekommen, hätt ich das gemacht

Blue-Tiger schrieb:

Hi!
Es gibt mittlerweile den BCC 5.5 gratis auf der Borland-Seite

An neueren IDEs / Compilern hapert's nicht

Blue-Tiger schrieb:

VICI::cpFullName = (char*) malloc(strlen(cpValue)*sizeof(char));

ging mir damals recht schnell von der Hand! Die Alternative wär in BC++ gewesen, sich eine allfällige cString selbst zu bauen, was mir in hinsicht Aufwand/Nutzen nicht gerechtfertigt schien.

Blue-Tiger schrieb:

malloc in C++ verwenden ist vollkommen out! in C++ verwendet man new, new[] und delete, delete[]

<= glaub mir, _mir_ wär's symphatisch gewesen - BC jedoch nicht

Blue-Tiger schrieb:

Was mir sonst noch im Code auffaellt:
class VICI {
  private:
    [...]
  public:
    VICI();
    void TellName(void);
    void TellVersion(void);
    void TellDateTime(void);
    void ExplainName(void);
    void Tell(char *cpWhat);
};
// ... [snip] ...

  VICI::Tell(VICI_INI_LOADED);
  VICI::TellName();
  VICI::TellDateTime();
Das kann kein C++ Compiler akzeptieren, der was auf sich haelt. Die Funktionen sind nicht static, die brauchen eine Instanz um aufgerufen zu werden funktionieren. Das duerfte gar nicht compilieren

Just once to make you even more confused

#include "stdafx.h"

enum INIVALUES{INVALID, NAME, FULLNAME, VERSION, PROMPT};
const char *DAYOFWEEK[] = {"Sunday", "Monday", "Tuesday", "Wensday", "Thuesday", "Friday", "Saturday"};
const int MAX_COMMAND_LENGTH = 80;
typedef enum{TRUE=1, FALSE=0} BOOL;
typedef int HANDLE;
typedef unsigned int UINT;
class VICI {
	private:
		char *cpName;
		char *cpFullName;
		char *cpVersion;
		char *cpPrompt;
		BOOL bExit;
	public:
		VICI();
		BOOL Exit(void);
		void Prompt(char *cpCommand);
		void ParseCommand(char *cpCommand);
		void TellName(void);
		void TellVersion(void);
		void ExplainName(void);
		void Tell(char *cpWhat);
};
BOOL VICI::Exit(void) {
	return bExit;
}
void VICI::ParseCommand(char *cpCommand) {
	if(cpCommand[0]=='\0') {
		return;
	}
	if(stricmp(cpCommand, "exit")==0) {
		bExit=TRUE;
		return;
	}
	if(strcmpi(cpCommand, "who are you")==0||
		stricmp(cpCommand, "who are u")==0||
		stricmp(cpCommand, "who are you?")==0||
		stricmp(cpCommand, "who are u?")==0) {
		VICI::TellName();
		return;
	}
	if(strcmpi(cpCommand, "vici")==0||
		stricmp(cpCommand, "vici?")==0) {
		VICI::ExplainName();
    return;
	}
	cprintf("Unrecognized Command!\r\n");
}
void VICI::Prompt(char *cpCommand) {
	char buffer[MAX_COMMAND_LENGHT+3];
	buffer[0] = MAX_COMMAND_LENGHT + 1;
	cprintf("%s", VICI::cpPrompt);
	strcpy(cpCommand, cgets(buffer));
	cprintf("\r\n");
}

void VICI::TellVersion(void) {
	cprintf("This is version %s!\r\n", VICI::cpVersion);
}
void VICI::Tell(char *cpWhat) {
	cprintf("%s\r\n", cpWhat);
}
void VICI::TellName(void) {
	cprintf("My name is %s.\r\n", VICI::cpName);
}
void VICI::ExplainName(void) {
	cprintf("%s means %s.\r\n", VICI::cpName, VICI::cpFullName);
}
VICI::VICI() {
	HANDLE hVICIiniFile;
	bExit = FALSE;
	char *cpTemp = NULL;
	VICI::cpName = NULL;
	VICI::cpVersion = NULL;
	VICI::cpFullName = NULL;
	VICI::cpPrompt = NULL;
	VICI::bExit = FALSE;
	hVICIiniFile = open("VICI.INI", O_RDWR|O_TEXT);
	if(hVICIiniFile==-1) {
		VICI::Tell(VICI_INI_NOT_FOUND);
		VICI::Tell(VICI_TERMINATED);
		exit(0);
	}
	cpTemp = (char*)malloc((strlen(VICI_INI_LENGTH_INFO)+CountDigits(filelength(hVICIiniFile))-2)*sizeof(char));
	sprintf(cpTemp, VICI_INI_LENGTH_INFO, filelength(hVICIiniFile));
	VICI::Tell(cpTemp);
	free(cpTemp);
	cpTemp = NULL;
	char cBuffer;
	char *cpVariable = NULL;
	char *cpValue = NULL;
	int iCounter = 0;
	do {
		read(hVICIiniFile, &cBuffer, 1);
		if(cBuffer=='#') {
			do {
				read(hVICIiniFile, &cBuffer, 1);
			} while(!(cBuffer==10||eof(hVICIiniFile)));
		} else {
			lseek(hVICIiniFile, -1, SEEK_CUR);
			iCounter = 0;
			do {
				read(hVICIiniFile, &cBuffer, 1);
				iCounter++;
			} while(!(cBuffer=='='||eof(hVICIiniFile)));
			free(cpVariable);
			cpVariable = NULL;
			cpVariable = (char*)malloc(iCounter*sizeof(char));
			lseek(hVICIiniFile, -iCounter, SEEK_CUR);
			read(hVICIiniFile, cpVariable, iCounter);
			cpVariable[iCounter-1] = '\0';
			iCounter = 0;
			do {
				read(hVICIiniFile, &cBuffer, 1);
				iCounter++;
			}while(!(cBuffer==';'||eof(hVICIiniFile)));
			free(cpValue);
			cpValue = NULL;
			cpValue = (char*)malloc(iCounter*sizeof(char));
			lseek(hVICIiniFile, -iCounter, SEEK_CUR);
			read(hVICIiniFile, cpValue, iCounter-1);
			cpValue[iCounter-1]='\0';
			read(hVICIiniFile, &cBuffer, 1);
			read(hVICIiniFile, &cBuffer, 1);
			INIVALUES iv = INVALID;
			if(stricmp(cpVariable, "Prompt")==0) {
				iv = PROMPT;
			}
			if(stricmp(cpVariable, "Name")==0) {
				iv = NAME;
			}
			if(stricmp(cpVariable, "FullName")==0) {
				iv = FULLNAME;
			}
			if(stricmp(cpVariable, "Version")==0) {
				iv = VERSION;
			}
			switch(iv) {
				case PROMPT:
					VICI::cpPrompt = (char*) malloc(strlen(cpValue)*sizeof(char));
					strcpy(VICI::cpPrompt, cpValue);
					break;
				case VERSION:
					VICI::cpVersion = (char*) malloc(strlen(cpValue)*sizeof(char));
					strcpy(VICI::cpVersion, cpValue);
					break;
				case NAME:
					VICI::cpName = (char*) malloc(strlen(cpValue)*sizeof(char));
					strcpy(VICI::cpName, cpValue);
					break;
				case FULLNAME:
					VICI::cpFullName = (char*) malloc(strlen(cpValue)*sizeof(char));
					strcpy(VICI::cpFullName, cpValue);
					break;
				case INVALID:
					cprintf("Unrecognized variable: %s!\r\n", cpVariable);
					break;
			}
		}
	} while(!eof(hVICIiniFile));
	close(hVICIiniFile);
	VICI::Tell(VICI_INI_LOADED);
	VICI::TellName();
//	VICI::TellDateTime();
}
void main(void) {
	VICI v;
	char cCommand[80];
	do {
		v.Prompt(cCommand);
		v.ParseCommand(cCommand);
	}while(!v.Exit());
}

<= compiliert sogar unter VC7!
(etwas abgeändert da eindeutig(VC!=BC)).

Blue-Tiger schrieb:

BTW solltest du Klassendefinitionen und -deklarationen in eigene Dateien auslagern

Wäre das Progrämmchen über die 'quick-and-dirty'-Phase hinausgekommen, hätt ich das gemacht

hab ich wirlich auf 'Absenden' geklickt *rotwerd*
wär mir sehr recht falls n Moderator einen meiner letzten beiden Beiträge killn könnt...

Blue-Tiger

Swordfish schrieb:

Wie gesagt: du koenntest dir
<= compiliert sogar unter VC7!
(etwas abgeändert da eindeutig(VC!=BC)).

ok, sorry, mein Fehler, hab in der Eile nicht gesehen, dass die Methoden innerhalb anderer Klassenmethoden aufgerufen werden Das
VICI:: vor den Namen find ich sehr verwirrend

Swordfish

Blue-Tiger.giveAbsolution();

Is doch passend, ich mein zu Ostern...

groovemaster

Swordfish schrieb:

int main(void) ist doch eindeutig overhead, oder...

Was meinst du mit Overhead?
Der Punkt ist einfach der, dass in C++ int als Rückgabetyp von main vorgeschrieben ist. Ein 'void main(void)' ist deshalb kein korrektes C++. Einige pedantische Compiler werden das auch bemängeln. In C sieht das übrigens nicht viel anders aus.

Swordfish

War nur 'n späßchen...

int main(void);

ist natürlich Korrekt, Verpflichtet jedoch zu 'unnötigem' Datentransport von main() an das aufrufende Environment.

vlg. http://de.wikipedia.org/wiki/Overhead