Skip to content
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

Thread.run()->Thread.start() #510

Closed
wants to merge 1 commit into from

Conversation

SunnyDay74
Copy link

Hello,
To utilize the multiple-thread of Java, is it good to use Thread.start() instead of Thread.run()?

@vorburger
Copy link
Member

vorburger commented Feb 4, 2019

@SunnyDay74 thank you very much for your interest in and contribution to Fineract/Mifos! I'm an (old) volunteer who got reminded of Mifos/Fineract this week-end at the FOSDEM conference, and I'm taking a moment to help the project clean up some of it's old issues and pull requests etc.

To utilize the multiple-thread of Java, is it good to use Thread.start() instead of Thread.run()?

They don't do the same thing, see the JavaDoc of start() VS run() .. I think you may have stumbled upon a small real issue here: Given that how it is now, using run() it doesn't actually really start a thread anyway, these few lines are unnecessarily complex - I think you could just remove the entire new Thread(new Runnable() { and simply do that line which is inside run() right after the if (without Thread), do you see why, and would you be willing to update this PR with that? We would love to merge a contribution from you as a First-time contributor - and hope to see more from you!

If we don't hear from you within 2 weeks, we will take the liberty to close this PR without merging it, to avoid confusion for future new contributors looking at this repository - hope you understand and that's fair.

Copy link
Member

@vorburger vorburger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment in main conversation

@vishwasbabu
Copy link
Member

@SunnyDay74 : Closing this pr as per @vorburger's comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants