3
\$\begingroup\$

I made this simple code which makes easy HTML tag generation. It is open-source and right now only makes tag names and replaces tags... :/

#include <iostream>
#include <fstream>

std::string makeHtmlTags(std::string tagname, std::string tagcontent) {
    return "<" + tagname + ">" + tagcontent + "</" + tagname + ">";
}

void changeHtmlTag(std::string * tag, std::string newtagname)
{
    if(tag->at(2) != '/')
        *tag = "<" + newtagname + ">";
    else
        *tag = "</" + newtagname + ">";
}

int main()
{
    std::string html = makeHtmlTags("hello", "world");
    std::string tag1 = html.substr(2, 5);
    changeHtmlTag(&tag1, "goodbye");
    changeHtmlTag(&tag1, "/goodbye");
    std::cout << tag1;
}

What can I do to make it more useful?

\$\endgroup\$
4
\$\begingroup\$

OOP(s)!

Right now, the meat of your code is two separate functions, just floating around and minding their own business.

It would be better practice to put these two functions into a utility class. That way, they both exist together and can be easily accessed from the same parent.

I think these methods should be static.

EDIT:

According to Jerry Coffin,

These don't look like member functions to me. In any case, a class that contains only static functions should usually be a namespace instead. – Jerry Coffin


Versatility

It is very, very uncommon for an HTML tag to just be a tag along with inner content; there are almost always attributes.

Right now, your code can only generate something like this:

 <foo>bar</foo>

What about this?

<foo spam="eggs">bar</foo>

To improve you code, I recommend create a class/struct that stores the attribute name and value, like this:

typedef struct {
    std::string name;
    std::string val;
} HTML Attribute

Then, your method could take an array of these and loop through them, adding them to the string each time.

\$\endgroup\$
  • \$\begingroup\$ These don't look like member functions to me. In any case, a class that contains only static functions should usually be a namespace instead. \$\endgroup\$ – Jerry Coffin Jan 6 '16 at 2:13
  • \$\begingroup\$ Thanks! I will make a libhtml library that has these functions and more, but I am still not superb in C++... anyway :) \$\endgroup\$ – Gustavo6046 Jan 6 '16 at 13:33
  • \$\begingroup\$ Btw, @SirPython , I updated the code and now it can make these attributes and more: paste.ubuntu.com/14420534 It took me some time and now I'm proud of myself because I never done such a big code :) \$\endgroup\$ – Gustavo6046 Jan 6 '16 at 14:41
  • \$\begingroup\$ @Gustavo6046 Congrats! If you want, you could post a follow-up question here. \$\endgroup\$ – SirPython Jan 6 '16 at 22:40
  • \$\begingroup\$ @JerryCoffin Thanks! I've reference you in my answer. \$\endgroup\$ – SirPython Jan 6 '16 at 22:42
4
\$\begingroup\$

At least in my opinion, there's quite a bit of room for improvement here. As far as usability goes, probably the most obvious change would be to directly support nesting. HTML and XML are frequently nested fairly deeply, but this doesn't really do anything to support nesting at all.

Looking at the code itself, I'd consider at least a couple of points:

1) You typically want to pass strings by const reference rather than value to avoid copying the string (unless you really need a copy, but you don't here). So:

std::string makeHtmlTags(std::string tagname, std::string tagcontent)

would change to:

std::string makeHtmlTags(std::string const &tagname, std::string const &tagcontent)

I would probably write ChangeHtmlTag to return the new string instead of modifying the existing one:

std::string changeHtmlTag(std::string const &tag, std::string const &newtagname)
{
    if(tag.at(2) != '/')
        return "<" + newtagname + ">";
    return "</" + newtagname + ">";
}

Rather than using .at, I'd also consider just returning the input intact if the input is too short to be a tag, then do the modification if it's long enough to be a tag.

The biggest problem I see, however, is that the code just doesn't seem like it does enough/has enough intelligence to make it obvious that using it gains much over playing with the strings directly.

\$\endgroup\$

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.