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.

Sign up for GitHub

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

Jump to bottom

Edit attendance #157

Merged
merged 11 commits into from
Oct 19, 2020
Merged

Edit attendance #157

merged 11 commits into from
Oct 19, 2020

Conversation

ypinhsuan
Copy link

Closes #139

@ypinhsuan ypinhsuan requested review from a team, dextertanyj and junlong4321 and removed request for a team October 18, 2020 09:59
@ypinhsuan
Merge branch 'master' into edit-attendance
bcafb4b 
# Conflicts:
#	src/main/java/seedu/address/commons/core/Messages.java
#	src/main/java/seedu/address/logic/parser/TutorsPetParser.java
#	src/test/java/seedu/address/logic/parser/TutorsPetParserTest.java
@ypinhsuan
Abstract methods into util class
6e19cd0
@codecov-io
Copy link

codecov-io commented Oct 18, 2020 •
edited

Codecov Report

Merging #157 into master will increase coverage by 0.45%.
The diff coverage is 90.98%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #157      +/-   ##
============================================
+ Coverage     81.48%   81.93%   +0.45%     
- Complexity      953      980      +27     
============================================
  Files           142      144       +2     
  Lines          2808     2923     +115     
  Branches        324      343      +19     
============================================
+ Hits           2288     2395     +107     
- Misses          441      443       +2     
- Partials         79       85       +6
Impacted Files Coverage Δ Complexity Δ
...seedu/address/logic/commands/AddLessonCommand.java 94.73% <ø> (ø) 8.00 <0.00> (ø)
...ddress/logic/commands/DeleteAttendanceCommand.java 80.55% <ø> (ø) 8.00 <0.00> (ø)
...du/address/logic/commands/DeleteLessonCommand.java 95.45% <ø> (ø) 9.00 <0.00> (ø)
...eedu/address/logic/commands/EditLessonCommand.java 97.05% <ø> (ø) 12.00 <0.00> (ø)
...java/seedu/address/logic/util/ModuleClassUtil.java 87.50% <0.00%> (ø) 11.00 <1.00> (?)
...main/java/seedu/address/logic/util/LessonUtil.java 89.65% <80.00%> (ø) 6.00 <2.00> (?)
...u/address/logic/commands/AddAttendanceCommand.java 92.10% <83.33%> (ø) 13.00 <0.00> (ø)
...ress/logic/parser/EditAttendanceCommandParser.java 88.00% <88.00%> (ø) 6.00 <6.00> (?)
.../address/logic/commands/EditAttendanceCommand.java 97.10% <97.10%> (ø) 18.00 <18.00> (?)
...va/seedu/address/logic/parser/TutorsPetParser.java 100.00% <100.00%> (ø) 32.00 <0.00> (+1.00)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e0dc6e...80d8252. Read the comment docs.

junlong4321
junlong4321 approved these changes Oct 19, 2020
View reviewed changes
Copy link

@junlong4321 junlong4321 left a comment

Choose a reason for hiding this comment

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

LGTM, some formatting changes

src/main/java/seedu/address/logic/commands/EditAttendanceCommand.java Show resolved Hide resolved
src/main/java/seedu/address/logic/commands/EditAttendanceCommand.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/logic/commands/EditAttendanceCommand.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/logic/commands/EditAttendanceCommand.java Show resolved Hide resolved
src/main/java/seedu/address/logic/parser/EditAttendanceCommandParser.java Outdated
boolean isLessonIndexPresent = argMultimap.getValue(PREFIX_LESSON_INDEX).isPresent();
boolean isStudentPresent = argMultimap.getValue(PREFIX_STUDENT_INDEX).isPresent();
boolean isWeekPresent = argMultimap.getValue(PREFIX_WEEK).isPresent();
if (!isModuleClassIndexPresent || !isLessonIndexPresent || !isStudentPresent || !isWeekPresent) {
Copy link

Choose a reason for hiding this comment

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

Separate guard clause

Suggested change
if (!isModuleClassIndexPresent || !isLessonIndexPresent || !isStudentPresent || !isWeekPresent) {
if (!isModuleClassIndexPresent || !isLessonIndexPresent || !isStudentPresent || !isWeekPresent) {

Copy link

Choose a reason for hiding this comment

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

@junlong4321 Actually, for such a case where the Boolean variables are only used in the guard clause, shouldn't they be part of the block for the guard clause?

src/main/java/seedu/address/logic/parser/EditAttendanceCommandParser.java
week = ParserUtil.parseWeek(argMultimap.getValue(PREFIX_WEEK).get());

EditAttendanceDescriptor editAttendanceDescriptor = new EditAttendanceDescriptor();
if (argMultimap.getValue(PREFIX_PARTICIPATION_SCORE).isPresent()) {
Copy link

Choose a reason for hiding this comment

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

Suggested change
if (argMultimap.getValue(PREFIX_PARTICIPATION_SCORE).isPresent()) {
if (argMultimap.getValue(PREFIX_PARTICIPATION_SCORE).isPresent()) {

src/main/java/seedu/address/logic/commands/AddAttendanceCommand.java Show resolved Hide resolved
src/test/java/seedu/address/logic/commands/EditAttendanceCommandTest.java Outdated Show resolved Hide resolved
src/test/java/seedu/address/logic/commands/EditLessonCommandTest.java Show resolved Hide resolved
@junlong4321 junlong4321 added this to the v1.3-core milestone Oct 19, 2020
dextertanyj
dextertanyj approved these changes Oct 19, 2020
View reviewed changes
Copy link

@dextertanyj dextertanyj left a comment

Choose a reason for hiding this comment

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

LGTM! Just a very minor suggestion.

src/main/java/seedu/address/logic/commands/EditAttendanceCommand.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/logic/parser/EditAttendanceCommandParser.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/logic/parser/EditAttendanceCommandParser.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/logic/parser/EditAttendanceCommandParser.java Outdated
boolean isLessonIndexPresent = argMultimap.getValue(PREFIX_LESSON_INDEX).isPresent();
boolean isStudentPresent = argMultimap.getValue(PREFIX_STUDENT_INDEX).isPresent();
boolean isWeekPresent = argMultimap.getValue(PREFIX_WEEK).isPresent();
if (!isModuleClassIndexPresent || !isLessonIndexPresent || !isStudentPresent || !isWeekPresent) {
Copy link

Choose a reason for hiding this comment

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

@junlong4321 Actually, for such a case where the Boolean variables are only used in the guard clause, shouldn't they be part of the block for the guard clause?

@ypinhsuan ypinhsuan merged commit 7ccf177 into AY2021S1-CS2103T-T10-4:master Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dextertanyj dextertanyj dextertanyj approved these changes

@junlong4321 junlong4321 junlong4321 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.3-core
Development

Successfully merging this pull request may close these issues.

Implement command and parser support for edit attendenceRecord
4 participants
@ypinhsuan @codecov-io @dextertanyj @junlong4321