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

ExtendedInteger done. #6

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

IgorAfanasenko
Copy link

No description provided.

@xSAVIKx xSAVIKx self-assigned this Dec 8, 2016
Copy link
Member

@xSAVIKx xSAVIKx left a comment

Choose a reason for hiding this comment

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

waiting for other tasks. Also pay attention on comments below.

* @param value to check
* @return true if value is even, false - otherwise
*/
public static boolean isEven(int value) {
//TODO: implement me
if ((value & 1) == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

just return your expression, there is no need for if-else

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Member

Choose a reason for hiding this comment

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

@IgorAfanasenko I don't see any changes checked in, have you already perform commit and pushed your changes ?

* @param value to check
* @return true if value is odd, false - otherwise
*/
public static boolean isOdd(int value) {
//TODO: implement me
if ((value & 1) == 1) {
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

}
resultSum += result;
}
resultSum *= first;
Copy link
Member

Choose a reason for hiding this comment

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

You algorithm is OK, but implementation details sucks.
Try to create more valuable variable names, for example:
What do you mean with your first variable ? In fact, you need to check the sign of int value, if it's plus or minus and multiply result by 1 or -1 respectively. So, probably variable name sign will suite here better.
Additionally you've got j, k, i indexes, but you really need only one.
Also, there is no need for 2 variables for result you can reuse one.

But, anyway, you code do pass all the tests.

//TODO: implement me
return null;

char[] newChar = value.toCharArray();
Copy link
Member

Choose a reason for hiding this comment

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

What if value == null ?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

if (obj == this)
return true;

if (obj == null)
Copy link
Member

Choose a reason for hiding this comment

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

null check should always be the first one, as there is nothing to do if obj==null.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

if (obj == null)
return false;

if (!(getClass() == obj.getClass()))
Copy link
Member

Choose a reason for hiding this comment

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

it's always better to avoid negation.
this part could be rewritten as: getClass() != obj.getClass()

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.


if (!(getClass() == obj.getClass()))
return false;
else {
Copy link
Member

Choose a reason for hiding this comment

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

there is no need for else statement here, cause you always use return

return false;
else {
ExtendedInteger tmp = (ExtendedInteger) obj;
if (tmp.value == this.value)
Copy link
Member

Choose a reason for hiding this comment

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

you can just return result of this expression: return tmp.value == this.value

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

Successfully merging this pull request may close these issues.

2 participants