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

[UserStory] QT4BJ extension has its own panel in BlueJ Preferences>Extensions #112 #146

Merged
merged 5 commits into from
Dec 5, 2019

Conversation

holsappled
Copy link
Contributor

Created Preferences class and associated acceptance test as an AcceptanceTest.md.

@@ -0,0 +1,23 @@
# AcceptanceTest for QualityToolsExtension.jar
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there exactly one and only one acceptance test? Is a new file needed for an additional test?

Is the intent that this only apply to a file named QualityToolsExtension.jar? Does it need to be rewritten or replaced if the name of the jar file evolves during the lifetime of the product?

@@ -0,0 +1,23 @@
# AcceptanceTest for QualityToolsExtension.jar

Due to the difficulty of JUnit tests developers created the following acceptance test to verify the start up functionality of the BlueJ application.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is meant by "Due to the difficulty of JUnit tests"? What is the intent of the opening phrase of this document?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Invoking the BlueJ proxy object to create a JUnit test.

Copy link
Collaborator

@jody jody left a comment

Choose a reason for hiding this comment

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

Please have multiple members of your team perform a code review and attest to the quality and conformance to conventions.


# Steps

1) Gradle build the project with JDK version 11
Copy link
Collaborator

Choose a reason for hiding this comment

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

As I progress through this document, I begin to understand that this is intended more as a "one off" acceptance criterion than something useful for longer duration of product development and maintenance.

If this is the case, then it should appear as "Acceptance Criteria" in the associated issue, rather than as a document in the repository.

Consider copying the verification steps to the appropriate issue(s) and deleting this document.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussing with the group, having a JTextField displayed in the quality tools extension tab serves no purpose. The intent was to match the #112 user story "picture display"

class Preferences implements PreferenceGenerator
{
private JPanel myPanel;
private JTextField color;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the intent of this field? It seems irrelevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Field is needed to create JTextField from the Swing API.

After discussing with the group, having a JTextField displayed in the quality tools extension tab serves no purpose. The intent was to match the #112 user story "picture display"

new commit will remove field

Copy link
Collaborator

Choose a reason for hiding this comment

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

The intent was to match the #112 user story "picture display"

Matching that with something relevant to the product, such as, "Quality Analysis Tools", would have been fine. "color" just seems irrelevant.

*/
class Preferences implements PreferenceGenerator
{
private JPanel myPanel;
Copy link
Collaborator

Choose a reason for hiding this comment

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

All fields and methods should be commented, using Javadoc-style comments.

@@ -91,3 +99,42 @@ public URL getURL() {
}
}
}

/**
* Create a Preference Panel located in the BlueJ application
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment seems to indicate that this class determines the location of the panel. That doesn't seem to be the actual way this is working.

myPanel = new JPanel();
myPanel.add (new JLabel ("Quality Analysis Tools"));
color = new JTextField (40);
myPanel.add (color);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is "color"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will remove "color" object as it was used to store the JTextField. There is no user input required for this user story. Will remove lines of code associated with "color".

For reference and to answer your question see the information below.

color = new JTextField (40);
API recommends Note:
"We encourage you to specify the number of columns for each text field. If you do not specify the number of columns or a preferred size, then the field's preferred size changes whenever the text changes, which can result in unwanted layout updates."

https://docs.oracle.com/javase/tutorial/uiswing/components/textfield.html

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a note that it is less about "user input" and more about the choice of "color" in particular as having no obvious relevance and likely being an unintended artifact of a copy-paste of work done by others.

myPanel.add (new JLabel ("Quality Analysis Tools"));
color = new JTextField (40);
myPanel.add (color);
// Load the default value
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is meant by "default value"?

@jody jody requested a review from a team November 23, 2019 21:13
@jody jody added this to the Extension Build v0.1 milestone Nov 23, 2019
@jody jody added enhancement New feature or request and removed enhancement New feature or request labels Nov 23, 2019
Deleted AcceptanceTest.md.  Content of AcceptanceTest.md was better suited inside user story MetroCS#112
Added Javadoc-style comments, deleted example JPanel.
@holsappled
Copy link
Contributor Author

New commits will reflect BlueJ application under BlueJ->Preferences->Extensions Tab

Screen Shot 2019-12-04 at 6 38 37 AM

Copy link
Collaborator

@jody jody left a comment

Choose a reason for hiding this comment

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

This code doesn't appear to follow the coding convention guidelines for the project.
See: Coding Convention Guide

Method headers, class headers, authors
Copy link
Collaborator

@jody jody left a comment

Choose a reason for hiding this comment

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

Will approve to close this out. Need to open bug report to fix formatting (misplaced '{', etc.) to adhere to coding convention.

@jody jody merged commit 4314043 into MetroCS:master Dec 5, 2019
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