-
Notifications
You must be signed in to change notification settings - Fork 5
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 worker-pay command and tests #112
Add worker-pay command and tests #112
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, some minor issues
public static final String COMMAND_WORD = "worker-pay"; | ||
|
||
public static final String MESSAGE_USAGE = COMMAND_WORD | ||
+ ": Prints the pay earned by a worker identified by the index number used in the displayed worker list.\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+ ": Prints the pay earned by a worker identified by the index number used in the displayed worker list.\n" | |
+ ": Calculates the weekly pay earned by a worker identified by the index number used in the displayed worker list.\n" |
Prints
seems to be a more programming-specific term which the target user (ie. a McDonald's manager) may not understand.
} | ||
|
||
Worker selectedWorker = lastShownList.get(targetIndex.getZeroBased()); | ||
float calculatedPay = model.calculateWorkerPay(selectedWorker); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think implementing the logic of calculateWorkerPay()
in this class itself may be more consistent with the rest of our commands. Maybe the rest can chip in on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is ok, for a possible future implementation could allow shifts of differing number of hours - hence calculating worker pay will need to be at a model level. Besides Worker
has no knowledge of its assignments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll put calculateWorkerPay()
in model for now. Putting calculateWorkerPay()
in the command class is also fine.
@@ -21,6 +21,7 @@ | |||
*/ | |||
public class ModelManager implements Model { | |||
private static final Logger logger = LogsCenter.getLogger(ModelManager.class); | |||
private static final Integer HOURS_PER_SHIFT = 6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor issue but 8 hours may be more realistic since McDonald's usually open for more than 12 hours a day
Codecov Report
@@ Coverage Diff @@
## master #112 +/- ##
============================================
+ Coverage 70.05% 70.08% +0.03%
- Complexity 812 822 +10
============================================
Files 132 134 +2
Lines 2778 2808 +30
Branches 380 385 +5
============================================
+ Hits 1946 1968 +22
- Misses 711 716 +5
- Partials 121 124 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Resolves #72