asciidocfx / AsciidocFX

Asciidoc Editor and Toolchain written with JavaFX 21 (Build PDF, Epub, Mobi and HTML books, documents and slides)
http://www.asciidocfx.com/
Apache License 2.0
1.88k stars 295 forks source link

Needless processing and confusing logic in source code #524

Open Ayowel opened 3 years ago

Ayowel commented 3 years ago

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) :

https://github.com/asciidocfx/AsciidocFX/blob/5648eb1dcfcf1801ff6f69586787804e201bce86/src/main/java/com/kodedu/service/ui/impl/FileBrowseServiceImpl.java#L333-L363

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 commented 3 years ago

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

Ayowel commented 3 years ago

@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.