Sign in to follow this  
cherryhouse

Code critiqueing

Recommended Posts

cherryhouse    100
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).
#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();
}

Share this post


Link to post
Share on other sites
Kalazart    148
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.

Share this post


Link to post
Share on other sites
dave    2187
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

Share this post


Link to post
Share on other sites
cherryhouse    100
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..

#include <iostream>
#include <cstdlib>
#include <winsock2.h>

#define PORT 50

SOCKET 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?

Share this post


Link to post
Share on other sites
smr    2468
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.

Share this post


Link to post
Share on other sites
cherryhouse    100
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.

Share this post


Link to post
Share on other sites
cherryhouse    100
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?

Share this post


Link to post
Share on other sites
cherryhouse    100
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
#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 50

int 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]

Share this post


Link to post
Share on other sites
iMalc    2466
You could now at a stretch call this OOP, since you actually have an object. But it's a stretch.

That code can't compile because it has two instances of "SOCKET Sockfd;". Delete the top one.

You've written a constructor and a destructor. The thing is, there's no point in writing an empty public default constructor. Typically it would contain code to initialise member variables. In this case this is also what you need to do. You need it to initialise "Raddr" and "sockfd". Your destructor should also do any cleanup, by calling "Serv_cleanup".

Also, the whole point of making this a class is that it contains the functions and data together. So you don't pass in "sockfd" to any of those functions, and you do not declare a SOCKET in main. It's already contained in the class and it just uses "sockfd" automatically. You got it right with "Raddr" in this respect, so just do the same.

For extra credit, learn what "const references" are and use them to pass the strings to those functions.

The only two comments you've added serve no useful purpose.

Share this post


Link to post
Share on other sites
CTar    1134
Most of your problems have been pointed out. One little thing which I don't like is that the name of one of your parameters is the same as one of your members (of the same type) you could very easily mix up the two (I'm talking about sockfd).

You need to understand that OOP is about objects. Objects are abstractions. In this case OOP is about Server objects, and they are abstractions for servers. When working with an abstraction you shouldn't need to know anything about how it works internally. So the user of the Server class shouldn't really worry whether you use SOCKET members internally etc.

One thing I would improve when I had some time (not very important) is to also make an address class which abstracts away an IP and a port number. Something like this (It needs a couple of functions for stuff like accesing individual elements an operator<< taking a std::ostream, and an operator>> taking a std::istream etc. Also I haven't provided an implementation and there might be some errors since it was typed on GDNet):

class server_address
{
// Assume sizeof(short) == 2 and sizeof(int) >= 2 where 1 byte equals 8 bit
// Make an assert in the constructor or a static assert if such a thing
// is available
short ip[4]; // negative values means not-set
int port; // negative values means not-set
public:
server_address();
explicit server_address(unsigned int ip,unsigned int port = 0);
// This way to pass strings are in most cases better because you wont
// have to copy the whole string.
explicit server_address(const std::string& ip);
~server_address();

// Don't use operator= because there might be some confusion about what
// it does, maybe it assigns a name, the IP, the port or the IP and port.
// Maybe the format is "a.b.c.d:e" maybe the syntax is: "a.b.c.d/e".

// Format = a.b.c.d where a, b, c and d is numbers in base-10 format
// from 0 to 255
// Return false if the IP can't be set from the string.
bool IP_from_string( const std::string& IP );

bool port_from_string( const std::string& port);

// Functions to check whether the port and IP already have been assigned
bool port_set();
bool IP_set();

// Format: ip1.ip2.ip3.ip4:port
std::string to_string();
};



And one more problem I just noticed is that you internally use C functions and chars. For example:

char IP[15];
strcpy(IP, ip.c_str());

Unless you have a very good reason this should be:

std::string IP;
IP = ip;

Also why do you even create the IP variable couldn't you just change this line:

Raddr.sin_addr.s_addr = inet_addr(IP);

to

Raddr.sin_addr.s_addr = inet_addr(ip.c_str());


And in your Send_loop function you have a variable called Mlen (why is it called that BTW, message length?) you assign it inside the loop, but it would be easier to just assign it once outside the loop since the size of buf doesn't change. Also you can use const on Mlen, so you should do something like this:

void Send_loop()
{
const int Mlen = sizeof(buf);
char buf[256];

for(;;)
{
gets(buf);
if(send(sd, buf, Mlen, 0) == -1)
{
cout<<"Send failed.";
return;
}
}
}



It will most likely result in the same code because of the optimizer, but IMO this looks cleaner since you wont worry about why the buf's size is taken so many times.

One last thing, it is normal to end your cout statements with a newline (std::endl) so that the output won't just all be on the same line.

Share this post


Link to post
Share on other sites
Zahlman    1682
I'll take a stab at it:


#include <iostream>
#include <winsock2.h>
#include <cstdlib>
#include <string>
#include <algorithm>
#include <iterator>

// I am writing the function implementations within the class definition body.
// You normally shouldn't in C++, unless they're short - I'm just being lazy.

class Server {
// We let the socket instance be private because the whole point of the
// Server object is to manage this thing for us - other people shouldn't have
// to know it exists, but instead go through the Server interface.
SOCKET sockfd;
// similarly our struct...
sockaddr_in Raddr;

public:
// The only valid way to initialize a server requires providing an IP and
// port. So we provide a constructor which accepts these arguments.

Server(const std::string& ip, unsigned short port) :
sockfd(socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)) {
// Note that the string is passed by const reference.
// Also note the use of an initializer list.

// To deal with serious errors in constructors, normally you
// throw an exception. The destructors for any so-far-constructed
// things will be called, but not for the current object (since it wasn't
// actually constructed). Fortunately, our data members are just some simple
// types plus a vector, each of which can handle themselves fairly
// nicely, so we are ok.
if(sockfd == -1) {
throw std::exception("Socket initialization failed.");
}
// Now let's take care of the WSADATA busienss:
WSADATA wsadata;
if (WSAStartup(MAKEWORD(2,2), &wsadata) != 0) {
throw std::exception("WSAStartup failed.");
}
// Now we can set up the Raddr. We don't need to copy the IP string here.
// Note that the original code isn't even quite right: An IP string for
// a standard dot-and-number address requires *16* bytes, not 15 (don't
// forget the null terminator!).
memset(&Raddr, 0, sizeof(Raddr));
Raddr.sin_family = AF_INET;
Raddr.sin_addr.s_addr = inet_addr(ip.c_str());
// If we did need to copy it, there are still much more refined techniques
// available with the standard library that will avoid size limitations and
// also not require manual memory management.
Raddr.sin_port = htons(port);

if(connect(Sockfd, (sockaddr *) &Raddr, sizeof(Raddr)) == -1) {
// At this point we would need to do cleanup of Raddr if there were any
throw std::exception("Connection failed.")
}
// There's really no need to output something on successful initialization.
}

~Server() {
// In the destructor, we do the cleanup work.
WSACleanup();
closesocket(Sockfd);
}

// Now the class functionality. Note the return type: reporting the
// error isn't the server's responsibility; it just *detects* the error.
bool send(const std::string& message) {
// Your copying and gets()ing logic is just plain wrong as far as I can tell
// Also, the input text isn't a "buf"fer; that's the low-level concept
// that you're wrapping around.
const int bufsize = 256;
char buf[bufsize];
buf[bufsize - 1] = '\0'; // we will leave this in place.
// Copy the string into the buffer bufsize-1 chars at a time, sending each packet.
// After all, we don't want to limit the size of input messages :)
// We loop while there are (bufsize-1) characters available, and then
// handle the remaining part.
typedef std::string::iterator iter;
iter it = message.begin();
for (; message.end() - it >= bufsize; it += (bufsize - 1) {
std::copy(it, it + (bufsize - 1), buf);
if (send(sockfd, buf, bufsize, 0) == -1) return false;
}
int remainder = message.end() - it; // at most bufsize - 1
std::copy(it, message.end(), buf);
buf[remainder] = '\0';
return (send(sockfd, buf, remainder + 1, 0) != -1);
}
};

// Now you can use it something like:
int main() {
std::string ip;
const unsigned short MYPORT = 1337;
std::cout << "Where would you like to go today?" << std::endl;
std::getline(std::cin, ip);
Server s(ip, MYPORT);
// Later we can add exception handling... for now they will just crash
// the program, but they do it in a well-defined way.
std::string message = "";
while (message != "quit") {
std::cout << "Send as much as you like per line" << std::endl;
std::getline(std::cin, message);
if (!s.send(message)) {
std::cout << "Oops, lost connection to the server." << std::endl;
break; // can't send any more.
}
}
// Cleanup happens automatically here: don't need to write anything.
}


Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this