Needless processing and confusing logic in source code

[Ayowel]

I was looking at the code after seeing #523 to figure out what the current behavior is and stumbled on while loops with code that seems useless/confusing to me. Example (several functions with similar logic in this class) :

AsciidocFX/src/main/java/com/kodedu/service/ui/impl/FileBrowseServiceImpl.java

Lines 333 to 363 in 5648eb1

	ListIterator<TreeItem<Item>> listIterator = foundItems.listIterator();
	while (true) {
	if (Objects.isNull(searchFoundItem)) {
	if (listIterator.hasPrevious()) {
	searchFoundItem = listIterator.previous();
	}
	break;
	}
	if (listIterator.hasNext()) {
	TreeItem<Item> next = listIterator.next();
	if (next.getValue().equals(searchFoundItem.getValue())) {
	if (listIterator.hasPrevious()) {
	TreeItem<Item> previous = listIterator.previous();
	if (next == previous) {
	if (listIterator.hasPrevious()) {
	previous = listIterator.previous();
	}
	}
	searchFoundItem = previous;
	break;
	}
	}
	} else {
	break;
	}
	}

I can't figure out how this is more useful and clear than the following :

ListIterator<TreeItem<Item>> listIterator = foundItems.listIterator();

if (Objects.isNull(searchFoundItem)) {
    if (listIterator.hasPrevious()) {
        searchFoundItem = listIterator.previous();
    }
} else {

    while (listIterator.hasNext()) {

        TreeItem<Item> next = listIterator.next();
        if (next.getValue().equals(searchFoundItem.getValue()) && listIterator.hasPrevious()) {
            TreeItem<Item> previous = listIterator.previous();
            if (next == previous && listIterator.hasPrevious()) {
                previous = listIterator.previous();
            }
            searchFoundItem = previous;
            break;
        }

    }
}

With this, the null check on searchFoundItem is only performed once, we have a clear end condition and more simple branching on the loop.

+ Do you remember why if (next == previous) { ... } was written ? It does not seem to make much sense to me as we traverse the list linearly and from its start.

EDIT: a quick grep shows a few files that use while (true), I'll be going over them to try and make them use a proper end condition

$ grep -rE 'while\s*\(\s*true\s*\)' src/
src/main/java/com/kodedu/controller/ApplicationController.java:            while (true) {
src/main/java/com/kodedu/service/impl/DirectoryServiceImpl.java:            while (true) {
src/main/java/com/kodedu/service/impl/FileWatchServiceImpl.java:        while (true) {
src/main/java/com/kodedu/service/ui/impl/FileBrowseServiceImpl.java:            while (true) {
src/main/java/com/kodedu/service/ui/impl/FileBrowseServiceImpl.java:            while (true) {

[Thanatermesis]

is this the reason of a continuous cpu usage when the application is running even when doing nothing?

[Ayowel]

@Thanatermesis What kind of CPU impact do you see ? A low cpu usage is to be expected, if it is significant that should be investigated.

• Three of those loops are actually bounded and should not impact CPU consumption (see Code quality - drop while(true) #526)
• One (in ApplicationController) is the main application/rendering loop and should be the cause of a slight CPU usage in my experience. An evolution could be to set it to wait when the app is not in focus so that it does not render needlessly, but I don't think the risk/reward ratio makes it worth it at the moment. There might be ways to reduce it's CPU consumption by enforcing a frame rate if it's not already done or lowering the number of re-generated items each frame, but diagnosing/fixing such issues is beyond my ability at the moment.
• One (in FileWatchServiceImpl) listen to system events and contains a blocking call. As such, as long as neither your application nor your system accesses/modifies files that trigger its events it should not affect the CPU consumption.