1
\$\begingroup\$

I'm new to Java right now. Here is my following code snippet:

for(int i = (SelectedAssets.size()-1); -1 < i; i--){

    Double supportTime = 0;
    //Date class must be used. Cannot replace with another class
    Date oDate=SelectedAssets[i].Entitlement_End_Date__c;
    Date dateDifference = oDate.daysBetween(date.today());
    /****************************************************************************
        If the number of days between Entitlement End Date and Todays 
        date is a positive number, then that means the support given to client is expired since 
        entitlement end date happened before todays date.         
    *****************************************************************************/
    if(dateDifference>0){
        supportTime=((dateDifference/365.00));
        //If the number of days is greater than one year, I want 
        //to find out exactly the amount of days it's gone over in decimals. 
        if(supportTime>1){
            supportTime=1;
            supportTime+=(date.today().daysBetween(TheEndDate)/365.00);
        }       
    }
    else{
        //if it hasn't gone over a year, find out how much time has elapsed in years. 
        supportTime=oDate.daysBetween(TheEndDate)/365.00;
    }
}

Is there any redundancies anywhere that I'm overlooking? Is there an easier way of doing this with fewer lines?

\$\endgroup\$
6
  • 2
    \$\begingroup\$ Please reformat and indent your code. Good code formatting is very important for readability and it's an important habit to take care of it. Also wrap lines to some reasonable column count (such as 72). Scrolling to the right to read comments and back makes reading difficult and makes one lose track. See Is inconsistent formatting a sign of a sloppy programmer? (I'm not implying any sloppiness at your side. The answers provide valuable information on the subject.) \$\endgroup\$
    – Petr
    Jul 4, 2013 at 19:39
  • \$\begingroup\$ As I was commenting and rewriting the code, I realised that my understanding of the current code was wrong because of the wrong formatting. Please fix this before anything. \$\endgroup\$
    – SylvainD
    Jul 4, 2013 at 20:38
  • \$\begingroup\$ Hi, I think I fixed the formatting issues. Hope it helps. \$\endgroup\$ Jul 4, 2013 at 21:13
  • \$\begingroup\$ asking for fewer lines of code might be a little difficult without a little more context. Maybe we could see the entire method instead of just this one function. Anyway I feel that this for loop needs to get cleaned up a bit, but more context would help to do that. \$\endgroup\$ Jul 5, 2013 at 4:17
  • 2
    \$\begingroup\$ I am wrong in saying that this piece of code doesn't do anything at the moment ? (Just updating variables with a scope limited to the for-loop). \$\endgroup\$
    – SylvainD
    Jul 5, 2013 at 13:33

2 Answers 2

2
\$\begingroup\$

First thing would be to scan your code with Sonar. This will hit all the formatting complaints, magic numbers, etc.. Likewise, set up your IDE to auto format on save action to save lots of time fixing those Sonar findings.

The next thing to simplify would be to use Calendar instead of Date.

\$\endgroup\$
1
\$\begingroup\$

This can also be written as below (avoid magic numbers, use private helper methods to make your code more clearer to understand):

private static final double NO_OF_DAYS_IN_AN_YEAR = 365.00;
..........
for(SelectedAsset asset : selectedAssets){
    double supportTime = 0.0;
    if(isSupportExpiredForAsset(asset)>0){
        supportTime = convertDaysToYear(getDaysBetweenTodayAndAssetEntitlementEndDate(asset));
        if(supportTime>1){
            supportTime=1+convertDaysToYear(date.today().daysBetween(TheEndDate));
        }
    }else{
        supportTime=convertDaysToYear(asset.Entitlement_End_Date__c.daysBetween(TheEndDate));
    }
}

private int getDaysBetweenTodayAndAssetEntitlementEndDate(SelectedAsset asset){
    return asset.Entitlement_End_Date__c.daysBetween(date.today());
}

private double convertDaysToYear(int days){
    return days/NO_OF_DAYS_IN_AN_YEAR
}

private boolean isSupportExpiredForAsset(SelectedAsset asset){
    return getDaysBetweenTodayAndAssetEntitlementEndDate(asset)>0;
}
\$\endgroup\$
2
  • \$\begingroup\$ Just my 2 cent, but I think it would be better to use some comments instead of super-specific and verbose private method that are used only once and make the whole code harder to follow. \$\endgroup\$
    – tobias_k
    Jul 6, 2013 at 14:05
  • \$\begingroup\$ I agree with specific naming. If the name has to be a mile long for clarity i'd prefer that. My eyes get to read his code like a book. I don't have to go to the private helper methods and read them because I already know what they do. They MIGHT be able to be reworded, but from what I'm reading it is all good. \$\endgroup\$ Jul 8, 2013 at 13:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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