current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote -1 down vote favorite

I was curious to anyone's thoughts, comments, and input as to something I was presented with during a job interview I went to recently. I was provided with some java code and asked to determine what was wrong with it and how bad it was. So, seeing as I didn't get the job I was curious what are the thoughts on the following code.

public void assessAdjustSuspendFJOffense(Long offID, Date nextDueDate, List fjList, List amountList, List reasonList, OverrideApprovalHolder ovrApprovalBean) {

  Offense off = (Offense) getDom(offID, "Offense", Offense.class);
  Assert.notNull(nextDueDate, "due date cannot be null");

  Defendant def = (Defendant) off.getDefendant();
  Person person = (Person) def.getEntity();

  StandardAssetAccount accRcv = (StandardAssetAccount) accDao
    .getStandardAccount(StandardAccount.ACCOUNT_RECEIVABLE);
  StandardLiabilityAccount accDefPay = (StandardLiabilityAccount) accDao
    .getStandardAccount(StandardAccount.DEFERRED_PAYABLE);

  Iterator it = fjList.iterator();
  for (int i = 0; i < fjList.size(); i++) {

    FJOffense fjOff = (FJOffense) fjList.get(i);
    AssessAdjustSuspendBean aas = (AssessAdjustSuspendBean) amountList.get(i);
    Type reasonType = (Type) reasonList.get(i);

    if (fjOff.getID() != null && aas.getAdjust().compareTo(ZERO) == 0 && aas.getSuspend().compareTo(ZERO) == 0) {
      continue;
    }
    if (fjOff.getID() != null) {
      fjOff = (FJOffense) accDao.getById(FJOffense.class, fjOff.getID());
    }

    BigDecimal assAmt = aas.getAssess();

    List msgs;
    if (fjOff.getID() == null) {
      if (ZERO.compareTo(assAmt) >= 0) { // otherwise there is an ugly error coming from an assert way deep.
        msgs = new ArrayList();
        msgs.add("original assessments need to be positve");
        throw new ValidationFailureException(msgs);
      }
      FineFee finFee = (FineFee) accDao.getById(FineFee.class, fjOff.getFineFee().getID());

      fjOff = off.assess(assAmt, finFee, accRcv, accDefPay);

      msgs = fjOff.accept(saveValidator);
      if (!msgs.isEmpty()) {
        throw new ValidationFailureException(msgs);
      }
      accDao.save(fjOff);

      def.add(fjOff);

      ThreadSession.flush();
    } else {
      fjOff = (FJOffense) accDao.getById(FJOffense.class, fjOff.getID());
    }

    if (aas.getAdjust().equals(ZERO) == false) {

      FinancialJudgmentAdjustmentTransaction fjAdjTrn = fjOff.adjust(aas.getAdjust(),
        reasonType, ovrApprovalBean.getApprover(), accRcv, accDefPay);

      msgs = fjAdjTrn.accept(saveValidator);
      if (msgs.isEmpty() == false) {
        throw new ValidationFailureException(msgs);
      }
      accDao.save(fjAdjTrn);
    }
    if (aas.getSuspend().equals(ZERO) == false) {

      FinancialJudgmentSuspensionTransaction fjSusTrn = fjOff.suspend(aas.getSuspend(),
        reasonType, ovrApprovalBean.getApprover(), accRcv, accDefPay);
      msgs = fjSusTrn.accept(saveValidator);
      if (msgs.isEmpty() == false) {
        throw new ValidationFailureException(msgs);
      }
      accDao.save(fjSusTrn);
    }
  }

  if (isBeforeToday(nextDueDate)) {
    ArrayList msgs = new ArrayList();
    msgs.add("next due date cannot be a passed date");
    throw new ValidationFailureException(msgs);
  }

  def.setDueDate(nextDueDate);

}

share|improve this question

put on hold as off-topic by vnp, h.j.k., 200_success♦ Nov 10 at 5:40

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.
1  
Java is not JavaScript. Please do not put in in a code snippet. –  Pimgd Nov 10 at 8:49
    
Apologies as I was asked to come over here from Stack Exchange and am just trying to familiarize myself with some of the tools. –  FROSTYSMOOTH 2 days ago

1 Answer 1

up vote 4 down vote accepted

During reading this code following thoughts came to my mind:

  • method name assessAdjustSuspendFJOffense is not very meaningful
  • this method has too many argument (in my opinion)
  • raw types for fjList, amountList and reasonList (use generics instead)
  • a lot of type casting (may cause a ClassCastException)
  • method is way too long (should be splitted into several smaller methods)
  • raw type of Iterator it (causes more type casting :( )
  • this mentioned iterator is never used
  • aas.getAdjust().compareTo(ZERO) == 0 should be aas.getAdjust().equals(ZERO)
  • more raw types coming this way: List msgs;
  • the usage of that list msgs should be replaced with a variable with a "smaller" scope
  • throw new ValidationFailureException(msgs) it might by handy to add a corresponding throws clause to the method
  • aas.getAdjust().equals(ZERO) == false is the same as !aas.getAdjust().equals(ZERO)
  • ... the above change applies to several if statements

I assume that there is more stuff, that could be changed, but I hope you have an overview now.

share|improve this answer
4  
That's a very nice answer! +1 It's a pity that the question itself is in straight violation of our policies :( Save your energy for better questions! ;-) –  janos Nov 10 at 7:19
    
I was given the code and asked to reach out to others and I figured since I was given the code and it is in my possession I would "reach out". So if that is in violation I apologize and will not seek assistance for code that I own. –  FROSTYSMOOTH 2 days ago

Not the answer you're looking for? Browse other questions tagged or ask your own question.