AsciidocFX icon indicating copy to clipboard operation
AsciidocFX copied to clipboard

Needless processing and confusing logic in source code

Open Ayowel opened this issue 4 years ago • 2 comments
trafficstars

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

Ayowel avatar Jul 17 '21 18:07 Ayowel

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

Thanatermesis avatar Jul 27 '21 04:07 Thanatermesis

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

Ayowel avatar Jul 31 '21 17:07 Ayowel