1
\$\begingroup\$

It should return true, if there is no element that follow or the following elements are greater than their predecessors.

public class Element { 
    private int value; 
    private Element next;

    public boolean isSorted(){
        if (this.next == null)
            return true;
        else return (this.value < this.next.value && this.next.isSorted());
    }
}

What do you guys think of it?

\$\endgroup\$
5
  • 1
    \$\begingroup\$ It's hard to review without seeing the rest of the class. For instance, we can't see the declaration of this.next and this.value with what you've shown. It's hard to see why you're explicitly writing this. when accessing members, too. \$\endgroup\$ Nov 25, 2020 at 15:37
  • 1
    \$\begingroup\$ Why not simply public boolean isSorted() { return next == null || value < next.value && next.isSorted(); }? That said, you may be better with iterative, rather than recursive, code. \$\endgroup\$ Nov 25, 2020 at 15:39
  • \$\begingroup\$ public class Element { private int value; private Element next;} the class also includes the isSorted function \$\endgroup\$ Nov 25, 2020 at 16:22
  • 1
    \$\begingroup\$ Welcome to Code Review! When adding additional information you should edit your question instead of adding a comment. I have added that information to your post. Learn more about comments including when to comment and when not to in the Help Center page about Comments. \$\endgroup\$ Nov 25, 2020 at 17:18
  • \$\begingroup\$ for anyone in the close vote queue: this post is on-topic now \$\endgroup\$ Nov 25, 2020 at 17:19

3 Answers 3

3
\$\begingroup\$

With this implementation it is impossible for a list with duplicate elements to be sorted. However, that possibility is easy enough to support:

return (next == null)
    || ((value <= next.value) && next.isSorted());

Now, empty lists are explicitly sorted by the first clause. Lists with an element strictly greater than a previous element are not sorted by the second. And the third says that if the first two values are sorted and the rest of the list is sorted, the list as a whole is sorted.

This works because this uses short circuit Boolean operators. So when next is null, it doesn't bother to check the rest of the statement (true || is always true). And when the current value is greater than the next, it doesn't need to continue (false && is always false). Finally, if it makes it to the last clause, it can simply return the result of it. Because it knows that the first clause was false and the second was true; otherwise, it would never have reached the third.

Alternately, you could change the name from isSorted to isIncreasing which would better describe what you are actually doing. If that is what you actually wanted to do.

Moving from a recursive solution on Element to an iterative solution on the linked list would also make sense.

\$\endgroup\$
7
\$\begingroup\$

I would like to add few improvements for better readability.

  1. You don't need an else statement; rather you should directly return it.

  2. For every if and else statement, even if it is a single line and Java allows it without braces, it's not preferable because of possible code bugs which may arise in future due to missing braces.

  3. Better to put (this.value < this.next.value) in brackets for better readability.

So the updated code should look like:

public boolean isSorted() {
    if (this.next == null) {
        return true;
    }
      
    return (this.value < this.next.value) && this.next.isSorted();
} 
\$\endgroup\$
1
  • 1
    \$\begingroup\$ thank you for your improvements :) \$\endgroup\$ Nov 25, 2020 at 14:32
2
\$\begingroup\$

There is no need for else. For better code readability single point of return should be used.

return (this.next == null) || ((this.value < this.next.value) && this.next.isSorted());
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.