Skip to content

Hw1 lazy#1

Merged
XRater merged 6 commits intomasterfrom
HW1-Lazy
May 3, 2018
Merged

Hw1 lazy#1
XRater merged 6 commits intomasterfrom
HW1-Lazy

Conversation

@XRater
Copy link
Copy Markdown
Owner

@XRater XRater commented Feb 28, 2018

No description provided.

Comment thread HW1-Lazy/build.gradle
}
dependencies {
classpath 'org.junit.platform:junit-platform-gradle-plugin:1.0.2'
classpath 'com.google.guava:guava:23.5-jre'
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

С отступами что-то не так :) Где-то пробелы, где-то табы

Comment thread HW1-Lazy/build.gradle Outdated
}
// configurationParameter 'junit.jupiter.conditions.deactivate', '*'
// enableStandardTestTask true
// reportsDir file('build/test-results/junit-platform') // this is the default
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Куча закомментированного кода, в общем-то, не нужна. Это же какой-то стандартный пример, можно было его почистить

import java.util.function.Supplier;

/**
* Factory class to produce different ru.spbau.mit.kirakosian.Lazy classes. Unsynchronized and synchronized versions
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ru.spbau.mit.kirakosian.Lazy в комментарии --- неудачное переименование? :)

}

// as i see, if there is no description the one from the interface will be shown
// it looks more beautiful then the link in my opinion
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ага

synchronized (this) {
if (supplier != null) { // check that we have not done this section yet
value = supplier.get();
supplier = null;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supplier надо сделать volatile, иначе получится как описано тут: https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html. supplier уже null, поэтому условие в строке 85 уже ложно, но в value ещё фигня какая-то.

oneCreateSynchronizedLazyTest("hello");
oneCreateSynchronizedLazyTest(null);
} catch (@NotNull final InterruptedException e) {
// no idea what to do here... It is not a failure
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Дать ему упасть. InterruptedException-ов ведь в этом тестовом сценарии быть не должно. Так что лучше прописать InterruptedException в throws, и если поток таки кто-то прервёт, пусть тест не пройдёт.

Copy link
Copy Markdown

@yurii-litvinov yurii-litvinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Окей, теперь существенных замечаний не осталось, зачтена.

Comment thread HW1-Lazy/build.gradle
}
dependencies {
classpath 'org.junit.platform:junit-platform-gradle-plugin:1.0.2'
classpath 'com.google.guava:guava:23.5-jre'
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Стало даже хуже :)

*
* @param supplier supplier to get element from.
* @param <T> type of the stored in ru.spbau.mit.kirakosian.Lazy element
* @return new concurrent unsafe ru.spbau.mit.kirakosian.Lazy instance
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ну и я бы сказал, что последствия неудачных переименований надо ликвидировать. Либо если это было сделано намеренно, то всё равно ликвидировать :)

@XRater XRater merged commit 4f80b20 into master May 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants