1

I'm curious if there is a pattern or at least a better way to code this situation. For example, say you're writing a rest api for a reporting workflow. You have a User class and a Job class. Each User has a Job, and each Job a JobType. Some actions will be the same for the user but have additional logic based on their Job Type - submitting a Report for a user in Finance or Legal will have some more logic involved than an HR user's Report. Based off of only the User's id I want to check if their report requires extra work and then execute the proper logic.

Pseudocode:

SubmitReportForUser(int userId, Report report) {
  DoCommonReportWork(report)
  if (ReportsNeedMoreWork(userId)){
    DoExtraReportWork(userId, report);
  }
}

ReportsNeedMoreWork(int userId) {
  JobType = GetJobTypeForUser(userId); // Get from db
  return (JobType == Finance || JobType == Legal);
}

DoExtraReportWork(userId, report) {
  JobType = GetJobTypeForUser(userId); // Get from db
  if (JobType == Finance) { DoFinanceReportWork(report); }
  else if (JobType == Legal) { DoLegalReportWork(report); }
}

Having to load the JobType twice and doing multiple checks on what the JobType is both come off as code smells to me, but I'm not sure the best way to go about this situation. The only immediate fix I see would be to load the JobType earlier and have ReportsNeedMoreWork and DoExtraReportWork take the JobType instead of the user id, but that doesn't get around the duplicated if/elses. This is a simplified example, in my case there are many more JobTypes that would involve extra work.

Thanks for reading so far, any feedback is appreciated! This is an old codebase I'm working with, but looking to improve upon. Thanks!

Odd822
  • 47
  • 2
  • 1
    What's the problem with calling `DoExtraReportWork` all the time? It currently does nothing if there is no extra work to be done. – 1201ProgramAlarm Mar 06 '20 at 21:15
  • @1201ProgramAlarm I guess there is not an issue with calling it every time, that's a good point. My gut reaction to it was that a call to `DoExtraReportWork` would imply that there is extra work actually being done, but I don't know that that is something to be concerned about. – Odd822 Mar 06 '20 at 21:31
  • 1
    Does this answer your question? [Style for control flow with validation checks](https://softwareengineering.stackexchange.com/questions/148849/style-for-control-flow-with-validation-checks) – gnat Mar 06 '20 at 21:43
  • What if the user has more than one job type? Don't you need the report to know whether legal vs. financial? Also, what if one job is both legal and financial (like, say, real estate purchase)? – Erik Eidt Mar 06 '20 at 22:58
  • @gnat Not quite, I think that is a slightly different situation (but actually helpful for another area of the application, thank you!) – Odd822 Mar 09 '20 at 13:26
  • @ErikEidt In this situation, users will only have one job type and there won't be overlapping jobs. The only shared piece would be the reports themselves (they have the same fields to fill out). – Odd822 Mar 09 '20 at 13:27

0 Answers0