Code review

Discussion in 'C++' started by Ebenezer, Jan 6, 2011.

  1. Ebenezer

    Ebenezer Guest

    I'd like to request comments on the code in the archive on
    this page -- http://webEbenezer.net/build_integration.html .
    As you may know, I'm working on an on line code generator
    that writes C++ marshalling code based on user input. The
    code generator is closed source, but the interface to the code
    generator is open source. The code in the archive mentioned
    above is the interface code. I use a three-tier architecture
    as follows:

    C++ Middleware Writer (CMW) -- this is a server ... closed source

    C++ Middleware Writer Ambassador (CMWA) -- this is a server ... open
    source

    "direct" program -- this program runs once and exits ... open source

    The two open source tiers are in the archive along with supporting
    files. One of the files in the archive -- msg_shepherd.hh -- is
    output from the CMW. That file is based on another file in the
    archive called direct.mdl and some include files.

    One thing that I expect will be brought up is that the software
    mixes naming conventions. I haven't picked one and been
    consistent with it as is sometimes advised here. I agree with
    that advice for the most part, but have been lazy about following
    it so far.

    There's a mediateResponse function in the CMWA that
    may result in comments. Some knowledge of the CMW
    is needed --- if the returned transaction number is zero, the
    value of the request result (reqResult) will be false. I can
    only think of oddities like sun storms that would cause
    this to be violated. But it wouldn't be difficult to add code
    that checks if returnedTransactionNbr == 0 and reqResult
    is true and throws an exception.

    In the "direct" program the function that connects to the
    ambassador uses 127.0.0.1. That needs to be worked
    on.

    The software in the archive has been tested on Linux and Windows.
    I'd appreciate advice on improving the existing code as well as
    suggestions of new functionality. Thanks in advance.


    Brian Wood
    Ebenezer Enterprises
    http://webEbenezer.net
    (651) 251-9384
     
    Ebenezer, Jan 6, 2011
    #1
    1. Advertising

  2. Ebenezer

    Ebenezer Guest

    On Jan 6, 8:11 am, Leigh Johnston <> wrote:
    > On 06/01/2011 04:27, Ebenezer wrote:
    >
    >
    >
    >
    >
    > > I'd like to request comments on the code in the archive on
    > > this page --http://webEbenezer.net/build_integration.html.
    > > As you may know, I'm working on an on line code generator
    > > that writes C++ marshalling code based on user input.  The
    > > code generator is closed source, but the interface to the code
    > > generator is open source.  The code in the archive mentioned
    > > above is the interface code.   I use a three-tier architecture
    > > as follows:

    >
    > > C++ Middleware Writer (CMW)  --  this is a server ... closed source

    >
    > > C++ Middleware Writer Ambassador (CMWA) -- this is a server ... open
    > > source

    >
    > > "direct" program  -- this program runs once and exits ... open source

    >
    > > The two open source tiers are in the archive along with supporting
    > > files.  One of the files in the archive -- msg_shepherd.hh -- is
    > > output from the CMW.  That file is based on another file in the
    > > archive called direct.mdl and some include files.

    >
    > > One thing that I expect will be brought up is that the software
    > > mixes naming conventions.  I haven't picked one and been
    > > consistent with it as is sometimes advised here.  I agree with
    > > that advice for the most part, but have been lazy about following
    > > it so far.

    >
    > > There's a mediateResponse function in the CMWA that
    > > may result in comments.  Some knowledge of the CMW
    > > is needed --- if the returned transaction number is zero, the
    > > value of the request result (reqResult) will be false.  I can
    > > only think of oddities like sun storms that would cause
    > > this to be violated.  But it wouldn't be difficult to add code
    > > that checks if returnedTransactionNbr == 0 and reqResult
    > > is true and throws an exception.

    >
    > > In the "direct" program the function that connects to the
    > > ambassador uses 127.0.0.1.  That needs to be worked
    > > on.

    >
    > > The software in the archive has been tested on Linux and Windows.
    > > I'd appreciate advice on improving the existing code as well as
    > > suggestions of new functionality.   Thanks in advance.

    >
    > > Brian Wood
    > > Ebenezer Enterprises
    > >http://webEbenezer.net
    > > (651) 251-9384

    >
    > Why do you think anyone would spend time reviewing all your code for
    > free?  Reviewing a small class for free is probably not asking too much
    > but not entire an archive containing multiple non-trivial source files.
    >


    The way I see it, some people may not want to review any of it,
    some may review a little of it and others may choose to review a
    lot of it. I'll take what I can get. People thinking about
    using the CMW may be willing to put in some effort to help me
    improve it.
     
    Ebenezer, Jan 6, 2011
    #2
    1. Advertising

  3. Ebenezer

    Ebenezer Guest

    On Jan 6, 9:58 am, Ebenezer <> wrote:
    > On Jan 6, 8:11 am, Leigh Johnston <> wrote:
    >
    >
    >
    > > Why do you think anyone would spend time reviewing all your code for
    > > free?  Reviewing a small class for free is probably not asking too much
    > > but not entire an archive containing multiple non-trivial source files.

    >
    > The way I see it, some people may not want to review any of it,
    > some may review a little of it and others may choose to review a
    > lot of it.  I'll take what I can get.  People thinking about
    > using the CMW may be willing to put in some effort to help me
    > improve it.


    I have a specific question that I've come across recently. It has to
    with exceptions and control flow. This is from the CMWA.

    cmwAmbassador::cmwAmbassador(char const* configfile) :
    sendbuf(200000), buf(40000),

    localsendbuf(4096),
    #ifdef BIG_ENDIANS

    byteOrder(most_significant_first),
    #else

    byteOrder(least_significant_first),
    #endif

    configData(configfile)
    {
    fd_set master; // master file descriptor list
    fd_set read_fds; // temp file descriptor list for select()
    FD_ZERO(&master);
    FD_ZERO(&read_fds);

    sock_type CMWfd = login();
    sock_type listener = serverPrep(configData.portnumber);
    // Add the listener to the master set
    FD_SET(CMWfd, &master);
    FD_SET(listener, &master);
    #undef max // for Windows
    int32_t fdmax = std::max(listener, CMWfd);

    for (;;) {
    read_fds = master;
    if (select(fdmax + 1, &read_fds, NULL, NULL, NULL) == -1) {
    if (EINTR == errno) {
    continue;
    }
    throw failure("select() failed with errno of ") << errno;
    }

    for (int32_t sock = 0; sock <= fdmax; ++sock) {
    if (FD_ISSET(sock, &read_fds)) {
    if (sock == CMWfd) {
    try {
    if (!buf.GotPacket()) {
    continue;
    }
    } catch(eof const& ex) {
    closeSocket(CMWfd);
    FD_CLR(CMWfd, &master);
    flex_string<char> errorMsg = "CMW crashed before your
    request was processed.";
    transactions_t::iterator itr =
    pendingTransactions.begin();
    transactions_t::iterator endit =
    pendingTransactions.end();
    for (; itr != endit; ++itr) {
    localsendbuf.sock_ = (*itr).second.sock;
    msg_shepherd::Send(localsendbuf, false, errorMsg);
    closeSocket((*itr).second.sock);
    FD_CLR((*itr).second.sock, &master);
    }
    pendingTransactions.clear();

    CMWfd = login();
    fdmax = std::max(listener, CMWfd);
    FD_SET(CMWfd, &master);
    continue; // This line could go away if I
    // move the code below up into the try.
    }
    int32_t localsock = mediateResponse();
    if (localsock != 0) {
    closeSocket(localsock);
    FD_CLR(localsock, &master);
    }
    } else {
    ....


    That's the way I have it currently. I've thought about changing it
    to the following, picking up from the last for statement in the above:

    for (int32_t sock = 0; sock <= fdmax; ++sock) {
    if (FD_ISSET(sock, &read_fds)) {
    if (sock == CMWfd) {
    try {
    if (!buf.GotPacket()) {
    continue;
    }
    int32_t localsock = mediateResponse();
    if (localsock != 0) {
    closeSocket(localsock);
    FD_CLR(localsock, &master);
    }
    } catch(eof const& ex) {
    closeSocket(CMWfd);
    FD_CLR(CMWfd, &master);
    flex_string<char> errorMsg = "CMW crashed before your
    request was processed.";
    transactions_t::iterator itr =
    pendingTransactions.begin();
    transactions_t::iterator endit =
    pendingTransactions.end();
    for (; itr != endit; ++itr) {
    localsendbuf.sock_ = (*itr).second.sock;
    msg_shepherd::Send(localsendbuf, false, errorMsg);
    closeSocket((*itr).second.sock);
    FD_CLR((*itr).second.sock, &master);
    }
    pendingTransactions.clear();

    CMWfd = login();
    fdmax = std::max(listener, CMWfd);
    FD_SET(CMWfd, &master);
    }
    } else {
    ....


    That way the continue statement in the catch block goes away,
    but I don't like having to move the call to mediateResponse from
    outside the try to inside of it in this form. That function isn't
    supposed to throw eof exceptions. Any advice on this?
     
    Ebenezer, Jan 10, 2011
    #3
  4. Ebenezer

    Ebenezer Guest

    I'm posting the complete constructor this time. I've reworked
    it since the previous post. The primary difference is pulling
    out the check for activity from the CMW to be certain that is
    always checked first. This version eliminates two of the
    continue statements that were in the previous version.
    I've added a comment to the post near the code where I have
    a question. The question follows below.


    cmwAmbassador::cmwAmbassador(char const* configfile) :
    sendbuf(200000), buf(40000),
    localsendbuf(4096),
    #ifdef BIG_ENDIANS
    byteOrder(most_significant_first),
    #else
    byteOrder(least_significant_first),
    #endif

    configData(configfile)
    {
    fd_set master; // master file descriptor list
    fd_set read_fds; // temp file descriptor list for select()
    FD_ZERO(&master);
    FD_ZERO(&read_fds);

    sock_type CMWfd = login();
    sock_type listener = serverPrep(configData.portnumber);
    // Add the listener to the master set
    FD_SET(CMWfd, &master);
    FD_SET(listener, &master);
    #undef max // for Windows
    int32_t fdmax = std::max(listener, CMWfd);

    for (;;) {
    read_fds = master;
    if (select(fdmax + 1, &read_fds, NULL, NULL, NULL) == -1) {
    if (EINTR == errno) {
    continue;
    }
    throw failure("select() failed with errno of ") << errno;
    }

    if (FD_ISSET(CMWfd, &read_fds)) {
    try {
    if (buf.GotPacket()) {
    int32_t localsock = mediateResponse();
    if (localsock != 0) {
    closeSocket(localsock);
    FD_CLR(localsock, &master);
    }
    }
    } catch(eof const& ex) {
    closeSocket(CMWfd);
    FD_CLR(CMWfd, &master);
    flex_string<char> errorMsg = "CMW crashed before your request
    was processed.";
    transactions_t::iterator itr = pendingTransactions.begin();
    transactions_t::iterator endit = pendingTransactions.end();
    for (; itr != endit; ++itr) {
    localsendbuf.sock_ = (*itr).second.sock;
    msg_shepherd::Send(localsendbuf, false, errorMsg);
    closeSocket((*itr).second.sock);
    FD_CLR((*itr).second.sock, &master);
    }
    pendingTransactions.clear();

    CMWfd = login(); // question relates to
    this line and the next two.
    FD_SET(CMWfd, &master);
    fdmax = std::max(listener, CMWfd);
    }
    }

    for (int32_t sock = 0; sock <= fdmax; ++sock) {
    if (FD_ISSET(sock, &read_fds)) {
    if (sock == listener) {
    int newsock;
    sockaddr_in amb_addr;
    socklen_t amblen = sizeof(amb_addr);
    if ((newsock = accept(listener, (sockaddr*) &amb_addr,
    &amblen)) < 0) {
    throw failure("accept() failed with errno of ") << errno;
    }
    FD_SET(newsock, &master);
    if (newsock > fdmax) {
    fdmax = newsock;
    }
    PersistentWrite(newsock, &byteOrder, 1);
    } else {
    if (sock != CMWfd) {
    if (!mediateRequest(sock)) {
    closeSocket(sock);
    FD_CLR(sock, &master);
    }
    }
    }
    }
    }
    }
    }


    In that catch block I'm calling login() which can throw
    so the ambassador could be abruptly terminated. I don't think
    there's any other option though but to attempt to reestablish
    the connection with the CMW though. Would you suggest adding
    logging calls before and after the call to login? Do you call
    functions in catch blocks that can throw?
     
    Ebenezer, Jan 14, 2011
    #4
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Volodymyr Sadovyy

    Code write \ code review productivity

    Volodymyr Sadovyy, Apr 23, 2004, in forum: Java
    Replies:
    8
    Views:
    798
    Roedy Green
    Apr 25, 2004
  2. Otto Wyss
    Replies:
    5
    Views:
    460
    Robert Vazan
    Sep 7, 2003
  3. andrew blah
    Replies:
    6
    Views:
    374
    andrew blah
    Oct 17, 2004
  4. Josiah Carlson
    Replies:
    1
    Views:
    364
    Andrew Clover
    Oct 13, 2004
  5. www
    Replies:
    51
    Views:
    1,518
Loading...

Share This Page