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

Issue 393 Notification Pattern #1912

Closed
wants to merge 30 commits into from
Closed

Conversation

NatalieSty
Copy link

Add Notification pattern (issue pattern #393
A Notification is an object that collects information about errors during validation of data. When an error appears the Notification is sent back to the view to display further information about the errors.

Pull request description

  • included Junit tests with 100% coverage
  • ReadMe.md, UML diagram included

@NatalieSty NatalieSty closed this Nov 17, 2021
@NatalieSty NatalieSty reopened this Nov 17, 2021
categories: Behavioural
language: en
tags:
- Notification
Copy link
Owner

Choose a reason for hiding this comment

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

For tags, I would put Decoupling and Presentation

Copy link
Owner

Choose a reason for hiding this comment

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

Please remove the tag Notification

Comment on lines 14 to 17
<properties>
<maven.compiler.source>17</maven.compiler.source>
<maven.compiler.target>17</maven.compiler.target>
</properties>
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think these are needed. We use Java 11 language level for the whole project.

notification/pom.xml Show resolved Hide resolved
Comment on lines 3 to 8
/**
* With the DAO pattern, we can use various method calls to
* retrieve/add/delete/update data without directly interacting
* with the data source. The below example demonstrates basic CRUD
* operations: select, add, update, and delete.
*/
Copy link
Owner

Choose a reason for hiding this comment

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

The text seems to be copied from another pattern. Please update it to introduce the notification pattern briefly and describe how the example code implements it.

Comment on lines 11 to 13
/**
* This is not called.
*/
Copy link
Owner

Choose a reason for hiding this comment

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

Instead, please describe the class

Comment on lines 23 to 27
final String courseId = "CSE427";
final String semester = "Fall21";
final String department = "Engineering";
final FormRegisterCourse form = new FormRegisterCourse(courseId, semester, department);
form.submit();
Copy link
Owner

Choose a reason for hiding this comment

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

It's possible to use local variable type inference (keyword var) here. Additionally, it would be nice to have some comments that describe what happens. This is an example code for others to read, after all.

Comment on lines 8 to 15
/**
* Get a Notification object.
*
* @return Notification object.
*/
public Notification getNotification() {
return notification;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Use Lombok to get rid of boilerplate code

Copy link
Owner

Choose a reason for hiding this comment

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

Please check this throughout the code

@@ -0,0 +1,23 @@
package com.iluwatar.notification;

public class Error {
Copy link
Owner

Choose a reason for hiding this comment

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

It's good practice to provide Javadoc for each class

Copy link
Owner

Choose a reason for hiding this comment

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

Error as a name can be a little bit too generic. Please consider alternatives.

import lombok.extern.slf4j.Slf4j;

@Slf4j
public class FormRegisterCourse {
Copy link
Owner

Choose a reason for hiding this comment

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

I find the name a little strange. Wouldn't RegisterCourseForm sound better?

Comment on lines 10 to 19
@Test
void testCourseServiceTest() {
RegisterCourseDto registerDTO = new RegisterCourseDto();
registerDTO.setDepartment("English");
registerDTO.setCourseId("CS444");
registerDTO.setSemester("Fall21");
CourseService course = new CourseService();
Boolean isRegistered = course.registerCourse(registerDTO);
assertTrue(isRegistered);
}
Copy link
Owner

Choose a reason for hiding this comment

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

This probably tests that the happy scenario for registering for a course succeeds. The naming of the test could be better. Additionally, there should be more tests that check the negative cases (course registration does not succeed).

@NatalieSty
Copy link
Author

@iluwatar thanks for the comments. I have made some changes and committed the change

@sonarcloud
Copy link

sonarcloud bot commented Dec 5, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 22 Code Smells

98.1% 98.1% Coverage
0.0% 0.0% Duplication

@@ -210,7 +210,7 @@ Example use cases

## Consequences

* Dependency injection in java hides the service class dependencies that can lead to runtime errors that would have been caught at compile time.
* Dependency injection in java hides the service class dependencies that can lead to runtime notificationErrors that would have been caught at compile time.
Copy link
Owner

Choose a reason for hiding this comment

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

It seems that some text has been accidentally replaced in many other patterns. Please revert the changes.

categories: Behavioural
language: en
tags:
- Notification
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove the tag Notification


## Intent

A Notification is an object that collects information about notificationErrors during validation of data. When an notificationError appears the Notification is sent back to the view to display further information about the notificationErrors.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
A Notification is an object that collects information about notificationErrors during validation of data. When an notificationError appears the Notification is sent back to the view to display further information about the notificationErrors.
Notification is an object that collects information about notification errors during the validation of data. When a notification error appears, the notification is sent back to the view to display further information about the notification errors.

Comment on lines +23 to +24
> There's a form to register for a course. We need to validate the inputs to make sure
> the inputs are valid.
Copy link
Owner

Choose a reason for hiding this comment

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

Please use passive voice (see https://www.grammarly.com/blog/passive-voice/)


In plain words

> An object that collects together information about notificationErrors and other information and communicates it to the presentation.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
> An object that collects together information about notificationErrors and other information and communicates it to the presentation.
> Notification is an object that collects together information about notification errors and communicates it to the presentation layer.

@iluwatar iluwatar added the status: stale issues and pull requests that have not had recent interaction label Sep 21, 2022
@iluwatar iluwatar added this to the 1.26.0 milestone Sep 21, 2022
@iluwatar iluwatar closed this Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: stale issues and pull requests that have not had recent interaction status: under review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants