Review code

5 files changed, 14 insertions, 1 deletions

diff --git a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/ernb/DummyErnbClient.java b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/ernb/DummyErnbClient.java
index 0c8a2f59..978be4d0 100644
--- a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/ernb/DummyErnbClient.java
+++ b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/ernb/DummyErnbClient.java
@@ -11,6 +11,7 @@ public class DummyErnbClient implements IErnbClient {
 
   @Override
   public ArrayList<RegisterResult> searchWithPersonIdentifer(String personIdentifer) {
+    // NOTE: Are we using Mockito or these fixed strings for testing, why are those defined here?
     switch (personIdentifer) {
       case "a12345":
       case "a12345-":
diff --git a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/DeSpecificDetailSearchProcessor.java b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/DeSpecificDetailSearchProcessor.java
index 51d6952f..b5e8551b 100644
--- a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/DeSpecificDetailSearchProcessor.java
+++ b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/DeSpecificDetailSearchProcessor.java
@@ -10,6 +10,7 @@ public class DeSpecificDetailSearchProcessor extends ICountrySpecificDetailSearc
 
   @Override
   public boolean canHandle(String countryCode, SimpleEidasData eidData) {
+    // NOTE: Please extract constant for "de"
     if (!countryCode.equalsIgnoreCase("de")) {
       return false;
     }
diff --git a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/ICountrySpecificDetailSearchProcessor.java b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/ICountrySpecificDetailSearchProcessor.java
index 3d6b35e9..b9ab2ceb 100644
--- a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/ICountrySpecificDetailSearchProcessor.java
+++ b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/ICountrySpecificDetailSearchProcessor.java
@@ -31,6 +31,7 @@ import org.springframework.beans.factory.annotation.Autowired;
 
 public abstract class ICountrySpecificDetailSearchProcessor {
 
+  // NOTE: Please use constructor injection
 
   protected IErnbClient ernbClient;
   protected IZmrClient zmrClient;
diff --git a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/tasks/InitialSearchTask.java b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/tasks/InitialSearchTask.java
index 1580603c..20ee9325 100644
--- a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/tasks/InitialSearchTask.java
+++ b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/tasks/InitialSearchTask.java
@@ -66,6 +66,8 @@ import java.util.Map;
  */
 @Slf4j
 @Component("InitialSearchTask")
+// NOTE: General: Please rebase git commit and squash them where useful, i.e. "remove unused import" should
+//       not be a separate commit.
 public class InitialSearchTask extends AbstractAuthServletTask {
 
   private List<ICountrySpecificDetailSearchProcessor> handlers = new ArrayList<>();
@@ -92,6 +94,7 @@ public class InitialSearchTask extends AbstractAuthServletTask {
       final ILightResponse eidasResponse = authProcessData
           .getGenericDataFromSession(Constants.DATA_FULL_EIDAS_RESPONSE, ILightResponse.class);
 
+      // NOTE: Why is eidas first converted to a map, and then to a SimpleEidasData?
       final Map<String, Object> simpleAttrMap = convertEidasAttrToSimpleMap(
           eidasResponse.getAttributes().getAttributeMap());
 
@@ -106,14 +109,17 @@ public class InitialSearchTask extends AbstractAuthServletTask {
     }
   }
 
+  // NOTE: Please rename methods ... "step2" doesn't tell the reader anything
   private String step2(SimpleEidasData eidData) throws TaskExecutionException {
     String personIdentifier = eidData.getPseudonym();
+    // NOTE: Is that comment really necessary?
     //search in register(step 2)
     MergedRegisterSearchResult result = searchInZmrAndErnp(personIdentifier);
     if (result.getResultCount() == 0) {
       return step5(result, eidData);
     } else if (result.getResultCount() == 1) {
       return step3(result, eidData);
+      // NOTE: Why is that code commented-out?
     } //else if (result.getResultCount() > 1) {
     throw new TaskExecutionException(pendingReq, "Initial search - Kitt Process necessary.",
         new ManualFixNecessaryException(personIdentifier));
@@ -179,7 +185,7 @@ public class InitialSearchTask extends AbstractAuthServletTask {
     log.debug("Update " + result + " with " + eidData);
     //TODO
 
-
+    // NOTE: Sometimes the bpk is returned, sometimes "105"?
     return result.getBpk();
   }
 
@@ -265,6 +271,7 @@ public class InitialSearchTask extends AbstractAuthServletTask {
 
     log.debug("Automerge " + initialSearchResult + " with " + eidData + " " + mdsSearchResult);
     //TODO
+    // NOTE: Sometimes the bpk is returned, sometimes "105"?
     return "105";
   }
 
@@ -329,6 +336,7 @@ public class InitialSearchTask extends AbstractAuthServletTask {
 
   //just for testing
   //TODO is there a nicer solution?
+  // There is: Constructor Injection see https://reflectoring.io/constructor-injection/ or https://www.baeldung.com/constructor-injection-in-spring
   @Autowired
   public void setErnbClient(IErnbClient ernbClient) {
     this.ernbClient = ernbClient;
@@ -342,6 +350,7 @@ public class InitialSearchTask extends AbstractAuthServletTask {
   @Autowired
   public void setHandlers(List<ICountrySpecificDetailSearchProcessor> handlers) {
     this.handlers = handlers;
+    // NOTE: There's a typo in "registrated"
     log.info("# " + handlers.size() + " country specific detail search services are registrated");
   }
 
diff --git a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/zmr/DummyZmrClient.java b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/zmr/DummyZmrClient.java
index 0f3436d8..3af2e39e 100644
--- a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/zmr/DummyZmrClient.java
+++ b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/zmr/DummyZmrClient.java
@@ -11,6 +11,7 @@ public class DummyZmrClient implements IZmrClient {
 
   @Override
   public ArrayList<RegisterResult> searchWithPersonIdentifer(String personIdentifer) {
+    // NOTE: Are we using Mockito or these fixed strings for testing, why are those defined here?
     switch (personIdentifer) {
       case "a12345":
       case "a12345-":