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.

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

Code refactoring #91

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

alensden2
Copy link


Set #1

  1. Explaining Variable
    Location - src\main\java\net\finmath\fouriermethod\calibration\models\CalibratableMertonModel.java
    method name - getCharacteristicFunctionModel()
    Prev commit - 93620a2
    This commit - 399f245
    Maven compile status - success
    Line : 104
    Explanation : In this refactored code, each field of the descriptor object is assigned to an explaining variable with a descriptive name. The method then returns a new MertonModel object constructed using these variables. The refactoring has improved the readability and
    understandability of the code.

  1. Decompose Conditional
    Location - src\main\java\net\finmath\marketdata\products\MarketForwardRateAgreement.java
    method - getValue()
    Prev commit - 399f245
    This commit - a0ff0fb
    Maven compile status - success
    Line : 62
    Explaination :
    While running Designite - it picked up a complex conditional smell -
    “if(forwardCurve == null && forwardCurveName != null && forwardCurveName.length() > 0)”
    This was refactored by adding another layer of abstraction and encapsulating the if conditional logic, to another method -
    “ public boolean checkForwardCurve(ForwardCurve forwardCurve, String forwardCurveName){
    return (forwardCurve == null && forwardCurveName != null && forwardCurveName.length() > 0);
    }

    Now the if becomes - if(checkForwardCurve(forwardCurve, forwardCurveName))

  1. Rename method/variable
    Location : src\main\java\net\finmath\timeseries\models\parametric\DisplacedLognormalARMAGARCH.java
    method : getLogLikelihoodForParameters()
    Prev commit : a0ff0fb
    This commit : 9535176
    Maven compile status : success
    Line : 92
    Explanation : In this version, variables like evalPrev, eval, value1, and value2 have been renamed to evaluationPrev, evaluation, currentValue, and nextValue, respectively, to make their purpose more clear.

Set #2

  1. Extract Class
    Location : src\main\java\net\finmath\marketdata\calibration\CalibratedCurves.java
    method : createDiscountCurve()
    Prev commit : 9535176
    This commit : 0ab8b66
    Maven compile status : success
    Line : 758
    Explanation : the class CalibratedCurves had a feature envy problem due to the fact that this class had multiple responsibilities assigned to it, Here the class had the responsibility of creating calibrated curves and discounted analytic curves.By following the "Extract Class" refactoring. I have moved the method createDiscountCurve to a new class DiscountedAnalyticCurves and made it a public method.

By doing this, I have improved the code's readability and maintainability. The code now has a clear separation of concerns, and the CalibratedCurves class no longer has a feature envy issue. The DiscountedAnalyticCurves class now has a single responsibility of creating discount curves from an analytic model.

Additionally, I have reduced the coupling between the CalibratedCurves and AnalyticModel classes by passing the AnalyticModel as a parameter to the createDiscountCurve method instead of accessing it through a field. This makes the DiscountedAnalyticCurves class more reusable and testable.

New added class Location : src\main\java\net\finmath\marketdata\calibration\DiscountedAnalyticCurves.java

  1. Replace condition with polymorphism
    Location : src\main\java\net\finmath\interpolation\RationalFunctionInterpolation.java
    method - getValue()
    Prev commit - 0ab8b66
    This commit - ea7248c
    Maven compile status - success
    Explanation -
    To replace the conditional with polymorphism, I identified the common behaviour between the different types of extrapolation methods and then create a superclass that contains this behaviour. Then, I create a subclass for each type of extrapolation method and move the behaviour specific to each method to its respective subclass.
    Created an abstract superclass Extrapolation: location - src\main\java\net\finmath\interpolation\Extrapolation.java

Created a subclass for each type of extrapolation method and implement the specific behaviour in each subclass:
Location - src\main\java\net\finmath\interpolation\LinearExtrapolation.java
Location - src\main\java\net\finmath\interpolation\ConstantExtrapolation.java

With this refactored code, we have replaced the conditional logic for extrapolation with a polymorphic approach, making the code easier to understand and maintain.

  1. Move method
    Location : src\main\java\net\finmath\montecarlo\GammaProcess.java
    method : getIncrement()
    line: 118
    Moved method to : src\main\java\net\finmath\montecarlo\RandomVariableLazyEvaluation.java
    line: 111
    Prev commit - ea7248c
    This commit - 64f8845
    Maven compile status - success
    Explanation - The Gamma method had getIncreament method in it which was returning a random variable, It also follows lazy initialization and there was already a class for randomVariablesLazyEvaluation.
    I moved the method to that class to ensure better code maintainability and readablity.

ALL COMMITS :
commit 64f8845 (HEAD -> code-refactoring)
Author: Alen John al283652@dal.ca
Date: Wed Apr 5 21:36:23 2023 -0300

Move Method -Set 2

commit ea7248c
Author: Alen John al283652@dal.ca
Date: Wed Apr 5 18:07:56 2023 -0300

Replace condition with polymorphism -Set 2

commit 0ab8b66
Author: Alen John al283652@dal.ca
Date: Wed Apr 5 16:31:25 2023 -0300

Extract class -Set 2

commit 9535176
Author: Alen John al283652@dal.ca
Date: Wed Apr 5 14:59:04 2023 -0300

Rename variable - Set 1 -  done

commit a0ff0fb
Author: Alen John al283652@dal.ca
Date: Wed Apr 5 14:35:44 2023 -0300

Decompose conditional statement - Set 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant