Ist dieser Code "schön"?

DocShoe

Vielleicht wäre std::map<char,unsigned int> hübscher als ein rohes C-style Array. Damit musst du keinen Index mehr verbiegen und hast eine direkete Zuordnung von Buchstabe zu dessen Anzahl.

zeusosc

encourriage schrieb:

Das carriage return macht deinen Code unportabel, lass das.

?? Das ist nicht ganz richtig, geschichtlich gesehen ist der cursor bei der
ausgabe von \n nicht nach vorne gesetzt worden,..
Der carrier musste nach vorne mittels \r gesetzt werden.
Halt die pure Funktionalität der Steuerungszeichen (ASCII) ist Aufgrund von
Abwärtskompatibilität bei Konsolenanwendungen (und EXAKT bei externer perephiere) heute noch zu Gewährleisten.

Daher ist die genannte "unportabilität" eigentlich nur durch verwendung eines unvollständigen satzes an steuerzeichen vorhanden...

grüße

du könntest noch return -1; für "Nicht genug Parameter" und return -2; für "Dateifehler" machen.

Beim Parameterfehler sollte er sagen wie es richtig geht.

Shade Of Mine

zeusosc schrieb:

?? Das ist nicht ganz richtig, geschichtlich gesehen ist der cursor bei der
ausgabe von \n nicht nach vorne gesetzt worden,..
Der carrier musste nach vorne mittels \r gesetzt werden.
Halt die pure Funktionalität der Steuerungszeichen (ASCII) ist Aufgrund von
Abwärtskompatibilität bei Konsolenanwendungen (und EXAKT bei externer perephiere) heute noch zu Gewährleisten.

Daher ist die genannte "unportabilität" eigentlich nur durch verwendung eines unvollständigen satzes an steuerzeichen vorhanden...

Blödsinn. Sorry.

Prinzipiell stimmt es ja was du schreibst - aber das ist alles irrelevant. \n ist die korrekte Schreibweise hier. Denn dafür sorgt das System. \r\n kann zuviel machen (muss aber nicht). zB auf OS 9 wäre \r\n falsch.

Deshalb: \n ist die korrekte und portable Form.

Shade Of Mine schrieb:

Deshalb: \n ist die korrekte und portable Form.

Ist nicht eher std::endl die korrekte und portable Form? Macht halt leider noch ein flush dazu, sollte hier aber nicht weh tun. Ich kenne es so, dass die Zeilentrenner wie folgt sind: Windows \r\n, Linux \n, MacOS \r.

Shade Of Mine

fdgdfg schrieb:

Shade Of Mine schrieb:

Deshalb: \n ist die korrekte und portable Form.

Ist nicht eher std::endl die korrekte und portable Form? Macht halt leider noch ein flush dazu, sollte hier aber nicht weh tun. Ich kenne es so, dass die Zeilentrenner wie folgt sind: Windows \r\n, Linux \n, MacOS \r.

Nein \n in einem Text-Kontext ist schon OK. Wenn du mit einer binaer Datei (ios::binary) arbeitest, ist \r\n und \n unter windows unterschiedlich - im Text-Kontext nicht.

Und unter OS X hast du auch \n als Line Ending. \r war unter OS 9

Frage: wenn \n unter Windows durch \r\n ersetzt wird, wäre dann ein \r\n selbst dort falsch? Es müsste doch zu \r\r\n geändert werden.

Shadow of Mine hat es doch schon erläutert. Der einzige Grund für das Text-Flag bei Dateioperationen, ist der unterschiedliche Zeilenumbruch. Ein Zusätzliches Carriage Return \r tut aber nicht weh. Es veranlasst ja nur, dass der Cursor an den Zeilenanfang geht, dort ist er aber schon.

Nexus

Paul Müller schrieb:

Ein Zusätzliches Carriage Return \r tut aber nicht weh.

Kommt wohl sehr drauf an, wie der Editor das interpretiert. Warum überhaupt ein '\r' zuviel, wenn '\n' reicht?

#include <iostream>
#include <fstream>
#include <cctype>
#include <map>

typedef std::map<char, int> Occurrences;

int main(int argc, char *argv[])
{
	if (argc < 2)
	{
		std::cerr << "Zu wenig Parameter uebergeben" << std::endl;

		return -1;
	}

	std::ifstream file("a.txt");

	if (!file.good())
	{
		std::cerr << "Datei konnte nicht geoffnet werden" << std::endl;

		return -2;
	}

	Occurrences occurrences;

	for (int i = 0; i < 26; ++i)
	{
		occurrences['A' + i] = 0;
	}

	int counted = 0;

	while (!file.eof())
	{
		char ch = file.get();

		if (isalpha(ch))
		{
			++occurrences[toupper(ch)];

			++counted;
		}
	}

	std::cout.precision(3);

	for (Occurrences::iterator it = occurrences.begin(); it != occurrences.end(); ++it)
	{
		std::cout << it->first << " = " << it->second << " (" << (static_cast<float>(it->second) / counted) * 100.f << "%)" << std::endl;
	}
}

abc.w

trzrtztr schrieb:

...

Schön
Persönlich mag ich statt

if (!file.good())

so was:

if (0 == file.good())

Und statt

for (int i = 0; i < 26; ++i)

das:

size_t i = 0u;
...
for (i = 0u; i < 26u; ++i)

ints mag ich überhaupt nicht

Nexus

abc.w schrieb:

Persönlich mag ich statt
if (!file.good())
so was:
if (0 == file.good())

Nicht nur, dass du 0 statt false nimmst, auch die explizite Abfrage boolscher Werte und das Literal auf der linken Seite... Finde ich sehr übel.

Werner Salomon

trzrtztr schrieb:

int counted = 0;

    while (!file.eof())
    {
        char ch = file.get();
        // ..

.. es sollte doch bei Funktionen, die einen Fehler zurück liefern, normal sein, dass man zuerst die Funktion aufruft und erst danach schaut, ob ein Fehler aufgetreten ist - oder?
Nur beim Lesen aus einem Stream scheinen viele anderer Meinung zu sein.

Mit der obigen Konstruktion wird als letztes Zeichen ein EOF gelesen, was implizit in einem char konvertiert wird - hoffentlich ist es kein Buchstabe, dann fällt es nicht auf und erst danach geht der Stream in den Zustand EOF. Hätte der Stream während des Lesens einen Fehler (Datei auf einem Server, Netztstecker fliegt raus) dann entpuppt sich obiges als Endlos-Schleife, weil der Stream zwar nach FAIL aber nie mehr nach EOF geht.

Korrekt wäre hier z.B. ein

for( char ch; file >> ch; ) {
        // ...

Gruß
Werner

Standardheader alphabetisch ordnen
<locale> anstatt <cctype>, da gibts ein isalpha nach C++-Art
typedef in main reinnehmen, es ist nur da relevant
!file.good() besser !file (wobei es hier mehrere Meinungen gibt)
Occurrences eventuell als std::vector definieren, weil schneller (vorausgesetzt 'A'=='B'-1=='C'-2==...=='Z'-26 )
Ansonsten: lass die Initialisierung von occurences komplett weg, wenn ein Eintrag nicht vorhanden ist, ist der standardmässig 0, ausserdem werden nur die Buchstaben mit einem Eintrag versehen, die auch benötigt werden. Dann muss nur noch die letzte Schleife angepasst werden.
int counted = 0; besser: size_t alpha_count = 0;
Ideale Verbesserung wurde schon vorgeschlagen von while (!file.eof()) { char ch = file.get();
std::cout.precision(3); besser direkt in der Ausgabe
Der static_cast<float> lässt sich vermeiden, wenn du das 100f nach vorne nimmst
"%)" << std::endl wäre besser als "%)\n" , nach jeder Zeile manuell zu flushen ist wohl nicht nötig (am Ende des Programms)
Achja: "A = x (y%)" mag ich als Mathematiker nicht sonderlich. Eventuell Doppelpunkt?

Lass dich nicht verunsichern: im Grossen und Ganzen gefällt mir dein Code besser als viele andere Codingstyles (z.B. der von abc.w), entgegen meinem Namen bin ich tendenziell zufrieden. Es gibt nur zu praktisch allem etwas zu motzen.

abc.w schrieb:

Und statt

for (int i = 0; i < 26; ++i)

das:

size_t i = 0u;
...
for (i = 0u; i < 26u; ++i)

Igitt.

Shade Of Mine

abc.w schrieb:

ints mag ich überhaupt nicht

Wenn dann unsigned wenn es dich so stört. Prinzipiell ist hier aber int (bzw unsigned) deutlich besser als size_t.

Und das u hinter den Zahlen, häßlich und bringt nichts.

zu if (0 == file.good()) hat Nexus ja schon alles gesagt.

pumuckl

abc.w schrieb:

Und statt

for (int i = 0; i < 26; ++i)

das:

size_t i = 0u;
...
for (i = 0u; i < 26u; ++i)

Über size_t kann man sich streiten. Kommt imo drauf an wozu der Zähler benutzt wird. Wenn man penibel sein will, kann man für jede Iterierung den "richtigen" Typen nehmen, d.h. std::size_t für C-Arrays, std::vector<T>::size_type für vector, ...
Wenn man nicht so penibel sein will reicht auch unsigned oder int, die Konvertierung tut nicht weh (wenn denn was zu konvertieren ist).

Was aber auf keinen Fall schön ist, ist die Definition der Schrittvariablen aus dem for-Scope zu ziehen. Die hat außerhalb nichts verloren, und dazu gibts schließlich das for-init-statement.

Ist dieser Code &quot;schön&quot;?

Ist dieser Code "schön"?