6
\$\begingroup\$

This is an exercise to use C++, Cevelop IDE & libtidy rather than the HTML Tidy command line tool. The program just reads from standarg input, processes the contents with HTML Tidy then puts the cleaned up version to standard out. Errors and diagnostics go to standard error. Function tidyit takes a std::string and returns one with error messages going to stderr while main handles the standard I/O.

I am using the most current C++17 with no concerns for older versions. The version of Cevelop IDE is 1.7.1. HTML Tidy provides both a command line tool and a C library, libtidy. The code is based upon the C code in the Sample Program.

I am looking for improvements given it is in C++ rather than the original in C.

// tidyit.cpp
#include <sstream>
#include <iostream>
#include <string>

const std::string tidyit(const std::string a_input);           // Change to auto in cevelop 1.8.

int main(int argc, char **argv) {
   std::stringstream input;
   input << std::cin.rdbuf();
   std::cout << tidyit(input.str());
   }

#include <tidy/tidy.h>
#include <tidy/buffio.h>

bool is_severe_error(int a_code){return (a_code<0);}           // Change to auto in cevelop 1.8.

const std::string tidyit(const std::string a_input) {          // Change to auto in cevelop 1.8.
   TidyBuffer errbuf = {0}, outbuf = {0};
   TidyDoc tdoc = tidyCreate();
   tidyOptSetInt(tdoc, TidyIndentContent, yes);
   tidyOptSetBool(tdoc, TidyXhtmlOut, yes);
   tidyOptSetBool(tdoc, TidyForceOutput,yes);
   int rc{-1};                                                 // Change to auto in cevelop 1.8.
   if (is_severe_error(rc=tidySetErrorBuffer(tdoc, &errbuf))
         || is_severe_error(rc=tidyParseString(tdoc, a_input.c_str()))
         || is_severe_error(rc=tidyCleanAndRepair(tdoc))
         || is_severe_error(rc=tidyRunDiagnostics(tdoc))
         || is_severe_error(rc=tidySaveBuffer(tdoc,&outbuf))){
      std::cerr << "tidyit-A severe error (" << rc << ") occurred.\n";
      }
   else {
      if(errbuf.bp)
         std::cerr << "tidyit-Diagnostics:\n\n" << errbuf.bp << "\n";
      }
   const std::string outstr{(char*)(outbuf.bp)};
   tidyBufFree(&outbuf);
   tidyBufFree(&errbuf);
   tidyRelease(tdoc);
   return outstr;
   }

Cevelop C++ IDE

Version: 1.7.1-201704211123

Compiler info

Command: clang++-4.0

All options: -std=c++1z -O0 -g3 -Wall -c -fmessage-length=0 -Wno-attributes

Library info

Libraries: tidy

Updated code available at: CodeReview - HTML Tidy Sample Program in C++17 (pastebin)

\$\endgroup\$
  • \$\begingroup\$ I added language tag so that it would be visible on C++ questions list. \$\endgroup\$ – Incomputable Jun 6 '17 at 6:40
  • 1
    \$\begingroup\$ Never heard of Cevelop, which compiler does it use? \$\endgroup\$ – yuri Jun 6 '17 at 8:10
  • 1
    \$\begingroup\$ That is nearly unreadable. Have you though about using some vertical white space to help the reader? \$\endgroup\$ – Martin York Jun 6 '17 at 16:26
  • 1
    \$\begingroup\$ While it is easier to take screenshots, it would be much more readable (and searchable) if you had posted your configuration/environment settings in text form. It wouldn't be that hard to transcribe the Cevelop version number, and to copy-paste the Clang command-line arguments. (Images of things that should be text is one of my pet peeves.) \$\endgroup\$ – Cody Gray Jun 8 '17 at 8:58
  • 1
    \$\begingroup\$ what's the ;][4]][4] in tidyit? A C++19 operator? \$\endgroup\$ – jvb Jun 13 '17 at 12:12
6
\$\begingroup\$

Observation.

Though you are compiling this with a C++ compiler and using a couple of C++ classes this is still C code. The style is C and does not reflect the standard idioms of C++.

We refer to code written like this as "C with classes".

Exception Safety

When you see code that looks like:

<Create Resource>

<Do Stuff>

<Destroy Resource>

This is not exception safe. If the code in <Do Stuff> throws an exception then <Destroy Resource> will not be executed. Now you think none of this code throws exceptions and Tidy is a C-library so it will not. But I bet Tidy allows you to register callbacks. These callbacks can throw exceptions and eventually you will add one to the code.

Example:

   TidyDoc tdoc = tidyCreate();
   // Stuff
   tidyRelease(tdoc);

The correct way to do this is to use the RAII idiom. This means creating a class where the constructor allocates the resource and the destructor deallocates the resource.

Since the destructor of an object is automatically called when an object goes out of scope (this includes when the stack is unwound by an exception being thrown) this makes resource allocation/deallocation exception safe.

class TidyDocCPP
{
    TidyDoc tdoc;
    public:
        TidyDocCPP() {
            tdoc = tidyCreate();
        }
        ~TidyDocCPP() {
            tidyRelease(tdoc);
        }
        // This is used to convert an object back to the C form
        // so it can be used in the normal C tidy functions.
        operator TidyDoc() const {
            return tdoc;
        }

        // Need to do this to satisfy rule of 5
        // Don't worry about this section yet.
        TidyDocCPP(TidyDocCPP const&)            = delete;
        TidyDocCPP(TidyDocCPP&&)                 = delete;
        TidyDocCPP& operator=(TidyDocCPP const&) = delete;
        TidyDocCPP& operator=(TidyDocCPP&&)      = delete;
};

Now it can be used like this:

{
    TidyDocCPP    doc;
    tidyOptSetInt(doc, TidyIndentContent, yes); // doc is auto converted
}
// doc goes out of scope here.
// This calls the destructor and releases the resource.

Design

You read the input into a string. Then pass this string to tidyit().

The first issue I have with this is that you pass the string by value. This basically makes a copy of the whole string. Given that modern web pages are huge this is a terrible idea. The main page of stackoverflow.com is 250K.

You should pass immutable strings by const reference (this prevents the copy but still allows full read access).

But even reading the data into a string is incorrect. You have a stream of data (the input) you should use Tidy stream reading capabilities.

Rather than using

 tidyParseString(tdoc, a_input.c_str()

You should use:

 tidyParseStdin(doc);

If you want to be more generic:

 tidyParseSource(doc, inputSource);
 // You will need to build a wrapper aournd TidyInputSource
 // http://tidy.sourceforge.net/docs/api/group__IO.html#g86fcc3c86bd63b26a559938bc38d34bb

Vertical Space

Add vertical space to make your code readable. The current way it is all jammed together makes it highly unreadable.

Brace Alignment

Use one of the standard brae alignment strategies. Yours does not match any standard I have seen.

// K&R Style.  Very common
if () {
}

// Allman.     Very common
if ()
{
}

What you are using seems to be a mix of K&R and Ratliff Style

// Ratliff     Very uncommon
if () {
    }

I have never seen it outside this wikipedia page: https://en.wikipedia.org/wiki/Indent_style

Pick one and use it consistently.

\$\endgroup\$
  • \$\begingroup\$ Ratliff Style has been around for at least four decades. Used by Brian W. Kernighan in Software Tools amazon.com/Software-Tools-Brian-W-Kernighan/dp/020103669X \$\endgroup\$ – CW Holeman II Apr 3 '18 at 21:22
  • \$\begingroup\$ @C.W.HolemanII My statement stands. Its very uncommon. Your reference book seem to be out of print and the style never caught on with developers. BUT my main point (which you missed) is that he was using two different styles. K&R and Ratliff Style. My comment was pick one and stick to it. Though I personally would prefer the code to be in one of the standard styles I will live with uncommon as long as it is consistent. \$\endgroup\$ – Martin York Apr 4 '18 at 15:18
  • \$\begingroup\$ 'I have never seen it outside this wikipedia page' -- I was just giving a reference since you had never seen it. 'Pick one and use it consistently'. I thought you were just emphasizing that one should stick to the choice whatever it may be rather than that you found inconsistency. I do not see any inconsistency in the sample code. \$\endgroup\$ – CW Holeman II Apr 4 '18 at 17:19
  • \$\begingroup\$ @C.W.HolemanII Fair; looking at the code again neither do I. I did the review a long time ago. Misremembering (or being lazy this morning). I am still going to stick with Its very uncommon. But if you are consistent you can use it. But I personally think it is better to use a more common style (but thats an opinion and you can take it or leave it). Though we are probably of the same era that's not a book I have (but I think it shows our age that we still use books :-) ) \$\endgroup\$ – Martin York Apr 4 '18 at 17:38
  • \$\begingroup\$ Oldest computer non-textbook in my collection. It may have been the my first exposure to braces being used for blocks (vs endif/begin/end). The font in the code is simple. For me it is the simplest and cleanest style. The banner style simply says that the next item at the same level of indent is the next item. There is no need to skip over an ending brace. Other styles require me to do more visual processing. This is not the only area of software where popularity has won on merit. In a perfect world... \$\endgroup\$ – CW Holeman II Apr 4 '18 at 18:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.