• Najnowsze pytania
  • Bez odpowiedzi
  • Zadaj pytanie
  • Kategorie
  • Tagi
  • Zdobyte punkty
  • Ekipa ninja
  • IRC
  • FAQ
  • Regulamin
  • Książki warte uwagi

Ocena kodu c/cpp

Object Storage Arubacloud
0 głosów
357 wizyt
pytanie zadane 10 marca 2020 w C i C++ przez edwardkraweznik Dyskutant (9,930 p.)
otwarte ponownie 12 marca 2020 przez edwardkraweznik

Witam.

Zwracam się z prośbą o ocenę poniższego kodu, którego zadaniem jest wspomagać postfixa.

Kod celowo nie jest pisany obiektowo  a serwer jest blokujący jednowątkowy...

#include <string>

#include <iostream>
#include <fstream>

#include <ctime>
//-------------------------------
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <netinet/in.h>
#include <arpa/inet.h>
//-------------------------------
#define LEN 10240
#define PORT 2121
#define IPNUM "127.0.0.1"
#define CONN 5
//-------------------------------
#define DUNNO "action=dunno\n\n"
#define DEFER "action=defer_if_permit Service temporarily unavailable\n\n"
#define REJECT "action=reject Incorrect email address\n\n"

//--- greylis delay sec
#define DELAY 120

int main()
{

struct sockaddr_in serwer =
	{
	.sin_family = AF_INET,
	.sin_port = htons(PORT)
	};

if(inet_pton(AF_INET, IPNUM, & serwer.sin_addr ) <= 0)
	{
	perror("inet_pton() ERROR");
	exit(1);
	}

const int socket_ = socket(AF_INET, SOCK_STREAM, 0);

if(socket_ < 0)
	{
	perror("socket() ERROR");
	exit(2);
	}

socklen_t len = sizeof(serwer);

if(bind(socket_,(struct sockaddr *) & serwer, len) < 0)
	{
	perror("bind() ERROR");
	exit(3);
	}

if(listen(socket_, CONN) < 0)
	{
	perror("listen() ERROR");
	exit(4);
	}

while(1)
	{
	struct sockaddr_in client = {};

	const int clientSocket = accept(socket_,(struct sockaddr *) & client, & len);

	if(clientSocket < 0)
		{
		perror("accept() ERROR");
		continue;
		}

	char buffer[LEN] = {};

	if(recv(clientSocket, buffer, sizeof(buffer), 0) <= 0)
		{
		perror("recv() ERROR");
		exit(5);
		}

	//--------------------------------------------------------------------------

	std::string buff;	//--- cpp buff
	buff = buffer;		//--- copy c buffer to cpp buff

	//std::cout << buff.length();

	std::string line = "";

	std::string sender = "";
	std::string sasluname = "";

	while (buff.find("\n"))
		{

		line = buff.substr(0, buff.find("\n"));
		buff.erase (0, buff.find("\n")+1);

		if (line.find("sender=") == 0)
			{
			sender = line.substr(7, line.length());
			}

		else if (line.find("sasl_username=") == 0)
			{
			sasluname = line.substr(14, line.length());
			}

		}

//	std::cout << "|||" << sender << "|||" << sasluname << "|||\n";

	//--- sender --- sasluname ----------------------------------------

	if (sasluname != "" && sasluname == sender) //--- gdy user lokalny prawidłowy email
		{
		strcpy( buffer, DUNNO );
		std::cout << "OK local sender" << "\n";

		}

	else if (sasluname != "" && sasluname != sender) //--- podszywanie się
		{
		strcpy( buffer, REJECT );
		std::cout << "REJECT sasl|sender - " << sasluname << "|" << sender  << "\n";
		}

	else 	{ //--- remote user

		std::cout << "remote user" << "\n";

		std::time_t t = std::time(nullptr);

		std::ifstream file ("grey/"+sender);

		if ( file.is_open () )
			{

			while ( getline (file,line) )
				{

				if (std::stoi(line) + DELAY <= t)
					{
					strcpy( buffer, DUNNO );
					std::cout << "GREY dunno\n";
					}
				else
					{
					strcpy( buffer, DEFER );
					std::cout << "GREY defer\n";
					}
				}
			file.close();

			}

		else 	{
			std::ofstream newfile;
			newfile.open ("grey/"+sender);
			newfile << t;
			newfile.close();
			strcpy( buffer, DEFER );
			std::cout << "GREY defer\n";
			}

		}


//	strcpy( buffer, DUNNO );



	if(send(clientSocket, buffer, strlen(buffer), 0) <= 0)
	{
	perror("send() ERROR");
	exit(6);
	}

	shutdown(clientSocket, SHUT_RDWR);
	}
shutdown(socket_, SHUT_RDWR);
}

 

komentarz 10 marca 2020 przez j23 Mędrzec (194,920 p.)
        if (recv(clientSocket, buffer, sizeof(buffer), 0) <= 0) {
            perror("recv() ERROR");
            exit(5);
        }

        std::string buff; //--- cpp buff
        buff = buffer; //--- copy c buffer to cpp buff

Ryzykowna konstrukcja. Tak zrób:

char buffer[LEN] = {};
int n;

if ((n = recv(clientSocket, buffer, sizeof(buffer), 0)) <= 0) {
  perror("recv() ERROR");
  exit(5);
}

std::string buff(buffer, n);

 

komentarz 10 marca 2020 przez edwardkraweznik Dyskutant (9,930 p.)
a dlaczego tak ? nie kumam za bardzo tego
komentarz 10 marca 2020 przez j23 Mędrzec (194,920 p.)

Zakładasz, że recv zawsze wpisze do tablicy buffer c-stringa zakończonego zerem (o ile klient w ogóle wysyła c-stringa).

komentarz 10 marca 2020 przez edwardkraweznik Dyskutant (9,930 p.)
edycja 10 marca 2020 przez edwardkraweznik

no tak.

zobacz: http://www.postfix.org/SMTPD_POLICY_README.html

do tego serwera łączy się tylko 1 klient jest nim postfix

a to n to co robi ? bo zgodnie z dokumentacją rozmiar mam tu:

recv(sockfd, buf, len, flags);

tutaj także nic takiego nie napisano:

https://www.cplusplus.com/reference/string/string/string/

no chyba że chodzi o to:

string (const char* s, size_t n);

tylko objaśnij co to daje? co się stanie jak postfix jakims cudem wysle nie takie dane ?

komentarz 10 marca 2020 przez j23 Mędrzec (194,920 p.)

W n masz ilość bajtów odebranych przez recv. I dokładnie taka ilość jest wpisywana z buffer do buff.

komentarz 10 marca 2020 przez edwardkraweznik Dyskutant (9,930 p.)
hmm.

Czyli chodzi o to, że jak tablica znakow ma wymiar 10240 i nie znajdzie się w niej 0 na koncu to do stringa trafi te 10240 i czesc z tego to beda jakies smieci z ramu ?
komentarz 10 marca 2020 przez j23 Mędrzec (194,920 p.)
Gorzej. Trafi tam też wszystko to, co nie jest zerem, a znajduje się poza tablicą.
komentarz 10 marca 2020 przez edwardkraweznik Dyskutant (9,930 p.)
ok dzięki za info

2 odpowiedzi

0 głosów
odpowiedź 10 marca 2020 przez RafalS VIP (122,820 p.)
wybrane 10 marca 2020 przez edwardkraweznik
 
Najlepsza
  1. Zacznij używać jakiegoś formattera do kodu. Po co martwić się wstawianiem białych znaków? Osobiście "Format document" to najczęstszy skrót jakiego używam w IDE.
  2. Wszystko w mainie. Brak funkcji. Złożoność cyklometryczna maina na poziomie 19.
  3. Niepotrzebne komentarze:
    //--- copy c buffer to cpp buff
    Nie komentuje się prostych i powszechnych operacji z biblioteki standardowej, jeśli ktoś nie wie co tam się stało powinien to wygooglować na własną rękę.
  4. Użycie komentarzy do opisu bloków kodu zamiast funkcji.
    Zamiast: //--- gdy user lokalny prawidłowy email
    Napisz funkcję w stylu: isCorrectEmail
  5. Mieszanie C++ i C przeważnie jest brzydkie, ale wiem, że przy socketach ciężko to ominąć. Jedyna opcja to wrzucenie niskopoziomowego kodu w C do nazwanych funkcji. Wtedy przynajmniej w mainie poziom abstrakcji jest wyższy.
  6. Jeśli piszesz w C++ to używaj constexpr zamiast define.
  7. line.substr(7... Długość może się rozjechać gdy zmodyfikujesz string "sender=" a zapomnisz o siódemce. Lepsze byłoby użycie std::string::length
  8. Plików przeważnie nie trzeba zamykać, dzieje się to w destruktorze std::fstream i pochodnych.
  9. Miałbym wątpliwości co do użycia std::exit w kodzie C++: https://stackoverflow.com/questions/30250934/how-to-end-c-code. Zmień je na return.

exit is a C function and it's not aware nor compatible with the C++ idioms. It does not perform cleanup on your objects, 

komentarz 10 marca 2020 przez edwardkraweznik Dyskutant (9,930 p.)
no fakt... zastanawiam się właśnie czy jakoś na obiekty tego nie przerobić
komentarz 12 marca 2020 przez edwardkraweznik Dyskutant (9,930 p.)

@RafalS, propo constexpr

nie zadziała to tak, że będę miał zdefiniowaną jakoś stałą globalną ?

wolałbym tego uniknąć(jeśli tak będzie)

komentarz 12 marca 2020 przez RafalS VIP (122,820 p.)
Niekoniecznie, ale nawet jeśli, to w czym problem?
komentarz 12 marca 2020 przez edwardkraweznik Dyskutant (9,930 p.)
nie wiem w czym ale ponoć jest zasada, że nie wolno tworzyć zmiennych/stałbych globalnych.

kiedyś byłem za to rugany i rugowany...

PS: napisz co sądzisz o zmiennych i stałych globalnych ?
komentarz 12 marca 2020 przez RafalS VIP (122,820 p.)
No widzisz, a ja nie widzę nic złego w stałych globalnych. Deklaruj do woli. Co do zmiennych globalnych, to musisz mieć porządny powód, raczej unikać.
komentarz 12 marca 2020 przez edwardkraweznik Dyskutant (9,930 p.)
ok, wielkie dzięki za info... faktycznie na stałych globalnych to będzie fajnie wyglądało
0 głosów
odpowiedź 10 marca 2020 przez Bondrusiek Maniak (61,370 p.)

Cześć,

ogólnie kod jest dobry. Jedynie do czego bym się doczepił to formatowanie. Na przykład dla if'ów, pętli czy funkcji klamra otwierająca oraz zamykająca powinna być w tym samym miejscu przy starcie nazwy danego bloku.Np

QVariant CAN_Model::data(const QModelIndex &index, int role) const
{
    if( index.row() < 0 || index.row() >= m_canStructs.count() )
    {
        return QVariant();
    }
    CANStruct* canStruct = m_canStructs[ index.row() ];
    if( role == Qt::DisplayRole || role == Qt::EditRole )
    {
        if( index.column() == 0)
        {
            return canStruct->name;
        }
        if( index.column() == 1 )
        {
            return QString::number(canStruct->value);
        }
        if( index.column() == 2 )
        {
            return  canStruct->comment;
        }
    }
}

Dodatkowo niepotrzebnie łamiesz linie

    std::string buff;   //--- cpp buff
    buff = buffer;      //--- copy c buffer to cpp buff
 
    //std::cout << buff.length();
 
    std::string line = "";
 
    std::string sender = "";

Nie ma sensu tak bez laku używać spacji. Jakby co to powinieneś użyć spacji aby oddzielić jeden funkcjonalny blok kodu od drugiego. Jeszcze możesz używać komentarz jednowierszowy // na samym początku bez żadnych wcięć.

Tutaj masz jakiś stronkę która formatuje Ci kod:

https://codebeautify.org/cpp-formatter-beautifier

komentarz 10 marca 2020 przez edwardkraweznik Dyskutant (9,930 p.)
dzięki za pomoc

Podobne pytania

0 głosów
1 odpowiedź 174 wizyt
pytanie zadane 27 listopada 2019 w Rozwój zawodowy, nauka, praca przez Mike_920 Nowicjusz (120 p.)
0 głosów
2 odpowiedzi 612 wizyt
+1 głos
3 odpowiedzi 2,390 wizyt
pytanie zadane 29 marca 2017 w C i C++ przez WireNess Stary wyjadacz (11,240 p.)

92,576 zapytań

141,426 odpowiedzi

319,652 komentarzy

61,961 pasjonatów

Motyw:

Akcja Pajacyk

Pajacyk od wielu lat dożywia dzieci. Pomóż klikając w zielony brzuszek na stronie. Dziękujemy! ♡

Oto polecana książka warta uwagi.
Pełną listę książek znajdziesz tutaj.

Akademia Sekuraka

Kolejna edycja największej imprezy hakerskiej w Polsce, czyli Mega Sekurak Hacking Party odbędzie się już 20 maja 2024r. Z tej okazji mamy dla Was kod: pasjamshp - jeżeli wpiszecie go w koszyku, to wówczas otrzymacie 40% zniżki na bilet w wersji standard!

Więcej informacji na temat imprezy znajdziecie tutaj. Dziękujemy ekipie Sekuraka za taką fajną zniżkę dla wszystkich Pasjonatów!

Akademia Sekuraka

Niedawno wystartował dodruk tej świetnej, rozchwytywanej książki (około 940 stron). Mamy dla Was kod: pasja (wpiszcie go w koszyku), dzięki któremu otrzymujemy 10% zniżki - dziękujemy zaprzyjaźnionej ekipie Sekuraka za taki bonus dla Pasjonatów! Książka to pierwszy tom z serii o ITsec, który łagodnie wprowadzi w świat bezpieczeństwa IT każdą osobę - warto, polecamy!

...