C++ code review

Started by
39 comments, last by ApochPiQ 13 years, 5 months ago
Hi. I'd like to hear your thoughts on the code on my website -- www.webEbenezer.net. There are two kinds of software on the site -- hand-written and computer-written. This archive has mostly hand-written code, but the file msg_shepherd.hh is output from the C++ Middleware Writer.

There are two interfaces to the C++ Middleware Writer: a web interface and a command line interface. The command line interface is what I'm focusing on here. The command line interface is part of a three-tier architecture. One tier is the C++ Middleware Writer which is closed source. The other two tiers are open source and are in that archive. The middle tier is called the C++ Middleware Writer Ambassador and the final tier is a command line program currently called "direct", but that name is subject to change. Please be specific as far as suggestions. Thanks a lot.

Brian Wood
Ebenezer Enterprises
Advertisement
I can't find where on your website you actually explain what the "C++ Middleware Writer" actually is. Judging by that file "msg_shepherd.hh" it seems to include serialization code generation, but that's about all I can figure out. And I now know the names of three tiers, but not what they're for or what they make up together.

Take a step back. What is this thing you wrote? What is it for? How does one use it? How does it work?
Quote:Original post by Sneftel
I can't find where on your website you actually explain what the "C++ Middleware Writer" actually is. Judging by that file "msg_shepherd.hh" it seems to include serialization code generation, but that's about all I can figure out. And I now know the names of three tiers, but not what they're for or what they make up together.


Since posting some thoughts like that have crossed my mind. Your post helps me get out of a lazy mood and actually do more than just think about it.

Yes, the C++ Middleware Writer writes low-level marshalling/serialization code based on high-level user input. One of the advantages of using the C++ Middleware Writer is that it writes marshalling functions for user defined classes if desired. In competing serialization libraries that job is done by users. If you make changes to class data members, it is on you to be sure you correctly update the associated serialization functions.

As far as the tiers, we used to have two tiers and have over the past seven months added a third. The middle tier, the C++ Middleware Writer Ambassador, holds a connection to the C++ Middleware Writer. The three tier structure has both administrative and performance advantages over the two tier approach we were using. Previously with the two-tier approach, all users needed to have firewall access opened up for them. Now all user requests are funneled through the ambassador and only that process has to have special privileges. In the two-tier approach every request involved establishing a connection with the C++ Middleware Writer and the password had to be sent with each request.

Quote:
Take a step back. What is this thing you wrote? What is it for? How does one use it? How does it work?


It is an on line C++ code generator. It's a binary alternative to SOAP. It is intended to be helpful in developing distributed applications with C++. There are two ways to use it: a web interface and a command line interface. The command line interface is a lot easier to integrate into a build environment than the web interface. Thanks for your comments.

Brian Wood

[Edited by - wood_brian on September 25, 2010 9:26:56 PM]
Quote:
It is an on line C++ code generator. It's a binary alternative to SOAP. It is intended to be helpful in developing distributed applications with C++. There are two ways to use it: a web interface and a command line interface. The command line interface is a lot easier to integrate into a build environment than the web interface. Thanks for your comments.

Ok. So I think I finally understand what you are selling. That isn't clear AT ALL from your website. That mostly stems from the fact that your webpage is just a jumble of text with no real formatting.
* A code formatter like seen here on my blog would help to make your code snippets more readable. As would a non-black-and-white color scheme that emphasizes links and headings better to give a good separation of text.

* Specifying exactly what your tool does would be good. While it is somewhere in your page, and you've stated it here in the forums, it isn't obvious from your site. You need to really redo the "flow" of the page to Introduce->Compare->Try so that people can get all the information they need from the front page. Likewise, I could care less about your changelog when I visit the front page.

* Naming and grouping your links better would help the flow. Having "Web interface to ...." as one of the top links doesn't really do me any good, since I don't even know what I'm looking at yet. Secondly, since I don't know what I'm looking at I also don't know what I'm supposed to do on that page. After a proper introduction to the service, chaning the link to a "Try the web interface here..." would be a better introduction to that page.

* "Userguide! Oh god, now I need more files?" - What are these middle files? You don't even mention them when comparing your tool to Boost. The boost code is explicit, in my code, and readable. Your code "automatically" takes care of things, but until I clicked the "web interface" link, I had no idea I even had to write these other "middle files"(or maybe there were hints, but I still didn't know what they were supposed to look like).

* Comparing to boost? Could you supply the WHOLE pipeline somewhere? I know with the boost code all I need is that one file. With yours I see that it generated a file, but you don't supply how it did so (ie your middle file and the process to get that generated file). This is huge since it immediately seems your tool is more complicated, so the benefits need to outweigh that complication.

* Are your "middle files" more readable than the boost code? not sure they are.
* How can I tailor the output/input? The endianess of my files tends to matter when reading them from different platforms. Boost's version is explicitly written in my code, so I can do those transforms there.
* How can I tailor the output/input? Boost has a bunch of serialization methods, like XML, binary, text. Can I somehow do that with your tool?

* Consider changing the name of your tool "Middleware Writer" and "Middleware Writer Ambasador" sound like "buzzwords" to me. Pick something unique, related to serialization, and less wordy. Actually, the "less wordy" part goes for a lot of your descriptions of your tool. Again, people need to first know what it is you are selling?(serilization) How is it different(automatic)? how do you use it?(online) Don't confuse people by throwing all that information at them at once. Enterprise descriptions like "The command line interface is part of a three-tier architecture." read as "blah blah blah" to most people. However technically correct the statements might be. You need to walk people in piece by piece so they are sold before the complexity hits. Remember, your customers are idiots, and they are always right. If something else seems simpler, they are going to go look at that first.

Lastly, since you've posted about this a lot on the forums, I'd go back and look at all those posts. What questions do people have? How important are those answers? The more important the answers and the dumber sounding questions should be answerable on the front page of your site within a moments look at the page. If I have to search for more than 5-10 seconds to find the simple answers that will sell the product, then you just lost a customer.
Quote:Original post by KulSeran

Ok. So I think I finally understand what you are selling. That isn't clear AT ALL from your website. That mostly stems from the fact that your webpage is just a jumble of text with no real formatting.
* A code formatter like seen here on my blog would help to make your code snippets more readable. As would a non-black-and-white color scheme that emphasizes links and headings better to give a good separation of text.

* Specifying exactly what your tool does would be good. While it is somewhere in your page, and you've stated it here in the forums, it isn't obvious from your site. You need to really redo the "flow" of the page to Introduce->Compare->Try so that people can get all the information they need from the front page. Likewise, I could care less about your changelog when I visit the front page.

I expanded on my introduction. It used to say:

We offer free services aimed at helping those who are developing applications with C++.

Now it says:

We offer free services aimed at helping those who are developing applications with C++. Our primary service is the C++ Middleware Writer. It is an on line code generator that writes low-level C++ marshalling code based on high-level user input.

Thanks for your comments. When I get a chance I'll reply to some of your other comments.

Brian Wood
I've gotten some helpful feedback here and have made a few changes to the site based on that. The feedback so far has focused on the website and documentation. It's fine if you want to comment on that. Code review comments would also be helpful.
What are the benefits of your product over Apache Thrift or Google's Protocol Buffers?
I only had a look at a couple of files but I must say I find your usage of curly braces requires some though for me to see the flow, for example in your loop in main that would take me a bit of effort to to see the flow of logic and to me it looks like you have a catch outside of main,

You have a couple of instances of repeating the same code surrounded by macros
#ifdef BIG_ENDIANS  static ReceiveCompressedBuffer<LeastSignificantFirst>* otherlocalbuf =          new ReceiveCompressedBuffer<LeastSignificantFirst>(4096);#else  static ReceiveCompressedBuffer<MostSignificantFirst>* otherlocalbuf =          new ReceiveCompressedBuffer<MostSignificantFirst>(4096);#endif


I personally would change this to something like the following
#ifdef BIG_ENDIANS    typedef ReceiveCompressedBuffer<LeastSignificantFirst> ReceiveCompressedBuffer_t;#else    typedef ReceiveCompressedBuffer<MostSignificantFirst> ReceiveCompressedBuffer_t;#endif

Your code would then read
  static ReceiveCompressedBuffer_t* otherlocalbuf =  new ReceiveCompressedBuffer_t(4096);

Regarding this code you use a lot of statics and the repeating magic numbers are somewhat questionable.
Quote:Original post by CmpDev


Thank you for your comments. I tend to agree with what you suggested regarding the typedef, but am still thinking about it. The following function is from the same file. I believe in some circles the return statement in the body of the loop would be frowned upon.

int32_tgetConnected(cmwa_config_data& cmwadata){  int32_t sockfd;  if ((sockfd = socket(AF_INET, SOCK_STREAM, 0)) < 0) {    throw failure("socket() failed. errno is ") << errno;  }  // Some platforms don't support getaddrinfo yet, but  // all seem to support gethostbyname still.  sockaddr_in ambaddr;  ambaddr.sin_family = AF_INET;  if (cmwadata.localhost_) {    ambaddr.sin_addr.s_addr = inet_addr( "127.0.0.1" );  } else {    struct hostent* hostEnt;    if ((hostEnt = gethostbyname("www.webEbenezer.net")) == NULL) {      throw failure("gethostbyname() failed. errno is ") << errno;    }    ambaddr.sin_addr = *(struct in_addr *)hostEnt->h_addr_list[0];  }  for (;;) {    int const basePort = 56789;    for (int j = 0; j <= 19; ++j) {      ambaddr.sin_port = htons(basePort + j % 10);      if (connect(sockfd, (struct sockaddr*) &ambaddr, sizeof(ambaddr)) == 0) {        return sockfd;      }      // 111 is connection refused and 113 is no route to host.      printf("connect() failed on port %d. errno is %d\n", basePort + j % 10, errno);    }    sleep(cmwadata.sleepseconds_);  }}


Here's an include file in the archive I mentioned. It is fairly simple.
It has two types that are marshalled between the three processes involved.

#ifndef account_info_hh#define account_info_hh #include <stdint.h>#include <include/loki/flex/flex_string.h>#include <lil_string.hh> class Counter;             class SendCompressedBuffer;template <typename R>class ReceiveCompressedBuffer; struct account_info {  uint32_t accountnumber_;  lil_string password_;    account_info()   {}  template <typename R>  explicit account_info(ReceiveCompressedBuffer<R>* buf);   void CalculateMarshallingSize(Counter& cntr) const;  void SendMemberData(SendCompressedBuffer* buf) const;  void SendTypeNum(SendCompressedBuffer* buf) const;  void   Send(SendCompressedBuffer* buf, bool = false) const  {    SendMemberData(buf);  }}; struct user_info{  uint32_t accountNbr_;   flex_string<char> directory_;   flex_string<char> filename_;    user_info()   {}  template <typename R>  explicit user_info(ReceiveCompressedBuffer<R>* buf);   void CalculateMarshallingSize(Counter& cntr) const;  void SendMemberData(SendCompressedBuffer* buf) const;  void SendTypeNum(SendCompressedBuffer* buf) const;  void   Send(SendCompressedBuffer* buf, bool = false) const  {    SendMemberData(buf);  }}; #endif

This topic is closed to new replies.

Advertisement