-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Solve UVa problems #29
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey fatosmorina, thanks for the PR. Have some overall comments that affect all files.
Once these are answered I'll review more in depth.
UVa/BasicRemains.java
Outdated
} | ||
} | ||
|
||
static BufferedReader in; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and in other files: global variables should go at the top of the class.
UVa/BasicRemains.java
Outdated
} | ||
|
||
public static void main(String[] args) { | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why catch, print, and exit? If there is an error this will happen anyways, no need for the try/catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned above, I simply used a template and did not make any changes in it.
UVa/BasicRemains.java
Outdated
public static void main(String[] args) { | ||
try { | ||
solve(); | ||
} catch (Throwable e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why catch a Throwable instead of an exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simply used a template and did not make any changes in it.
UVa/BasicRemains.java
Outdated
input.close(); | ||
} | ||
|
||
static int nextInt() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are this and the next method used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have used a template that a friend sent me. It is a general template that is supposed to be useful and avoid the necessity to repeat the same procedures of getting input and output the result for some common coding challenges that are at Online Judges. This is a similar template with the one that I used
https://github.com/ifhuang/Problems/blob/master/src/codeforces/util/Template7.java
However, in this problem, I had to get a BigInteger as an input, so I used Scanner to get the input from the console, but did not remove the methods of the template that are automatically inserted by the IDE when a new class is created.
I used .close() method of Scanner, because of resource leaks. However, I researched more on this and decided to remove it, as JVM will close it.
@EugenHotaj I committed my changes. Please review them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the new changes. Took another pass just looking at surface level stuff.
Once you fix these, I'll take an actual look at the logic.
UVa/BasicRemains.java
Outdated
} | ||
|
||
input.close(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you have an extra right bracket here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the extra bracket as well.
UVa/BasicRemains.java
Outdated
10 | ||
789 | ||
*/ | ||
import static java.lang.Integer.parseInt; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix your imports. Only import files you're actually using in the class. This goes for all other files in the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed unused classes that were left imported from earlier changes.
UVa/BasicRemains.java
Outdated
System.out.println((p.mod(m)).toString(baseNumber)); | ||
} | ||
|
||
input.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: unnecessary close statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this unnecessary close statement.
UVa/JollyJumper.java
Outdated
} | ||
} | ||
|
||
public static int findMaximalElement(int[] numbers) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this method. It's not being used anywhere AFAICT
UVa/Newspaper.java
Outdated
Sample Output | ||
3.74$*/ | ||
|
||
package UVa; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: remove package or code will break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the package declaration.
UVa/PrimeFactors.java
Outdated
200 = 2 x 2 x 2 x 5 x 5*/ | ||
|
||
//https://uva.onlinejudge.org/index.php?option=com_onlinejudge&Itemid=8&page=show_problem&problem=524 | ||
package UVa; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the package declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem like you removed the package declaration here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sorry @EugenHotaj. There were a lot of package declarations, and I thought I removed all of them. I committed the change with the removed package declaration.
UVa/PrimeFactors.java
Outdated
boolean isNegative = false; | ||
if (number < 0) { | ||
isNegative = true; | ||
number = (int) Math.abs(number); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you casting to an int here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the unnecessary casting that I did.
UVa/PrimeFactors.java
Outdated
|
||
static void formatOutput(int number, List<Integer> primeFactors, boolean isNegative) { | ||
if (isNegative) { | ||
number = number * (-1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this to be number *= -1;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it in this shorter form. It looks better now :).
@EugenHotaj I committed these changes. Please review them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PRs Fatos. I'll leave it to @kdn251 to review correctness of the algos before he meges. Looks good from a readability standpoint after you fix this last comment.
UVa/BasicRemains.java
Outdated
@@ -0,0 +1,41 @@ | |||
|
|||
/* | |||
Given a base b and two non-negative base b integers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please fix these comments to follow either javadoc style or multiline comment stye:
/**
* commet here
* comment here
*/
Note that the only difference between a javadoc and a multiline comment is that a javadoc starts with /** while a comment starts with /*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed the comments and used javadoc. Actually, I intentionally used that type of commenting in the beginning, so that people who are struggling to solve these problems can find these solutions a lot easier by searching with the problem descriptions. Adding asterisks or slashes in the beginning of lines can make it difficult for other people to find these solutions by simply using the problem description as a Google search query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You actually make a very good point. I'll leave it up to @kdn251 to have the final say on if he wants to have a consistent style or "searchability".
@EugenHotaj I am glad and grateful to be able to contribute to this very helpful repository. As for the correctness, all these solutions of these problems have been accepted as correct solutions by the Online Judge at https://uva.onlinejudge.org/. @kdn251 may verify them :). |
UVa/BasicRemains.java
Outdated
@@ -0,0 +1,44 @@ | |||
|
|||
/** | |||
*Given a base b and two non-negative base b integers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: by the way, for the comment to really follow javadoc style it should be in this form:
/**
* <- all asterisks line up with the first asterisk
* <- one space between an asterisk and the first character of a line
*/
Up to you if you want to make this change or not.
Hi @fatosmorina, thank you so much for the PR and thanks @EugenHotaj for reviewing the commits. I have a few notes before I can merge your code... -Missing import statements for JollyJumpers (please add the S to the title of the class) Looking forward to your changes! |
Hi @kdn251 |
Anytime! I just resubmitted and it worked. My mistake I believe I was submitting it for the wrong problem! Now as just one last minor correction could you just change the name of the LastNonZeroDigit file to TheLastNonZeroDigit please. I would just like to stay consistent with the naming conventions. Thank you! |
@kdn251 I renamed the file. You can have look at it yourself. Thanks. |
Looks great, I'm merging the changes now. Thank you again for your contribution @fatosmorina! |
Thank you too @kdn251 . It is my pleasure. I am planning to continue with further contributions. |
Solutions of some UVa problems