AL_31_09 - Lekcja szyfrowania - Sprawdzenie przejrzystości i drobna poprawa

Question

#include <iostream>
#include <string>
#include <cstring>
using namespace std;
string wyraz;

string funkcja (string str) {
		
	char *cstr = new char[str.length() + 1];
	int a [str.length() + 1];
	char wyraz [str.length() + 1];
	strcpy(cstr, str.c_str());                 //zamiana string na char
	
	for (int przes = 0; przes<26; przes++) {
		cout<<endl;
		for (int i=0;i<(str.length());i++) {
		a[i] = cstr[i];    //zamiana char na int
		
		if (a[i] + przes>90) {                     //wyświetlanie i przesuwanie
		 wyraz[i] = a[i] + przes -26;
		}
		else
		wyraz[i] = (a[i] + przes);
		cout<<wyraz[i];
	}
}
	delete [] cstr;
	return "";
};

int liczba;

int main(int argc, char** argv) {
	cin>>wyraz;
	funkcja(wyraz);
	

	return 0;
}

Napisałem kod który deszyfruje kod przesunięciem cezara. Mam prośbę żeby ktoś się przyczepił co do przejrzystości kodu i mnie poprawił :) 

I mam problem, gdyż nie wyświetla mi wszystkich wyrazów tylko jeden. Nie wiem jak zrobić żeby program wiedział ile wyrazów wprowadziłem i potem wprowadzić to do pętli.

Answer 1

 mam problem, gdyż nie wyświetla mi wszystkich wyrazów tylko jeden.

`cin >> wyraz` z definicji wczytuje do spacji. Jeśli chcesz linię, użyj getline().

return "";

Jeśli funkcja nie musi nic zwracać, to niech będzie `void`.

char *cstr = new char[str.length() + 1];

Po to jest std::string, żeby nie trzeba było się ręcznie bawić w alokowanie stringów.

int a [str.length() + 1];

Takie tworzenie tablicy technicznie nie jest legalne w C++ie; powinieneś dostać co najmniej ostrzeżenie od kompilatora. No i ogólnie nie potrzebujesz w ogóle tej tablicy do niczego. (no i nazwa `a` nic nie mówi)

strcpy(cstr, str.c_str());

Po to również jest C++ i std::string żeby nigdy nie trzeba było używać funkcji takich jak strcpy. Ogólnie... po co Ci w ogóle ta "zamiana"?

int liczba;

Nieużyta zmienna. (o tym też kompilator powinien był ostrzec)

    }
}

Nierówne klamry...

string wyraz;

Nie musi być globalna.

//zamiana char na int

Raczej taka zamiana nie jest potrzebna?

Comments

Poprawiłem się :)

#include <iostream>
#include <string>
#include <cstring>
using namespace std;


void odszyfruj (string wyraz) {
	const int stala = 26;

	for (int przes=0;przes<26;przes++) {
		cout<<endl;
		for (int i=0;i<wyraz.size();i++) {
			if ((int(wyraz[i]) + przes) > 90) {
				wyraz[i] = int(wyraz[i]) + przes - stala;
				cout<<wyraz[i];
			}
			else {
				wyraz[i] = int(wyraz[i]) + przes;
				cout<<wyraz[i];
			}
		}
	}
}

int main(int argc, char** argv) {
string wyraz;
	
	cin>>wyraz;
	odszyfruj(wyraz);
	
	return 0;
}

 

Answer 2

• Brak wcięć tam gdzie trzeba (kod w tym wewn. forze czy ifie). Klamry zamykające też rozwalone (te od pętli) - powinny być na takim wcięciu jak samo słówko for. Komentarze też sobie latają gdzie chcą. Dodatkowo nadmiarowy średnik po definicji funkcji.
• Strasznie niejednolity styl. Spacje między operatorami i operandami czasem są, czasem nie ma. Wcięcia czasem są, czasem nie ma. Nawet nie styl - po prostu czasem kod da się czytać a czasem nie. Nie mam pojęcia jak to zrobiłeś.
• Dwa razy deklarujesz zmienną `wyraz` - globalny string i char[] w funkcji. Już samo używanie globali jest potencjalnym błędem, a tu nie ma żadnego ku temu żadnego powodu.
• Po co funkcja w ogóle cokolwiek zwraca jeśli jedyną instrukcją return jest return ""?
• Nazwa 'funkcja' dla funkcji naprawde nie mówi za wiele o jej przeznaczeniu. Podobnie nazwy zmiennych: 'str', 'cstr', 'a'.
• Stałe powinieneś gdzieś sobie zapisać do jakiejś... stałej. Po prostu nazwać jakoś tą liczbe (np. 26 u ciebie) i do tej stałej się w kodzie odnosić zamiast miliardy razy pisać bezpośrednio liczbe. W taki sposób, jeśli zachce ci się zmian, łatwo je wprowadzisz - musisz wtedy tylko zmienić wartość tej zadeklarowanej gdzieś stałej.

Reszta uwag będzie bardziej odnośnie wydajności czy sensu niż stylu:

• Takie obiekty jak string koniecznie przez referencje (const jeśli można) przekazuj do funkcji. Kopiowanie trwa stringa (alokacja nowej tablicy, przekopiowanie wszystkiego do niej) trwa relatywnie długo.
• Ponownie wracamy do "rozdwojenia jaźni" :P Linie 10 i 11: deklaracja tablicy o rozmiarze będącym zmienną jest nielegalne w c++. GCC ci na to pozwala (takie rozszerzenie), ale ogólnie nie powinieneś tak robić. Wydaje się jakbyś o tym wiedział, bo w 9. linii zaalokowałeś tablice dynamicznie... Stąd nazwałem to "rozdwojeniem jaźni" (podobna historia jak z wcięciami itd.) :P
• Nie rozumiem sensu tej zewnętrznej pętli for. Po co chcesz przesuwać wszystkie litery 26 razy o kolejne 'offsety'? Może poczytaj jeszcze raz o szyfrze Cezara :P
• 17 linia. "Zamiana char na int". Po co? char jest typem liczb całkowitym i możesz na nim normalnie wykonywać operacje arytmetyczne czy bitowe jak na każdym innym. Tablica `a` jest zupełnie zbędna. Nawet jeśli już koniecznie byś chciał pracować na intach - korzystasz tylko z jednej litery na raz, nie potrzebujesz całej tablicy! Wystarczy, że jeden znak "w locie" przekonwertujesz na inta.
• Bez potrzeby korzystasz z tych wszystkich tablic. Dlaczego nie używasz też tutaj stringa?