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.

Sign up for GitHub

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

Jump to bottom

Needless processing and confusing logic in source code #524

Open
Ayowel opened this issue Jul 17, 2021 · 2 comments · May be fixed by #526
Open

Needless processing and confusing logic in source code #524

Ayowel opened this issue Jul 17, 2021 · 2 comments · May be fixed by #526

Comments

@Ayowel
Copy link
Contributor

Ayowel commented Jul 17, 2021 •
edited

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) {
@Ayowel Ayowel linked a pull request Jul 18, 2021 that will close this issue
Open
@Thanatermesis
Copy link

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

@Ayowel
Copy link
Contributor Author

Ayowel commented Jul 31, 2021 •
edited

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Code quality - drop while(true) Ayowel/AsciidocFX
2 participants
@Thanatermesis @Ayowel