1
\$\begingroup\$

I wrote a code to parse an HTML knock-off from HackerRank, and I would like a review. Mostly I hope to get better at STL and code style. But any tips would be very welcome.

Sample input that I would read from cin to parse and answer queries:

4 3
<tag1 value = "HelloWorld">
<tag2 name = "Name1">
</tag2>
</tag1>
tag1.tag2~name
tag1~name
tag1~value

My code:

#include <iostream>
#include <vector>
#include <map>
#include <algorithm>
#include <iterator>

int main() {
    std::string scope, cin_buffer, name;
    std::string::iterator strg_it;
    int lines, querrys;
    std::map<std::string,std::string> variables;

    std::cin >> cin_buffer;
    lines = atoi(cin_buffer.c_str());
    std::cin >> cin_buffer;
    querrys = atoi(cin_buffer.c_str());

    for (int lines_processed=0; lines_processed<lines; ++lines_processed){
        std::cin >> std::ws;
        while (std::cin.peek()!= '\n'){
            std::cin >> cin_buffer;
            if (cin_buffer.at(0)=='<' && cin_buffer.at(1)!='/'){
                strg_it = remove_if( begin(cin_buffer), end(cin_buffer), 
                                    [](unsigned char c){return c=='<' || c=='>';});
                cin_buffer.erase(strg_it, end(cin_buffer));
                if (scope.length()>0){
                    scope += "." + cin_buffer;
                }else{
                    scope = cin_buffer;
                } 
            }else if (cin_buffer.at(0)=='<' && cin_buffer.at(1)=='/'){
                int length_to_erase = (scope.length())-(cin_buffer.length()-3);
                scope.erase(length_to_erase);
                if (scope.back()=='.'){
                    scope.erase(scope.length()-1);
                }
            }else{
                name = scope + "~" + cin_buffer; 
                std::cin >> cin_buffer; 
                std::cin >> cin_buffer; 
                strg_it = remove_if( begin(cin_buffer), end(cin_buffer), 
                                    [](unsigned char c){return c=='"' || c=='>';} );
                cin_buffer.erase(strg_it, end(cin_buffer));
                variables[name]=cin_buffer;
            }
        }
    }

    for (int querry_number=0; querry_number<querrys; ++querry_number){
        std::cin >> cin_buffer;
        if (variables.find(cin_buffer)!=end(variables)){
            std::cout << variables[cin_buffer] << std::endl;
        }else{
            std::cout << "Not Found!" << std::endl;
        }
    }
    return 0;
}
\$\endgroup\$
1
1
\$\begingroup\$

It is good to see that you've avoided the use of using namespace std;.

Coding

Declare variables as close to their initial use as possible, and prefer construction of that initial value to default constructing an object then assigning the initial value. lines and querrys can be declared when they are assigned:

auto lines = atoi(cin_buffer.c_str());
// ...
auto querrys = atoi(cin_buffer.c_str());

name can be declared where it is used. strg_it can also be declared (in both places) as auto. This can help the compiler improve the code by letting it know the value isn't needed after the erase call.

auto strg_it = remove_if( begin(cin_buffer), end(cin_buffer), 
                    [](unsigned char c){return c=='<' || c=='>';});
cin_buffer.erase(strg_it, end(cin_buffer));

Alternatively, when using the erase-remove idiom, you can put the erase and remove in one statement

    cin_buffer.erase(remove_if( begin(cin_buffer), end(cin_buffer), [](unsigned char c){return c=='<' || c=='>';}), end(cin_buffer));

and not need the variable at all.

You have no error checking on your inputs. You should check for failed input, non-integer conversions from atoi, etc. One alternative (since you have a std::string) is to use std::stoi, which will throw an exception if there is an error during conversion.

cin_buffer.at(0) will throw with an empty string, and cin_buffer.at(1) will throw an exception if the string only has one character in it.

variables[name]=cin_buffer; will replace any existing value for name if one already exists. This may be the expected behavior.

In your final search loop, store the result of find in a variable to avoid having to look it up again.

auto it = variables.find(cin_buffer);
if (it != end(variables)) {
    std::cout << it->second << `\n`;
} else

You should avoid using std::endl unless absolutely necessary. Since it flushes the output, there can be a considerable performance hit when using it.

Readability

You should put more spaces in your code, rather than jamming all those characters together. Put spaces around binary operators

for (int lines_processed = 0; lines_processed < lines; ++lines_processed) {

which also makes it clearer that lines_processed < lines is an expression and not part of a possible template lines_processed<lines>.

The parentheses around (scope.length()) are unnecessary.

Spelling is important. Variable names should be spelled correctly to make it easier to read and easier to search for. querrys should be spelled queries.

\$\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.