Task/dtserwfour 553 update transfer fee zero state #260
Conversation
shanavas123
requested review from
SaiNarendran,
aparthibanpaypal,
rbanikPP and
vwagh-hw
…ttps://github.com/hyperwallet/hyperwallet-android-ui-sdk into task/DTSERWFOUR-553-update-transfer-fee-zero-state
transfermethodui/src/main/java/com/hyperwallet/android/ui/transfermethod/view/FeeFormatter.java
Outdated
Show resolved
Hide resolved
shanavas123
dismissed stale reviews from aparthibanpaypal and SaiNarendran
via
49f90dd
| protected static boolean isZeroFeeAvailable(@NonNull final Context context, String fee) { | ||
| return fee.contains(context.getResources().getString(R.string.noFee)); | ||
| } |
There was a problem hiding this comment.
method name doesn't sound right, if there is noFee then why does that mean zero fee is available ?
There was a problem hiding this comment.
renamed as isNoFeeStringAvailable.
| feeAndProcessingTime.setText( | ||
| feeAndProcessingTime.getContext().getString(R.string.feeAndProcessingTimeInformation, formattedFee, | ||
| processingTime.getValue())); | ||
| if (isZeroFeeAvailable(feeAndProcessingTime.getContext(), formattedFee)) { |
There was a problem hiding this comment.
This method has now become too complicated with so many if/else conditions, I recommend refactoring it.
Hint: Perhaps you can handle fee and processing time in separate conditions rather than mixing them up and then call feeAndProcessingTime.setText at the end when you have final value to be displayed
There was a problem hiding this comment.
moved to common method in FeeFormatter as getFormattedFeeAndProcessingTime()
| mDescriptionFeesAndProcessingTime.setText(mDescriptionFeesAndProcessingTime.getContext() | ||
| .getString(R.string.feeAndProcessingTimeInformation, formattedFee, | ||
| selectionItem.getProcessingTime().getValue())); | ||
| if (isZeroFeeAvailable(mDescriptionFeesAndProcessingTime.getContext(), formattedFee)) { |
There was a problem hiding this comment.
same applies in this method too, too many if/else conditions, please refactor
There was a problem hiding this comment.
moved to common method in FeeFormatter as getFormattedFeeAndProcessingTime()
| feeAndProcessingTime.setText( | ||
| feeAndProcessingTime.getContext().getString(R.string.feeAndProcessingTimeInformation, formattedFee, | ||
| processingTime.getValue())); | ||
| if (isZeroFeeAvailable(feeAndProcessingTime.getContext(), formattedFee)) { |
There was a problem hiding this comment.
too many if/else conditions in this method too, please refactor here too
There was a problem hiding this comment.
moved to common method in FeeFormatter as getFormattedFeeAndProcessingTime()
SaiNarendran
dismissed stale reviews from aparthibanpaypal and themself
via
c10af51
…ttps://github.com/hyperwallet/hyperwallet-android-ui-sdk into task/DTSERWFOUR-553-update-transfer-fee-zero-state
…-fee-zero-state' of https://github.com/hyperwallet/hyperwallet-android-ui-sdk into task/DTSERWFOUR-553-update-transfer-fee-zero-state
No description provided.