gumho / antplanner

This project has been succeeded by http://github.com/gumho/antplanner2
http://antplanner.appspot.com
9 stars 3 forks source link

instructor links to ratemyprofessor.com #77

Open gumho opened 13 years ago

styfle commented 13 years ago

I was going to implement this feature, but I don't know the best way to do it since there there might be multiple professors with the same last name. That means you can't simply link to a URL with the professor's review, so the link would have to bring up a search query with possible choices. Maybe clicking on the name will bring up a dialog box with the results of said query, and the user can select which professor seems correct. I wrote some javascript that will only show UC, Irvine professors when you do a query.

var table = document.getElementById('rmp_table');
var length = table.rows.length;
for (var i=0; i<length; i++) {
    if (table.rows[i].innerHTML.indexOf('sid=1074') === -1) {
        table.deleteRow(i);
        length--; // one less row to check
        i--; // check same row next iteration
    }
}

That can be injected at the end of a request to http://www.ratemyprofessors.com/SearchProfs.jsp?letter=PROF+NAME. What do you think?

styfle commented 13 years ago

I went ahead and implemented this feature and sent a pull request.

gumho commented 13 years ago

Checked out the pull request really briefly since I'm knee deep in work lately and looks awesome! I know early on I toyed around with the RMP site but like you said, it has a pretty questionable architecture that makes it pretty hard to get results easily. I'll take a look this weekend and see if I can work with the click problem then get everything merged into the main branch.

styfle commented 13 years ago

Cool! Also, maybe you can checkout the memcache part. I noticed you used memcache for the other requests but when I tried imitating how you wrote your code in the other functions, the request stopped working properly. Maybe there is something I'm missing.

Let me know if there is another issue you want me to work on. I was thinking about cleaning up instructions/documentation and error messages that were never implemented.

gumho commented 13 years ago

Got the click handling fixed up. Ran into some minor issues with some hover events on the school side so fixing that now. Then once I get the memcached working and tweak some interface elements, a push is in store!

styfle commented 13 years ago

Sounds good. I'll be interested to see what you did to get the click handling fixed since I don't have a lot of experience with JQuery.

gumho commented 13 years ago

It actually didn't require too much. I put in a "return false" at the end of the professor click handler which prevents propogation of all parent elements' click handlers (so the course row's click handler gets canceled out). I think event.stopPropogation() works as well but I've never used it so not sure if there's any side effects.

Another thing, I modified main.js courses that have 2 professors listed would only send one AJAX call. One thing I noticed though is that on IE 7, courses with 2 professors don't show any results. I'm going to investigate this regression later this week before I push live. I have the latest code on staging here: http://release-0-9-8-3.antplanner.appspot.com/

styfle commented 13 years ago

So that's the purpose of return false at the end of those click handlers. I was wondering about that. And good call on the one AJAX request for multiple professors. I just checked it out and it looks good.

One thing I noticed is that my AJAX error function was never formatted properly. It should probably look something like:

profTable.html('<tr><th colspan="6">An error occured!</th></tr><tr><td colspan="6">'+textStatus+' - '+errorThrown+'</td></tr>');
gumho commented 13 years ago

So I fixed the IE bug, the problem was that somehow IE was converting an element into uppercase when code was expecting lowercase.

But...there's another problem, the professor links are hammering the servers hard!! I've been getting some pretty crazy numbers for amount of CPU used - around 23800cpu ms average, which may not sit well with quota reductions from the new AppEngine pricing scheme Google announced yesterday.

Somehow we'll need to find another way around fetching pages from RMP on every request. I'm thinking it might be more practical to periodically start a workorder to index RMP professor ids and save them to the datastore. Then the professors could just link to the pages. Either that or more optimizations will be needed. :(

styfle commented 13 years ago

I noticed the cpu usage as well. I think setting up a cron job that runs once a month could collect all the RMP ratings and then store them in a database. That way, user queries are against the database and not scraping the RMP website every time. It would be a little outdated but I think memcache could make results outdated as well so thats not a huge difference.

Another option is to run the cron job each day but every day grabs a different letter. For example the first day you scrape http://www.ratemyprofessors.com/SelectTeacher.jsp?the_dept=All&sid=1074&orderby=TLName&letter=A and the next day you scrape letter=B. That way you get a consistent, lower cpu usage each day rather than a single, high cpu usage.

Also, from what I could tell the problem was not so much cpu usage as it was cpu time. Meaning RMP is just slow so it has to wait for the response before doing any processing and Google doesn't want you hanging up their servers. I don't know if there is any optimizations that can be done.

I hope that helps.