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

Add comments to mainwindow.cpp - partial #1109

Merged
merged 6 commits into from
Jul 30, 2024

Conversation

slspencer
Copy link
Collaborator

add comments through initializeScenes()

add comments through initializeScenes()
Copy link
Member

@csett86 csett86 left a comment

Choose a reason for hiding this comment

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

Thank you for asking for my opinion on this, @slspencer. To be very honest I dont see much value with many of these comments, as the code is anyway quite readable to me. But if they help you and @DSCaskey working on the code, I not at all blocking...

@DSCaskey
Copy link
Contributor

DSCaskey commented May 28, 2024

@csett86 No they dont help me... it's just more noise I will now have to read through.

Again these comments are overkill. Too many comments just make it harder to read. If someone doesn't know what for ex: an

#include <QSpinbox>

is - they need to learn.

Besides who comments #includes? Having code with commented inludes just make us look amateur.

Want to write @Briefs for the methods and functions that lack one - have it, but it's counter productive for all these redundant comments.

@DSCaskey
Copy link
Contributor

DSCaskey commented May 28, 2024

The more I look at this the more I disapprove. There's just a lot "well DUH" comments. Comments should NOT just repeat the code.

createActions();                           // Create actions
initializeScenes();                        // Initialize scenes

A reason to comment say createActions() might be:

createActions();      // Needs to be before initializeScenes()
initializeScenes(); 

This is C++ and you don't need to comment every thing. It's a waste of time and just adds more crap to read through.

 // Connect the list widget's currentRowChanged signal to the showLayoutPages slot
connect(ui->listWidget, &QListWidget::currentRowChanged, this, &MainWindow::showLayoutPages);

It's just a "lazy - well duh" comment repeating what the code says, and if a programmer can't read a connect like this - then they shouldn't be programming.

@sconklin
Copy link
Collaborator

This latest version is pretty awesome. It's readable and also provides the narrative.
I approve

@slspencer slspencer requested a review from Onetchou June 4, 2024 15:47
Copy link
Contributor

@Onetchou Onetchou left a comment

Choose a reason for hiding this comment

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

I find that most of these comments are only repeating what the code is telling pretty explicitly, and I fear this could be a little annoying since it creates other readability issues such as a more crowded code, unwanted line breaks, alignment issues etc.

But ofc the ones expliciting non readable parts of the code, as well as the briefs, are great!

Copy link
Contributor

@Esterjudith Esterjudith left a comment

Choose a reason for hiding this comment

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

There are some good comments added that might be useful for contributors.

The TODO comment looks fine to me. Although TODO comments are typically reminders of tasks that need to be done later, similar to a to-do list, and not explanations of what the code is doing, I think it's okay in this context. However, the "Create new VPattern object" comment is not necessary because the syntax used to create an object below is very common in many programming languages.

      // TODO: brief comment on what VPattern object does (everything is stored to the VPattern object)
       doc = new VPattern(pattern, &mode, draftScene, pieceScene);  // Create new VPattern object

Overall, documentation is important, especially in hard-to-read code, as long as the comments are not redundant and add to clarity.

@slspencer
Copy link
Collaborator Author

Thank you @DSCaskey !!!!

@slspencer slspencer merged commit 2438932 into develop Jul 30, 2024
10 checks passed
@slspencer slspencer deleted the add-comments-mainwindow-to-line596.cpp branch July 30, 2024 17:13
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.

6 participants