How much to do in a contructor

I am very new to C++ and am trying to learn how most people do things.

I have made a class and am wondering how much is too much in a constructor and what is considered bad practice. What I want to do with this class is (just for now) create a socket ready to listen on a server.

Here is how I am currently doing it.

Code:

---

class server_socket
{
    public:
        server_socket(short int port)
        {
            fport = port;
            fsock = 0;
            memset(&faddr, 0, sizeof(struct sockaddr_in));

            /* Create the socket */
            create();
            if (!isvalid()){return;}
            bind(fport);
            if (!isvalid()){return;}
            listen();
            if (!isvalid()){return;}
          //After I create the class I also call isvalid() in the function as well
        }
        ~server_socket()
        {
            close(fsock);
        }

        bool isvalid(void)
        {
            return (fsock != -1);
        }

        bool send(const char *);
        int recv(char *);

    private:
        int fsock;
        short int fport;
        struct sockaddr_in faddr;

        bool create(void);
        bool bind(const int port);
        bool listen(void);
        bool accept(int *fsock);
};

---

I also don't know if it is typically a good idea to "return" out of a constructor.

I'm all ears right now because I am trying to write code a standard C++ programmer typically would. Moving from C to C++ has quite a few differences to say the least. Still haven't moved into <string> yet.

If construction fails, it is better to throw an exception, rather than to create an error when the user tries to use the socket. The programmer should not need to call isValid() after creation. Nor should he need to do so before send() and recv().

Otherwise, looks good.

Oh, I forgot how useful that was. I forgot some of my old OO practices from PHP.

You should NOT ever return early from a constructor [1]. If you can not obey that principle, then the code should not be part of the constructor [I'm sure someone will come up with a useful exception to this rule - but the general rule certainly is "do not return before end of construction" (aside from footnote 1)]. Throwing an exception, as King Mir says, would be a plausible alternative to splitting the construction.

In this particular case, I'd say your constructor should set member variables, and a "connect" or "initialize_connection" or whatever you want to call it, should deal with the socket create, bind, listen, etc. You can then also return an error message to say "didn't work", and not have to call "is_valid" from the code using the class.

Also, if bind, listen, create are all functions returning bool, why are you not checking the return value?

[1] Obviously, if there are situations where some argument is optional (e.g. you may pass a NULL or a char */std::string pointer, and if the value is NULL, then there is less work to do, and this work is done last, then you would be allowed to return early after sufficient work besides the copying of the string or whatever the operation would be to deal with the optional parameter).

--
Mats

I'm having a little trouble understanding how I would try/catch when creating a class variable, because the second I do it, the constructor is called.

This doesn't work. I get some errors about braces and this and that.

Code:

---

int main (void)
{
    server_socket ssck;

    try (ssck)
    {
        std::cout << "Socket Creation Successful!" << std::endl;
    }
    catch (err)
    {
        if (err == errcreate)
        {
            cout << "Error at create()" << endl;
        }
    }

    return 0;
}

---

I know I've deviated off of the original topic but we've kind of gone a new direction.

--Edit. Nevermind, I'm a bit rusty on the syntax. I got it though. sorry.

Thnaks again for your suggestions!

Sorry for the double post but I have hit a block. I think I am doing what you guys suggested here but can't quite get it. Obviously the way I have my code currently won't work, but I hope you guys see what I am trying to achieve.

In the contructor I call a private function, which will return a boolean. Inside the constructor, I check that boolean value, and if false, I throw an exception, which is from inside the constructor.
I just am unsure what to put in the try block.

Code:

---

int main (void)
{
    try
    {
        server_socket ssck (80);
    }
    catch (int err)
    {
        if (err == ssck.errcreate)
        {
            std::cout << "Error at create()" << std::endl;
            return -1;
        }
    }

    std::cout << "Socket Creation Successful!" << std::endl;

    return 0;
}

---

Thanks for any help!

That looks almost right. Just move the "Socket Creation Successful!" message inside the try block as well since you don't want that to print when it failed.
I assume you are simply throwing an int?

Code:

---

int main (void)
{
    try
    {
        server_socket ssck (80);
    }
    catch (int err)
    {
        if (err == ssck.errcreate)
        {
            std::cout << "Error at create()" << std::endl;
            return -1;
        }
    }

    std::cout << "Socket Creation Successful!" << std::endl;

    return 0;
}

---

YUCK! GROSS! NO! WRONG! BAD! EVIL!

The purpose of an exception: TO BE INFORMATIVE!

Now, let's step back a bit from this design and consider a good approach by following the advice you've been given:

Code:

---

class socket_error:
  public std::runtime_error
{
  // ???
};

---

Code:

---

class socket_invalid_port:
  public socket_error
{
  // ???
};

---

Code:

---

class server_socket
{
// ???
  server_socket(short int port):
      fport(port)
  {
      // ???
      if(!validate_port())
      {
        throw(socket_invalid_port(port));
      }
  }
// ???
}

---

Code:

---

int main()
{
  try
  {
      server_socket ssck (80);
      // if execution gets here
      // we have a valid socket
      // full stop
      // don't check anything
      // just use the socket
      std::cout << "Socket Creation Successful!" << std::endl;
      return(0);
  }
  catch(const socket_error & error)
  {
      std::cout << error.what() << std::endl;
      return(-1);
  }
}

---

Soma

Thank you phantomotap. I think I will get more descriptive later, but right now, I need to get the simplest version working so I can build off of it. I will most assuredly reference your post when I am ready to expand upon it.

Back to what I had originally, I apologize that I failed to mention the error I was receiving:

Quote:

---

error: ‘ssck’ was not declared in this scope|

---

And yes iMalc, I am just throwing an int.

Code:

---

//Constructor
server_socket::server_socket(short int port)
{
    errcreate = 1;

    fport = port;
    fsock = 0;
    memset(&faddr, 0, sizeof(struct sockaddr_in));

    /* Create the socket */
    if (screate() == true){throw errcreate;}
}

---

I set it to true for testing.

errcreate is defined under public:

Edit: The main thing I am worried about is the scope of the class variable, which is only in the try block.

Quote:

---

Originally Posted by carrotcake1029

Edit: The main thing I am worried about is the scope of the class variable, which is only in the try block.

---

Use a pointer if you want the socket to exist in the scope outside of the try-catch block.

Code:

---


socket * sck;

try {
    sck = new socket(80);
} catch (socket_err & se) {
    // log error
    throw AnotherException;
}

// sck visible here

---

Thank you. Why didn't I think of that? :P

The try block doesn't need to be around object creation only but may be around a larger scope, perhaps even in a function higher up the call stack.

What you do in a constructor depends on your object oriented approach. If you want true RAII then you can init everything in the constructor and throw when/if they fail. One problem here is the example of if you allocate 2 objects on the heap successfully but the third one fails and you throw, you just leaked 2 objects on the heap. std::auto_ptr can help out with this as well as the boost smart pointers so it's not a huge issue if you handle it right.

The other approach is to initialize a class in a two-step process. The general rule here is don't perform any action in a constructor that could possibly fail. Another way of saying this is only perform operations that are guaranteed to succeed. Then you create an Init() function or Create() and it performs all the setup required for the object. Benefits to this approach is you know that an object will be created every time you call the constructor. Downside is it may not be very clear without some code spelunking on how to setup and create the object correctly. I would still call this a form of RAII because you are initializing prior to usage but via a two-step process.

Normally I use the latter approach because it is much simpler and does not require all the try catch blocks every time you create an object and there is no room for error when it comes to leaking memory since the constructor does not allocate any memory. Sometimes this approach has to be used in Win32 code because even though your C++ object exists your actual Win32 object may not which can cause problems.

Inevitably which approach you choose to use is up to you and I cannot say that one is more right or more wrong than the other. In an existing code line I would follow along the lines of what has already been done to maintain consistency.

Quote:

---

Normally I use the latter approach because it is much simpler and does not require all the try catch blocks every time you create an object

---

I adhere very strictly to the former approach, and the only try-catch blocks I have in my project are at COM boundaries. I consider two-stage construction something to be avoided if possible. (And it usually can be avoided.)

If you use two-step process or more, then the object is is a non-usable state after the constructor, so it usually means extra work in making sure you can't use the object before the initialization process is complete or you might invoke undefined behavior.
An ideal constructor will make the object ready for use after it's finished.

I would also say that multistage construction is not a good practice, unless an object can be reused -- that is if it it makes sense to redo the second construction step for the object. For example, if you want you socket to be re-binable to a different port, it makes sense to have a constructor that does not bind the port, and have a function that binds it later. It is still not a bad idea to have a constructor that does take a port. This is similar to the behavior of std::fstream.

Quote:

---

This is similar to the behavior of std::fstream.

---

std::fstream supports both approaches. You can pass the filename to the constructor or you can just create the object and then call open. If you just create the object and never call open the stream is not ready to be used. But if you pass the filename into the constructor and then call open that is just redundant code. Objects that support both types of creation can become confusing but again it is very minimal confusion.

I've seen objects that support one or the other and/or both and as long as the user is clear on how to use the object it usually does not cause any problems. I'm not endorsing one or the other here.

Wrapping a declaration in a try block obviously won't work, because the variable will go out of scope immediately. It is better to catch the exception at a higher level. Or, put the entire function body inside the try block.

Although it's an unusual practice, in some environments the decision has been made that exceptions will not be used -- in such cases, there is really no good alternative to two-phase initialization.

In this particular case, I think the creation/binding/listening of the socket in the constructor is the appropriate method. This is RAII. If the object was constructed, then everything is as expected and the object can be used normally. If any one of those calls fails, the object is inherently unusable and an exception should be thrown.