#include <iostream>
#include <stdlib.h>
#include <stdio.h>
#include <winsock2.h>
#define PORT 50
using namespace std;
SOCKET sd;
sockaddr_in Raddr;
void Init_serv()
{
char IP[32]="127.0.0.1";
WSADATA wsadata;
if(WSAStartup(MAKEWORD(2,2), &wsadata) !=0)
{
cout<<"WSAStartup failed.";
return;
}
if((sd = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)) == -1)
{
cout<<"Socket initialization failed.";
return;
}
memset(&Raddr, 0, sizeof(Raddr));
Raddr.sin_family = AF_INET;
Raddr.sin_addr.s_addr = inet_addr(IP);
Raddr.sin_port = htons(PORT);
if(connect(sd, (sockaddr *) &Raddr, sizeof(Raddr)) == -1)
{
cout<<"Connection failed.";
return;
}
cout<<"Connection successful.";
}
void Send_loop()
{
int Mlen;
char buf[256];
for(;;)
{
gets(buf);
Mlen = sizeof(buf);
if(send(sd, buf, Mlen, 0) == -1)
{
cout<<"Send failed.";
return;
}
}
}
void Serv_cleanup()
{
WSACleanup();
closesocket(sd);
return;
}
int main()
{
Init_serv();
Send_loop();
Serv_cleanup();
}
Code critiqueing
Author
I'm just wondering if anyone can critique my code, let me know if it looks alright and if it would be considered "real" OOP. I am in a "state" where I am attempting to improve my coding skills. Tell me anything and everything you can see that is wrong with this (keep it realistic please).
There's no OOP in this code. Just plain old C stuff, except for iostream. BTW, you don't need to include stdio if you already included iostream.
Hey bud,
- I recommend not doing:
using namespace std;
It defies the point of having a namespace around everything in it if you just bring it all in. It doesn't cost you anything to be explicit and list individual components that you want to bring in from a namespace.
- Your IP char array is too long, needs only to cater for a max of 12 numbers and 3 dots. I would recommend using std::string for storing this. You can attain a char* to the string by doing:
char* pChar = myString.c_str();
This makes it equally suitable for passing into inet_addr().
- You could really bloat this if you wanted to go to a fully functional Socket object model. But i recommend that for something that is this simple, you stick with C.
- It is possible to write Object Oriented C. For example, weak OO programming can be done by grouping procedural code in to header and C file pairs, although again it isn't really worth bothering with here.
Hope that helps,
Dave
- I recommend not doing:
using namespace std;
It defies the point of having a namespace around everything in it if you just bring it all in. It doesn't cost you anything to be explicit and list individual components that you want to bring in from a namespace.
- Your IP char array is too long, needs only to cater for a max of 12 numbers and 3 dots. I would recommend using std::string for storing this. You can attain a char* to the string by doing:
char* pChar = myString.c_str();
This makes it equally suitable for passing into inet_addr().
- You could really bloat this if you wanted to go to a fully functional Socket object model. But i recommend that for something that is this simple, you stick with C.
- It is possible to write Object Oriented C. For example, weak OO programming can be done by grouping procedural code in to header and C file pairs, although again it isn't really worth bothering with here.
Hope that helps,
Dave
Author
I've fixed what was wrong(I think), except for the OOP part. If it wouldn't be too much trouble, could somebody explain what I would have to do in order to make this OOP? I don't need a huge paragraph of what I would have to do, but if that's the only way to explain OOP, forget it.
Does this code look specifically C++ now or would it still be considered C? Thanks..
By the way, the reason I had the idea this was OOP was because the definition on this site: http://en.wikipedia.org/wiki/Object-oriented_programming .
"The idea behind object-oriented programming is that a computer program may be seen as composed of a collection of individual units, or objects, that act on each other, as opposed to a traditional view in which a program may be seen as a collection of functions or procedures, or simply as a list of instructions to the computer." - I got the idea, because I had 3 different parts of the program grouped into 3 separate functions, it would be considered OOP. Please explain, why isn't this OOP?
Does this code look specifically C++ now or would it still be considered C? Thanks..
#include <iostream>#include <cstdlib>#include <winsock2.h>#define PORT 50SOCKET sd;sockaddr_in Raddr;void Init_serv(){ std::string IP="127.0.0.1"; char ip[15]; strcpy(ip, IP.c_str()); WSADATA wsadata; if(WSAStartup(MAKEWORD(2,2), &wsadata) !=0) { std::cout<<"WSAStartup failed."; return; } if((sd = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)) == -1) { std::cout<<"Socket initialization failed."; return; } memset(&Raddr, 0, sizeof(Raddr)); Raddr.sin_family = AF_INET; Raddr.sin_addr.s_addr = inet_addr(ip); Raddr.sin_port = htons(PORT); if(connect(sd, (sockaddr *) &Raddr, sizeof(Raddr)) == -1) { std::cout<<"Connection failed."; return; } std::cout<<"Connection successful.";}void Send_loop(){ int Mlen; std::string buf; char Buf[256]; for(;;) { strcpy(Buf, buf.c_str()); gets(Buf); Mlen = sizeof(Buf); if(send(sd, Buf, Mlen, 0) == -1) { std::cout<<"Send failed."; return; } }}void Serv_cleanup(){ WSACleanup(); closesocket(sd); return;}int main(){ Init_serv(); Send_loop(); Serv_cleanup();}By the way, the reason I had the idea this was OOP was because the definition on this site: http://en.wikipedia.org/wiki/Object-oriented_programming .
"The idea behind object-oriented programming is that a computer program may be seen as composed of a collection of individual units, or objects, that act on each other, as opposed to a traditional view in which a program may be seen as a collection of functions or procedures, or simply as a list of instructions to the computer." - I got the idea, because I had 3 different parts of the program grouped into 3 separate functions, it would be considered OOP. Please explain, why isn't this OOP?
Quote:Original post by cherryhouse
I've fixed what was wrong(I think), except for the OOP part. If it wouldn't be too much trouble, could somebody explain what I would have to do in order to make this OOP?
Does this code look specifically C++ now or would it still be considered C? Thanks..
*** Source Snippet Removed ***
First of all, you have two global variables that represent the state of your server. Those should be members of a "Server" class, with the appropriate accessors. Make the functions members of the class that validate input and act on the private members. Errors should throw exceptions, not return error codes (you simply return doing nothing... not cleaning up after the function :)). The class should be self-contained and only depend on code in the libraries you use (stl, winsock, etc.) which means that it doesn't rely on globals defined within the program that uses it. The program that uses the server class should only be aware of the interface to the class in order to use it. It should not know any details about how it is implemented internally.
You're program is not OOP because you cannot create two different servers without modifying your program significantly. State and code are not packaged up in any meaningful way. This is procedural programming.
Author
Wow, that explains so much. I feel like I just took a giant step forward in the understanding of C++ OOP, although, probably not as big as I think. Thanks a lot.
Author
Would it be more logical putting the class template for the server and the functions inside a header file so that the cpp file looks like:
int main()
{
start_serv();
}
so that start_serv calls everything else, but everything else is "created" inside a header? Or should I create the functions inside the .cpp file so that the header file only holds the class template without the functions?
int main()
{
start_serv();
}
so that start_serv calls everything else, but everything else is "created" inside a header? Or should I create the functions inside the .cpp file so that the header file only holds the class template without the functions?
Author
Alright, so here is what I've come up with so far. It is a big step so I probably have quite a few errors.
Client.h
Client.cpp
I have a problem sending messages to the server with this though. If you spot the error that would be a nice plus.
Update: I've fixed the sending problem by making a public socket descriptor.
[Edited by - cherryhouse on March 14, 2006 1:01:52 AM]
Client.h
#include <iostream>#include <winsock2.h>#include <cstdlib>class Server{ public: SOCKET Sockfd; Server(); ~Server(); void Serv_init( SOCKET sockfd, std::string ip, unsigned short port ); void Serv_cleanup( SOCKET sockfd ); void Serv_send( SOCKET sockfd, std::string buf ); protected: sockaddr_in Raddr; SOCKET sockfd;};Server::Server(){ //Constructor}Server::~Server(){ //Destructor}void Server::Serv_init( SOCKET sockfd, std::string ip, unsigned short PORT ){ char IP[15]; strcpy(IP, ip.c_str()); WSADATA wsadata; if(WSAStartup(MAKEWORD(2,2), &wsadata) !=0) { std::cout<<"WSAStartup failed."; return; } if((Sockfd = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)) == -1) { std::cout<<"Socket initialization failed."; return; } memset(&Raddr, 0, sizeof(Raddr)); Raddr.sin_family = AF_INET; Raddr.sin_addr.s_addr = inet_addr(IP); Raddr.sin_port = htons(PORT); if(connect(Sockfd, (sockaddr *) &Raddr, sizeof(Raddr)) == -1) { std::cout<<"Connection failed."; return; } std::cout<<"Connection successful.";}void Server::Serv_cleanup( SOCKET sockfd ){ WSACleanup(); closesocket(Sockfd); return;}void Server::Serv_send( SOCKET sockfd, std::string buf ){ int Mlen; char Buf[256]; for(;;) { strcpy(Buf, buf.c_str()); gets(Buf); Mlen = sizeof(Buf); if(send(Sockfd, Buf, Mlen, 0) == -1) { std::cout<<"Send failed."; } }}Client.cpp
#include "Client.h"#define PORT 50int main(){ SOCKET sd; Server jay; jay.Serv_init(sd, "127.0.0.1", 50); jay.Serv_send(sd, "hello"); jay.Serv_cleanup(sd);}I have a problem sending messages to the server with this though. If you spot the error that would be a nice plus.
Update: I've fixed the sending problem by making a public socket descriptor.
[Edited by - cherryhouse on March 14, 2006 1:01:52 AM]
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
