I am learning Clean Code and Refactoring. And my project has many long method with view access, something like this.
private fun updateStatusFragmentUI(statusData: StatusListData.StatusData) =
with(binding.detail) {
with(statusData) {
tvCodeDoc.text = getString(R.string.reception_code).format(receptionKey)
tvTime.text = createdAt.toFullTimeString()
tvName.text = name
tvContact.text = phoneNumber
tvAddress.text = address
tvAddressOld.text = addressOld
btnSubAction.isVisible = false
contentsRead.root.isVisible = false
contentsWrite.root.isVisible = false
footerAction.root.isVisible = false
footerInformation.root.isVisible = false
when (detailStatus) {
ReceptionUiType.PRICE_REQUEST -> {
btnSubAction.setText(R.string.reception_cancel)
btnSubAction.isVisible = viewModel.isCancelButtonVisible
btnSubAction.setOnClickListener {
showDialogReceptionCancel(false)
}
setWriteContents(this)
setActionFooter(R.string.all_send_price)
}
ReceptionUiType.PAYMENT_WAIT -> {
btnSubAction.setText(R.string.reception_cancel)
btnSubAction.isVisible = viewModel.isCancelButtonVisible
btnSubAction.setOnClickListener {
showDialogReceptionCancel(false)
}
setReadContents(this)
contentsRead.clDuration.isVisible = false
setInformationFooter(R.string.include_detail_info_payment_method_title,
R.string.include_detail_info_payment_method_sub_title
)
}
ReceptionUiType.DISPENSING_WAIT -> {
btnSubAction.setText(R.string.dialog_dispensing_cancel_action)
btnSubAction.isVisible = false //notice 조제 대기 상태에서는 취소를 표시하지않음!
btnSubAction.setOnClickListener {
showDialogReceptionCancel(true)
}
setReadContents(this)
contentsRead.clDuration.isVisible = false
setInformationFooter(
titleRes = R.string.include_detail_info_dispensing_wait_title,
isOnConfirm = false)
setActionFooter(
R.string.include_detail_action_dispensing_wait,
isDispensingStart = true
)
}
ReceptionUiType.IN_DISPENSING -> {
btnSubAction.setText(R.string.dialog_dispensing_cancel_action)
btnSubAction.setOnClickListener {
showDialogDispensingCancel()
}
setReadContents(this)
setInformationFooter(R.string.include_detail_info_in_dispensing_title, R.string.include_detail_info_in_dispensing_sub_title,
isOnConfirm = false)
setActionFooter(R.string.include_detail_action_in_dispensing)
}
ReceptionUiType.DELIVERY_WAIT -> {
setReadContents(this)
// val subTitle = if (deliveryMethod == DeliveryMethodType.PARCEL) null else R.string.include_detail_info_delivery_wait_sub_title
setInformationFooter(R.string.include_detail_info_delivery_wait_title,
isOnConfirm = false)
setActionFooter(
R.string.include_detail_action_delivery_wait,
isDeliveryPayment = deliveryMethod == DeliveryMethodType.PARCEL
)
}
ReceptionUiType.DELIVERY_PAYMENT_WAIT -> {
setReadContents(this)
setInformationFooter(R.string.include_detail_info_waiting_payment_for_delivery_title,
R.string.include_detail_info_waiting_payment_for_delivery_sub_title,
isWaiting = true
)
}
ReceptionUiType.DELIVERY_PAYMENT_WAIT_FAIL -> {
setReadContents(this)
contentsRead.clDuration.isVisible = false
setInformationFooter(title = getString(R.string.include_detail_info_delivery_payment_wait_fail_title),
sub = getString(R.string.include_detail_info_delivery_payment_wait_fail_sub_title),
additionalInfo = getString(R.string.include_detail_info_delivery_payment_wait_time_out),
isOnConfirm = false
)
}
ReceptionUiType.DELIVERY_RECEPTION_WAIT -> {
setReadContents(this)
setInformationFooter(R.string.include_detail_info_delivery_reception_wait_quick_title,
R.string.all_please_wait,
isOnConfirm = false,
isWaiting = true
)
}
ReceptionUiType.DELIVERY_RECEPTION_REQUEST_PARCEL -> {
setReadContents(this)
setActionFooter(R.string.include_detail_info_delivery_reservation_wait_parcel_reception_completed,
isParcelReady = true
)
}
// ReceptionUiType.PICKUP_WAIT -> {
// setReadContents()
// setActionFooter(R.string.include_detail_action_pickup_wait, false, R.string.include_detail_action_pickup_wait_title, R.string.include_detail_action_pickup_wait_sub_title)
// }
ReceptionUiType.DELIVERY_RECEPTION_COMPLETED -> {
setReadContents(this)
val dispensingCompleteAt =
this.endAt.toFullTimeString().takeIf { it.isNotEmpty() }
val deliveryCompletedAt =
this.deliveryAt.toFullTimeString().takeIf { it.isNotEmpty() }
val sub =
if (dispensingCompleteAt != null && deliveryCompletedAt != null) getString(
R.string.include_detail_info_delivery_completed_sub_title).format(
dispensingCompleteAt,
deliveryCompletedAt) else null
setInformationFooter(getString(R.string.include_detail_info_delivery_completed_title),
sub)
}
ReceptionUiType.DELIVERY_COMPLETED_IN_PERSON -> {
setReadContents(this)
val dispensingCompleteAt =
this.endAt.toFullTimeString().takeIf { it.isNotEmpty() }
val deliveryCompletedAt =
this.deliveryAt.toFullTimeString().takeIf { it.isNotEmpty() }
val sub =
if (dispensingCompleteAt != null && deliveryCompletedAt != null) getString(
R.string.include_detail_info_delivery_completed_in_person_sub_title).format(
dispensingCompleteAt,
deliveryCompletedAt) else null
setInformationFooter(getString(R.string.include_detail_info_delivery_completed_in_person_title),
sub)
}
ReceptionUiType.DELIVERY_RECEPTION_COMPLETED_PARCEL -> {
setReadContents(this)
val dispensingCompleteAt =
this.endAt.toFullTimeString().takeIf { it.isNotEmpty() }
val deliveryCompletedAt =
this.deliveryAt.toFullTimeString().takeIf { it.isNotEmpty() }
val sub =
if (dispensingCompleteAt != null && deliveryCompletedAt != null) getString(
R.string.include_detail_info_delivery_completed_parcel_sub_title).format(
dispensingCompleteAt,
deliveryCompletedAt) else null
setInformationFooter(getString(R.string.include_detail_info_delivery_completed_parcel_title),
sub)
}
// ReceptionUiType.PICKUP_COMPLETE -> {
// setReadContents()
// setInformationFooter(R.string.include_detail_info_pickup_complete_title)
// }
ReceptionUiType.RECEPTION_CANCEL -> {
setReadContents(this)
contentsRead.clDuration.isVisible = false
val canceledAt =
this.canceledAt.toFullTimeString().takeIf { it.isNotEmpty() }
setInformationFooter(title = getString(R.string.include_detail_info_purchase_cancel_title),
sub = String.format(getString(R.string.include_detail_info_purchase_cancel_sub_title),
failReason),
additionalInfo = String.format(getString(R.string.include_detail_info_reception_cancel_sub_title),
canceledAt),
isOnConfirm = false
)
}
ReceptionUiType.RECEPTION_CUSTOMER_CANCEL -> {
setReadContents(this)
contentsRead.clDuration.isVisible = false
val canceledAt =
this.canceledAt.toFullTimeString().takeIf { it.isNotEmpty() }
setInformationFooter(title = getString(R.string.include_detail_info_customer_cancel_title),
sub = "고객 취소 $canceledAt",
isOnConfirm = false
)
}
ReceptionUiType.RECEPTION_PAYMENT_TIME_OUT -> {
setReadContents(this)
contentsRead.clDuration.isVisible = false
val canceledAt =
this.canceledAt.toFullTimeString().takeIf { it.isNotEmpty() }
setInformationFooter(title = getString(R.string.include_detail_info_reception_payment_time_out_title),
sub = getString(R.string.include_detail_info_reception_payment_time_out_sub_title).format(
canceledAt),
isOnConfirm = false
)
}
ReceptionUiType.DELIVERY_CANCEL -> {
setReadContents(this)
val canceledAt =
this.canceledAt.toFullTimeString().takeIf { it.isNotEmpty() }
setInformationFooter(getString(R.string.include_detail_info_delivery_cancel_title),
sub = "배송 취소 $canceledAt",
isOnConfirm = false
)
}
ReceptionUiType.DISPENSING_CANCEL -> {
setReadContents(this)
val canceledAt =
this.canceledAt.toFullTimeString().takeIf { it.isNotEmpty() }
setInformationFooter(title = getString(R.string.include_detail_info_dispensing_cancel_title),
sub = "조제 취소 $canceledAt",
isOnConfirm = false
)
}
ReceptionUiType.CANCEL -> {
setReadContents(this)
val sub = failReason.takeIf { it.isNotEmpty() }?.let { reason ->
this.canceledAt.toFullTimeString().takeIf { it.isNotEmpty() }?.let {
"$reason\n$it"
} ?: reason
}
setInformationFooter(getString(R.string.include_detail_info_cancel_title),
sub,
isOnConfirm = false
)
}
ReceptionUiType.PAYMENT_REFUND -> {
setReadContents(this)
setInformationFooter(
title = getString(R.string.include_detail_info_refunding_title),
sub = getString(R.string.include_detail_info_refunding_sub_title),
additionalInfo = getString(R.string.include_detail_info_delivery_payment_wait_time_out),
isOnConfirm = false
)
}
else -> {
// no-op
}
}
}
}
I learnt replace conditional with polymorphism and I have no idea how to apply this technique. How can I solve this and if it is possible to create classes, How can I name them? I am thinking like DetailViewManager, StatusUiManager, etc. But it doesn't seem right.
CodePudding user response：
(This got long with the code examples, but I hope it gives you some ideas!)
The polymorphism approach in your link is basically what you have now, just with the code for each case put in a function in a class. So instead of this:
when (detailStatus) {
ReceptionUiType.PRICE_REQUEST -> {
btnSubAction.setText(R.string.reception_cancel)
btnSubAction.isVisible = viewModel.isCancelButtonVisible
btnSubAction.setOnClickListener {
showDialogReceptionCancel(false)
}
setWriteContents(this)
setActionFooter(R.string.all_send_price)
}
// etc
you'd have something like this:
sealed class ReceptionUiType {
// we need to pass something in here - I'll talk about it below!
abstract fun doStuff(activity: MyActivity)
class PriceRequest : ReceptionUiType() {
override fun doStuff(activity: MyActivity) {
with(activity) {
btnSubAction.setText(R.string.reception_cancel)
btnSubAction.isVisible = viewModel.isCancelButtonVisible
btnSubAction.setOnClickListener {
showDialogReceptionCancel(false)
}
setWriteContents(this)
setActionFooter(R.string.all_send_price)
}
}
}
// add the classes for the other types
}
And then you could do this:
detailStatus.doThing(activity)
and whichever subclass of ReceptionUiType detailStatus happens to be, it has a doThing method that does something specific to that type. So instead of checking which type detailStatus is, and acting differently depending on the result, you're moving the code into the type itself, and saying "whatever you are, do your thing".
But I don't think that's a good fit here, considering how closely tied into your Activity or Fragment that code is. I'm passing in the Activity in the example because the code needs to call methods on it and poke at its UI (I don't know where it's actually running, this is just an example!). So that code isn't a neat separation of concerns - all these UI implementation details are spilling over into your ReceptionUiType classes!
Instead, you might want to try and keep data in your ReceptionUiType classes, and let your when statement handle doing something with that data, i.e. all the method calls, the UI setup, etc. That's not stuff your UiType classes should know anything about, right? They just need to represent some data, and the other component uses it - ideally without too much repetition!
So I think a good first approach is to tackle that repetition in the code blocks for each case. What do they all have in common? What do some of them have in common? If you can start to break things down, you can start to group things, and then handle them as a group:
One way (that won't work here, but as a general example) is to create a class (or a data class) that has properties for each bit of data you might need to handle:
data class ReceptionUiType(
// these are nullable, so you can have "no data" for a particular property
val buttonTextResId: Int?,
val buttonVisible: Boolean?,
// etc
)
Then instead of handling individual types, you can just set things depending on which bits of data are included:
// or ?.let { btnSubAction.setText(it) } - I think this is neater and clearer though!
detailStatus.buttonTextResId?.run(btnSubAction::setText)
detailStatus.buttonVisible?.run(btnSubAction::isVisible)
This way instead of having to define a case for each type and explicitly lay out what happens each time (which is sometimes good and helpful!) you can just handle each piece of data - if you have it, do the relevant thing with it. If you don't, skip to the next thing!
But your code is more complicated than that, with a lot of different things possibly happening, and data for each. So you could end up with a single data class that covers too much, too many different unrelated things, and then it makes sense to split them up into more specific classes for a few different use cases.
Looking at your code, it feels like there are some broad groups each case can fall into:
- stuff that affects the button / shows the cancel dialog
- stuff related to delivery completion
- stuff related to cancellation
and those affect things like whether a cancellation time is handled, whether a delivery time is handled, etc. Behaviour that doesn't exist in every group, but it's common to the cases in that group.
There's also stuff that applies to most cases (but not all) like the information footer.
So there are two approaches you could try - build a class hierarchy that creates groups with shared properties/behaviour, or use composition to define each bit of behaviour, and add them to each type as appropriate.
Here's how the hierarchy one would look:
sealed class ReceptionUiType {
// common to (mostly) everything
abstract val informationFooterTitleId: Int?
sealed class ButtonToucher: ReceptionUiType() {
// common to the ButtonToucher group
abstract val buttonTextId: Int
class PriceRequest : ButtonToucher() {
override val buttonTextId: Int = R.string.reception_cancel
override val informationFooterTitleId: Int? = null
}
}
}
Then your handler code can do things like:
// general stuff
detailsStatus.informationFooterTitleId?.let {
setInformationFooter(titleRes = it)
}
// group-specific stuff
if (detailStatus is ReceptionUiType.ButtonToucher) {
// set up the button using the specific properties on this subtype
btnSubAction.setText(detailStatus.buttonTextId)
// etc
}
That kind of thing can work when you have neat groups where some things are definitely a more specific type of another thing, like inheritance in general. A class can only have one parent superclass, and sometimes that's enough to describe the way behaviour and properties are shared.
But sometimes it's messier, and a type can share some of the behaviour and properties of another class, but also some from another class, and there's no obvious hierarchy there. So instead, it makes more sense to define things with interfaces, and combine those to compose a class's behaviour and properties.
I think this is the way you should go given how interconnected some of these cases are in some ways, but not in others. So something like this:
interface ButtonToucher {
val buttonTextId: Int
// this could be a nullable Boolean? but I wanted to show that you can
// define your data like this, with things like enums to represent states
val buttonVisibility: ButtonVisibility
val showCancelDialog: Boolean
enum class ButtonVisibility { VISIBLE, INVISIBLE, FOLLOW_VIEWMODEL }
}
interface SetsAction {
val actionTextId: Int
}
interface SetsInformation {
val informationTextId: Int
// other properties - you have a lot! make them nullable and null here by default
val isOnConfirm: Boolean? = null
}
// once you have this setup, you can define your types by combining the interfaces they use
sealed class ReceptionUiType {
PriceRequest : ReceptionUiType(), ButtonToucher, SetsAction {
override val buttonTextId = R.string.reception_cancel
}
}
// and interact with them as those types, checking for each type and doing the necessary stuff:
if (detailsStatus is ButtonToucher) {
// set up the button
}
A similar approach is to combine this with the data class idea above - but instead of having one data class with lots of properties, some of which won't be present in combination with other properties, you could create a structure that holds other data structures (I'd prefer this over interfaces I think):
data class ButtonConfig(
val buttonTextId: Int,
val visibility: ButtonVisibility?,
val showCancelDialog
)
data class InformationFooterConfig(
// all the properties you might need
)
// then define a class that may or may not contain these objects
sealed class ReceptionUiType(
val buttonConfig: ButtonConfig? = null,
val informationFooterConfig: InformationFooterConfig? = null,
// etc
) {
class PriceRequest : ReceptionUiType(
buttonConfig = ButtonConfig(
R.string.reception_cancel,
ButtonConfig.ButtonVisibility.FOLLOW_VIEWMODEL,
showCancelDialog = false
)
informationFooterConfig = InformationFooterConfig(
// define these details
)
)
}
And then you can check for those config objects:
detailsStatus.buttonConfig?.let { config ->
// set up the button since we have a config for it
}
// You can still check types if it makes sense for certain things
// For instance, setReadContents seems to happen every time EXCEPT in one case
if (detailsStatus is ReceptionUiType.PriceRequest) setWriteContents(this)
else setReadContents(this)
The nice thing about this approach is you can create an instance of ButtonConfig and reuse it, like PRICE_REQUEST and PAYMENT_WAIT seem to handle the button in the same way, so you could do val paymentButtonConfig = // that button config up there and pass that into your constructors, so you only need to define it once (and can easily make changes since you're not repeating yourself)
I hope that makes sense! Your example seems fairly complicated and needs to be broken down and organised, to see what stuff you can combine, what needs to be applied to everything, etc. So I just wanted to give you an overview of how you could approach it