1
\$\begingroup\$

I am trying to write industry standard code.
https://www.quora.com/How-do-I-follow-a-user-on-Spoj-for-solving-problems-Refer-Details
Someone gave me this A2A.
And I wrote this code for it

//compile with g++ -std=c++11
#include <bits/stdc++.h>
using namespace std;
struct PROBLEM
{
    string name;
    string links;
    int users;
    double points;
};
bool compare (PROBLEM & p1 , PROBLEM & p2)
{
    return p1.users < p2.users;
}
void savePage(string & username)
{
    string url = "http://www.spoj.com/users/" + username +"/";
    string cmd = "curl " + url + " >" + username + ".txt"; 
    system (cmd.c_str());
}
bool checkRegex(string & str)
{
    return regex_match (str , std::regex("^\\s{0,}<td\\salign.{0,}$"));
}
bool checkRegexProb(string & str)
{
    return regex_match(str , std::regex("^\\s{0,}<td\\sclass=\"t.{0,}$"));
}
int getNum(string & str)
{
    int i;
    string temp;
    for (i = 0 ; str[i] != '>' ; i++);
    i++;    
    for(;str[i] != '<' ; i++)
        temp += str[i];
    return stoi(temp);
}
int getUsers(string & str)
{
    string url = "http://www.spoj.com/ranks/" + str + "/";
    string cmd = "curl " +url + " >" + str +".txt";
    system(cmd.c_str());
    string filename = str + ".txt";
    int Count = 0;
    ifstream myfile(filename.c_str());
    if (myfile)  
    {
        string line;
        while (getline( myfile, line ))  
        {
            if (checkRegexProb(line) )
            {
                Count ++;
                if(Count == 2)
                {
                    myfile.close();
                    string deleteFile = "rm -r " + filename;
                    system(deleteFile.c_str());  
                    return getNum(line);
                }           
            }   
        }
    }   
}
vector<PROBLEM> readFile(string & username)
{
    std::vector<PROBLEM> v;
    string filename = username + ".txt";
    ifstream myfile(filename.c_str());
    if (myfile)  
    {
        string line;
        while (getline( myfile, line ))  
        {
            if (checkRegex(line) )
            {
                string temp;
                size_t found = line.find("/status/");
                for(size_t i = found + 8 ; line[i]!=',' ; i++)
                    temp += line[i];
                if(temp != "")
                {
                    PROBLEM p;
                    p.name = temp;
                    p.users = getUsers(p.name);
                    while(p.users == 0)
                        p.users = getUsers(p.name); // bug -> curl gives empty response
                    v.push_back(p);
                }   
            }
        }
        myfile.close();
    }
    return v;

}
void makeLink(std::vector<PROBLEM> & v)
{
    for(int i = 0 ; i < v.size() ; i++)
        v[i].points = 80.0/(40.0+ v[i].users),
        v[i].links = "<a href =\" http:://www.spoj.com/problems/" + v[i].name +"\"> " + v[i].name +" "+ to_string(v[i].points) +"</a> <br>";
}
string makeHtml(std::vector<PROBLEM> & v)
{
    string list ;
    for (int i = 0 ; i < v.size() ; i++ )
        list += v[i].links;
    string HTML = 
    "<html><body>"+ list +"</body></html>";
    return HTML;
}
int main(int argc, char const *argv[])
{
    string username ;
    cin >> username ;
    savePage(username) ;
    std::vector<PROBLEM> v = readFile(username);
    sort(v.rbegin(), v.rend(),compare);
    makeLink(v);
    std::ofstream outfile ((username + ".html").c_str());
    outfile <<makeHtml(v);
    system(("rm -f "+ username + ".txt").c_str());
    system(("gnome-open " + username + ".html").c_str());   
    return 0;
}

How can I improve it so that it matches industry standard?

Improve this question
\$\endgroup\$

2 Answers 2

Reset to default
2
\$\begingroup\$

A few more possibilities to consider:

pre-compille your regular expressions

Compiling a regular expression is (potentially) a somewhat expensive operation. In this case, the regular expressions remain constant throughout the program, so you might as well compile them once, and use the compiled regular expression after that:

bool checkRegex(string & str)
{
    static std::regex pattern{"^\\s{0,}<td\\salign.{0,}$"};

    return regex_match (str , pattern);
}

use operator overloading where sensible

For example, your compare function:

bool compare (PROBLEM & p1 , PROBLEM & p2)
{
    return p1.users < p2.users;
}

...really makes more sense as an overload of operator<:

bool operator<(PROBLEM & p1 , PROBLEM & p2)
{
    return p1.users < p2.users;
}

const correctness

Taking the operator above a step further, we don't want x < y to modify either x or y, so we should pass the operands by const reference:

bool operator<(PROBLEM const &p1 , PROBLEM const &p2)
{
    return p1.users < p2.users;
}

This way, when we sort our vector of Problem objects, we don't need to explicitly specify how to compare them:

sort(v.rbegin(), v.rend());

Use the force library, Luke

In addition to a command line utility, cURL is available as a library that you can use in your program. There's also a cURLpp library that provides a C++ wrapper around libcurl, which you may prefer. Either can help you write code that's quite a bit simpler and more readable.

Work with the library, not against it

Right now, you have (for example) a readFile to read the contents of a file into a vector of PROBLEM. That's not really how the standard library is meant to work though. To work optimally with it, you really just want to overload operator>> to read one PROBLEM from the file. Once you have that, you can (for example) use stream iterators to read from a file into a vector.

class Problem { 
    // ...

    friend std::istream &operator>>(std::istream &is, Problem &p) { 
        // code to read one Problem from the stream goes here
        return is;
    }
};

Then we can initialize our vector from a pair of iterators:

std::ifstream in("input.txt");
std::vector<Problem> v{ std::istream_iterator<Problem>(in), {} };
Improve this answer
\$\endgroup\$
1
\$\begingroup\$

Avoid system

It's not recommended to use system, ever, as it can easily lead to problems such as privilege escalation. It's hard to defend using system, so don't.

Don't using namespace std;

This is considered a bad practice.

Always close resources

In the getUsers function, myfile is probably not always closed, because the only code snippet that closes it is buried deep within nested conditionals. Reorganize the code to make sure the file always gets closed.

Naming

Capitalized names are commonly used for classes, not for variables. The Count variable stands out like that, it would be better to lowercase it.

Formatting

Do add empty lines between functions. Also consider adding some between statements inside functions, to visually emphasize blocks of code that are closely related.

It's recommended to use braces with all if/for/while statements, even when the body is a single line. This is especially bad:

for(int i = 0 ; i < v.size() ; i++)
    v[i].points = 80.0/(40.0+ v[i].users),
    v[i].links = "<a href =\" http:://www.spoj.com/problems/" + v[i].name +"\"> " + v[i].name +" "+ to_string(v[i].points) +"</a> <br>";

It's very easy to make an oversight and break this code, with that comma at the end of the second line. Just use braces:

for(int i = 0 ; i < v.size() ; i++) {
    v[i].points = 80.0/(40.0+ v[i].users);
    v[i].links = "<a href =\" http:://www.spoj.com/problems/" + v[i].name +"\"> " + v[i].name +" "+ to_string(v[i].points) +"</a> <br>";
}
Improve this answer
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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