reduce method complexity in openmrs-core

Hello!

methods in openmrs-core service implementation have a tendency to be long and complex, they are hard to read and thus maintain.

I wonder if you would be in favor of refactorings such as the following:

take PatientServiceImpl.savePatient(Patient)

it does a lot of things. If we would extract the related parts into smaller private methods we have the opportunity to give longer meaningful names and turn it into something like this:

public Patient savePatient(Patient patient) throws APIException {
	requireAppropriatePatientModificationPrivilege(patient);
	
	if (!patient.isVoided()) {
		checkPatientIdentifiers(patient);
	}

	setPreferredPatientIdentifier(patient);
	setPreferredPatientName(patient);
	setPreferredPatientAddress(patient);
	
	return dao.savePatient(patient);
}

and Im sure by doing that we can also simplify the code we extracted into the private methods since we’ll see things more clearly as the methods would then do less.

I know touching code is always a risk, but we do trust our tests, right :wink:

I think such improvements will make it easier for everyone to read the code and thus maintain it.

Should I create issues related to such refactorings? What do you say?

1 Like

I do support refactoring by extracting private methods. I wouldn’t try to simplify extracted methods in the same pass to make reviews easier and lower the risk of breaking things.

Thanks for the initiative!

I like this kind of refactoring and i also agree with @raff So @teleivo, go ahead and create the issues. :slight_smile:

good point! I will add this restriction to the issues.

I agree 100%

I agree 10%.

(the other 90% of my agreement is private)

1 Like