TRUNK-5748: Refactor code smells in openmrs-core

api/src/main/java/org/openmrs/Allergies.java

@@ -31,7 +31,7 @@ public class Allergies implements List<Allergy> {
 
 	private String allergyStatus = UNKNOWN;
 
-	private List<Allergy> allergies = new ArrayList<>();
+	private List<Allergy> allergyList = new ArrayList<>();
 
 	/**
 	 * @return the allergyStatus
@@ -44,12 +44,12 @@ public String getAllergyStatus() {
 	public boolean add(Allergy allergy) {
 		throwExceptionIfHasDuplicateAllergen(allergy);
 		allergyStatus = SEE_LIST;
-		return allergies.add(allergy);
+		return allergyList.add(allergy);
 	}
 
 	public boolean remove(Allergy allergy) {
-		boolean result = allergies.remove(allergy);
-		if (allergies.isEmpty()) {
+		boolean result = allergyList.remove(allergy);
+		if (allergyList.isEmpty()) {
 			allergyStatus = UNKNOWN;
 		}
 		return result;
@@ -58,11 +58,11 @@ public boolean remove(Allergy allergy) {
 	@Override
 	public void clear() {
 		allergyStatus = UNKNOWN;
-		allergies.clear();
+		allergyList.clear();
 	}
 
 	public void confirmNoKnownAllergies() {
-		if (!allergies.isEmpty()) {
+		if (!allergyList.isEmpty()) {
 			throw new APIException("Cannot confirm no known allergies if allergy list is not empty");
 		}
 		allergyStatus = NO_KNOWN_ALLERGIES;
@@ -73,7 +73,7 @@ public void confirmNoKnownAllergies() {
 	 */
 	@Override
 	public Iterator<Allergy> iterator() {
-		return allergies.iterator();
+		return allergyList.iterator();
 	}
 
 	/**
@@ -82,7 +82,7 @@ public Iterator<Allergy> iterator() {
 	@Override
 	public void add(int index, Allergy element) {
 		throwExceptionIfHasDuplicateAllergen(element);
-		allergies.add(index, element);
+		allergyList.add(index, element);
 		allergyStatus = SEE_LIST;
 	}
 
@@ -96,7 +96,7 @@ public boolean addAll(Collection<? extends Allergy> c) {
 			throwExceptionIfHasDuplicateAllergen(allergy);
 		}
 		allergyStatus = SEE_LIST;
-		return allergies.addAll(c);
+		return allergyList.addAll(c);
 	}
 
 	/**
@@ -109,80 +109,80 @@ public boolean addAll(int index, Collection<? extends Allergy> c) {
 			throwExceptionIfHasDuplicateAllergen(allergy);
 		}
 		allergyStatus = SEE_LIST;
-		return allergies.addAll(index, c);
+		return allergyList.addAll(index, c);
 	}
 
 	/**
 	 * @see java.util.List#contains(java.lang.Object)
 	 */
 	@Override
 	public boolean contains(Object o) {
-		return allergies.contains(o);
+		return allergyList.contains(o);
 	}
 
 	/**
 	 * @see java.util.List#containsAll(java.util.Collection)
 	 */
 	@Override
 	public boolean containsAll(Collection<?> c) {
-		return allergies.containsAll(c);
+		return allergyList.containsAll(c);
 	}
 
 	/**
 	 * @see java.util.List#get(int)
 	 */
 	@Override
 	public Allergy get(int index) {
-		return allergies.get(index);
+		return allergyList.get(index);
 	}
 
 	/**
 	 * @see java.util.List#indexOf(java.lang.Object)
 	 */
 	@Override
 	public int indexOf(Object o) {
-		return allergies.indexOf(o);
+		return allergyList.indexOf(o);
 	}
 
 	/**
 	 * @see java.util.List#isEmpty()
 	 */
 	@Override
 	public boolean isEmpty() {
-		return allergies.isEmpty();
+		return allergyList.isEmpty();
 	}
 
 	/**
 	 * @see java.util.List#lastIndexOf(java.lang.Object)
 	 */
 	@Override
 	public int lastIndexOf(Object o) {
-		return allergies.lastIndexOf(o);
+		return allergyList.lastIndexOf(o);
 	}
 
 	/**
 	 * @see java.util.List#listIterator()
 	 */
 	@Override
 	public ListIterator<Allergy> listIterator() {
-		return allergies.listIterator();
+		return allergyList.listIterator();
 	}
 
 	/**
 	 * @see java.util.List#listIterator(int)
 	 */
 	@Override
 	public ListIterator<Allergy> listIterator(int index) {
-		return allergies.listIterator(index);
+		return allergyList.listIterator(index);
 	}
 
 	/**
 	 * @see java.util.List#remove(int)
 	 */
 	@Override
 	public Allergy remove(int index) {
-		Allergy allergy = allergies.remove(index);
-		if (allergies.isEmpty()) {
+		Allergy allergy = allergyList.remove(index);
+		if (allergyList.isEmpty()) {
 			allergyStatus = UNKNOWN;
 		}
 		return allergy;
@@ -193,8 +193,8 @@ public Allergy remove(int index) {
 	 */
 	@Override
 	public boolean remove(Object o) {
-		Boolean removed = allergies.remove(o);
-		if (allergies.isEmpty()) {
+		Boolean removed = allergyList.remove(o);
+		if (allergyList.isEmpty()) {
 			allergyStatus = UNKNOWN;
 		}
 		return removed;
@@ -205,8 +205,8 @@ public boolean remove(Object o) {
 	 */
 	@Override
 	public boolean removeAll(Collection<?> c) {
-		boolean changed = allergies.removeAll(c);
-		if (allergies.isEmpty()) {
+		boolean changed = allergyList.removeAll(c);
+		if (allergyList.isEmpty()) {
 			allergyStatus = UNKNOWN;
 		}
 		return changed;
@@ -217,8 +217,8 @@ public boolean removeAll(Collection<?> c) {
 	 */
 	@Override
 	public boolean retainAll(Collection<?> c) {
-		boolean changed = allergies.retainAll(c);
-		if (allergies.isEmpty()) {
+		boolean changed = allergyList.retainAll(c);
+		if (allergyList.isEmpty()) {
 			allergyStatus = UNKNOWN;
 		}
 		return changed;
@@ -230,39 +230,39 @@ public boolean retainAll(Collection<?> c) {
 	@Override
 	public Allergy set(int index, Allergy element) {
 		allergyStatus = SEE_LIST;
-		return allergies.set(index, element);
+		return allergyList.set(index, element);
 	}
 
 	/**
 	 * @see java.util.List#size()
 	 */
 	@Override
 	public int size() {
-		return allergies.size();
+		return allergyList.size();
 	}
 
 	/**
 	 * @see java.util.List#subList(int, int)
 	 */
 	@Override
 	public List<Allergy> subList(int fromIndex, int toIndex) {
-		return allergies.subList(fromIndex, toIndex);
+		return allergyList.subList(fromIndex, toIndex);
 	}
 
 	/**
 	 * @see java.util.List#toArray()
 	 */
 	@Override
 	public Object[] toArray() {
-		return allergies.toArray();
+		return allergyList.toArray();
 	}
 
 	/**
 	 * @see java.util.List#toArray(T[])
 	 */
 	@Override
 	public <T> T[] toArray(T[] a) {
-		return allergies.toArray(a);
+		return allergyList.toArray(a);
 	}
 
 	/**
@@ -272,7 +272,7 @@ public <T> T[] toArray(T[] a) {
 	 * @return the allergy with a matching id
 	 */
 	public Allergy getAllergy(Integer allergyId) {
-		for (Allergy allergy : allergies) {
+		for (Allergy allergy : allergyList) {
 			if (OpenmrsUtil.nullSafeEquals(allergy.getAllergyId(), allergyId)) {
 				return allergy;
 			}
@@ -288,7 +288,7 @@ public Allergy getAllergy(Integer allergyId) {
 	 * @param allergy the given allergy whose allergen to compare with
 	 */
 	private void throwExceptionIfHasDuplicateAllergen(Allergy allergy) {
-		throwExceptionIfHasAllergen(allergy, allergies);
+		throwExceptionIfHasAllergen(allergy, allergyList);
 	}
 
 	/**
@@ -342,6 +342,6 @@ public boolean containsAllergen(Allergy allergy, Collection<? extends Allergy> a
 	 * @return true if the same allergen exists, else false
 	 */
 	public boolean containsAllergen(Allergy allergy) {
-		return containsAllergen(allergy, allergies);
+		return containsAllergen(allergy, allergyList);
 	}
 }

api/src/main/java/org/openmrs/Allergy.java

@@ -111,7 +111,7 @@ public void setAllergyId(Integer allergyId) {
 	 */
 	@Override
 	public Integer getId() {
-		return allergyId;
+		return getAllergyId();
 	}
 
 	/**

api/src/main/java/org/openmrs/AllergyReaction.java

@@ -79,7 +79,7 @@ public void setAllergyReactionId(Integer allergyReactionId) {
 	 */
 	@Override
 	public Integer getId() {
-		return allergyReactionId;
+		return getAllergyReactionId();
 	}
 
 	/**

api/src/main/java/org/openmrs/CohortMembership.java

@@ -197,7 +197,7 @@ public int compareTo(CohortMembership o) {
 	 */
 	@Override
 	public boolean equals(Object otherCohortMembershipObject) {
-		if (otherCohortMembershipObject == null || !(otherCohortMembershipObject instanceof CohortMembership)) {
+		if (!(otherCohortMembershipObject instanceof CohortMembership)) {
 			return false;
 		}
 		CohortMembership otherCohortMembership = (CohortMembership) otherCohortMembershipObject;

api/src/main/java/org/openmrs/Concept.java

@@ -931,35 +931,83 @@
 			log.debug("Getting shortest conceptName for locale: " + locale);
 		}
 
+		// Early return if explicit short name exists
 		ConceptName shortNameInLocale = getShortNameInLocale(locale);
 		if (shortNameInLocale != null) {
 			return shortNameInLocale;
 		}
 
+		// Extract Method — logic moved to dedicated private method
+		ConceptName shortestNameForLocale = findShortestNameForLocale(locale);
+		ConceptName shortestNameForConcept = findShortestNameForConcept();
+
+		// Decompose Conditional — exact match handling
+		return resolveShortestName(exact, shortestNameForLocale,
+			shortestNameForConcept, locale);
+	}
+
+	/**
+	 * Finds the shortest name matching the given locale exactly.
+	 * Extracted from getShortestName() to reduce cognitive complexity.
+	 *
+	 * @param locale the locale to match
+	 * @return the shortest ConceptName in the given locale, or null
+	 */
+	private ConceptName findShortestNameForLocale(Locale locale) {
+		if (locale == null) {
+			return null;
+		}
 		ConceptName shortestNameForLocale = null;
-		ConceptName shortestNameForConcept = null;
+		for (ConceptName possibleName : getNames()) {
+			// Introduce Explaining Variable — complex condition broken into readable parts
+			boolean isSameLocale = possibleName.getLocale().equals(locale);
+			boolean isShorterThanCurrent = shortestNameForLocale == null
+				|| possibleName.getName().length() < shortestNameForLocale.getName().length();
+			if (isSameLocale && isShorterThanCurrent) {
+				shortestNameForLocale = possibleName;
+			}
+		}
+		return shortestNameForLocale;
+	}
 
-		if (locale != null) {
-			for (ConceptName possibleName : getNames()) {
-				if (possibleName.getLocale().equals(locale) && ((shortestNameForLocale == null)
-				        || (possibleName.getName().length() < shortestNameForLocale.getName().length()))) {
-					shortestNameForLocale = possibleName;
-				}
-				if ((shortestNameForConcept == null)
-				        || (possibleName.getName().length() < shortestNameForConcept.getName().length())) {
-					shortestNameForConcept = possibleName;
-				}
+	/**
+	 * Finds the shortest name across all locales.
+	 * Extracted from getShortestName() to reduce cognitive complexity.
+	 *
+	 * @return the shortest ConceptName across all locales, or null
+	 */
+	private ConceptName findShortestNameForConcept() {
+		ConceptName shortestNameForConcept = null;
+		for (ConceptName possibleName : getNames()) {
+			// Introduce Explaining Variable — complex condition broken into readable parts
+			boolean isShorterThanCurrent = shortestNameForConcept == null
+				|| possibleName.getName().length() < shortestNameForConcept.getName().length();
+			if (isShorterThanCurrent) {
+				shortestNameForConcept = possibleName;
 			}
 		}
+		return shortestNameForConcept;
+	}
 
+	/**
+	 * Resolves which name to return based on exact flag.
+	 * Extracted from getShortestName() to decompose conditional logic.
+	 *
+	 * @param exact whether to return exact locale match only
+	 * @param shortestNameForLocale shortest name in the locale
+	 * @param shortestNameForConcept shortest name across all locales
+	 * @param locale the locale used for warning message
+	 * @return the appropriate ConceptName
+	 */
+	private ConceptName resolveShortestName(Boolean exact, ConceptName shortestNameForLocale,
+											ConceptName shortestNameForConcept, Locale locale) {
 		if (exact) {
 			if (shortestNameForLocale == null) {
-				log.warn(
-				    "No short concept name found for concept id " + conceptId + " for locale " + locale.getDisplayName());
+				log.warn("No short concept name found for concept id "
+					+ conceptId + " for locale " + locale.getDisplayName());
 			}
 			return shortestNameForLocale;
 		}
-
 		return shortestNameForConcept;
 	}