-
Notifications
You must be signed in to change notification settings - Fork 837
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
unused data #517
Comments
This is not the correct refactoring of this code. Zoom out more. |
Here is the patch
|
This is still not the correct refactoring of the code. I did understand what you suggested but this microoptimization is not the right fix. I was being terse because you sound like a bot or a service expecting free beta testing. Your paper turned up on Lobsters, that was useful context for this and #518: https://lobste.rs/s/u7srss/how_not_structure_your_database_backed#c_syfieu If you'd like invites to join the conversation, send me an email or DM on twitter. Or if you'd like to grab coffee, I'm on the south side most Tuesdays. |
@hyperloop-rails It's been a couple weeks - anyone there? |
First of all, could you point out which part of the patch is wrong. Since I test the function and the output is the same. |
The patch isn't wrong. The patch looks like a bot outputting rules-based fixes rather than a human reading the code, because there's a better solution to this if you look at this function, and another better one if you look at the larger context in the app. So if you have a bot, great, let's talk about your bot on the site and decide what would be useful. But running the bot and dropping micro-optimizations means you're volunteering me to be the human to spend time thinking about them. I'm really happy to see the codebase was useful in your studies, because the time contributors spent on it is providing bonus value at no extra work. When you paste bot output and use that as a metric in your next paper as evidence of your bot's value, you're asking contributors to do new work for you. When it's micro-optimizations with no human judgement or even performance metrics, you're providing an extremely marginal benefit to the codebase that is not outweighed by the volunteer time required to do review it. The problem is not that I don't understand what your patch does or what benefits you claim or what the general class of performance issue is. The problem is that you're not acting like you care about the costs. |
Closed. I'm out of polite things to say about you trying to use me like this. |
I feel so sorry to make you feel bad, and I appreciate a lot for your effort in responding to the issues I posted. Indeed we have a recently published tool (a RubyMine plugin) to help you identify such optimization and do automatic fix, as you can find here: https://hyperloop-rails.github.io/powerstation/, which you can try it on your own Rails project. I'm not intended to make you feel that I'm using you--I apologize sincerely to hurt feelings--but want to improve the performance problem solving in ORM applications. Helping find out both performance-improving and semantics doable patches is one of our target. And your feedback about the costs concern really helps us a lot in our future work of better tools. Anyway, I apologize again for my mis-behavior. |
You didn't hurt my feelings, you repeatedly neglected to identify yourself as researchers so that professional work me and other maintainers into participating in and evaluating your research project without consent. My feelings aren't hurt by these code patches. For the final time, the problem is your misconduct as an academic researcher. |
lobsters/app/models/vote.rb
Lines 70 to 75 in 8db5de9
There is unused data retrieval, only comment_id, vote, reason of Vote is used, so the code can be modified to:
votes = self.where( :user_id => user_id, :comment_id => comment_ids, ).select(:comment_id, vote, reason)
The text was updated successfully, but these errors were encountered: