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.

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

woc #22

Closed
wants to merge 1 commit into from
Closed

woc #22

wants to merge 1 commit into from

Conversation

Lumos000
Copy link

无选做

@@ -191,14 +192,23 @@ Song NextUpViewModel::getNextSong() {
}
case PlayMode::ListRepeat: {
// TODO
auto index = model.indexOf(song);
index++;
song = model[index];
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if index == size - 1?

break;
}
case PlayMode::Shuffle: {
// TODO
auto index = model.indexOf(song);
srand(QTime(0,0,0).secsTo(QTime::currentTime()));
auto random = rand() % model.count();
Copy link
Collaborator

@ArcticLampyrid ArcticLampyrid Feb 25, 2024

Choose a reason for hiding this comment

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

In fact, using the modulus operator cannot obtain the uniform distribution.
Suggest using modern methods provided by C++ or Qt.

@@ -28,6 +28,8 @@ class SongLyricViewModel : public QAbstractListModel {

QHash<int, QByteArray> roleNames() const override;

QList<SongLyric> ProcessLyric (QString);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
QList<SongLyric> ProcessLyric (QString);
QList<SongLyric> ProcessLyric(QString);

Follow our code style defined in clang-format.yaml, if there is no specific reason.

Copy link
Member

@Serein207 Serein207 Feb 25, 2024

Choose a reason for hiding this comment

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

use pass by const reference for large object if you don't expect to change it 👀

@ArcticLampyrid ArcticLampyrid added this to the LessonWork milestone Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants