Credit card validator 66 #11

marcin-kapolka-prv · 2021-02-07T21:00:43Z

marcin kapolka

…tion-of-candidates-task More thorough description of candidate's task

…-description Added straightforward task description

…iption-of-the-task-update Updated straightforward description of the task

marcin-kapolka-prv · 2021-02-07T21:27:42Z

General comments:

The requirement of creating a rest service is not satisfied.

The code submitted is simply a command-line tool. Switch to commonly used frameworks. Suggesting spring boot, since it is widespread over the world and you will find lots of q'n'a s over the internet about it with quality samples. If you want a neat "menu" like in the main() routine, you may extend spring boot with additional lib such as swagger which generates a gui for rest services.

General java project and coding guidelines were not followed.

Package naming is out of the convention. Please put com.worldremit.validator as a base, then use sub packages for appropriate project components.

Place code in src/main/java
Place tests in src/test/java

(You may consider some more up to date than junit - suggesting spock + groovy, which will make the test more readable, actual code docummentation and declarative src/test/groovy)

Instead of ide specific files, use project files for maven (pom.xml) or prefferably more up-to-date build tool "gradle". (the later escpecially if you want to use spring boot).

Git repository best practices were broken.

ide specific files committed
most probably a random gitingnore file uploaded. Please check your ide/github options to generate a valid file for specified language + ide composition.

Forget about eclipse ide :) get a real contemporary ide for jvm languages. It will save you a lot of effort and time. - follow hints from it :)

For details refer subsequent comments.

marcin-kapolka-prv · 2021-02-07T21:28:43Z

.classpath

@@ -0,0 +1,7 @@
+<?xml version="1.0" encoding="UTF-8"?>


remove this file from the repo (ide specific)

marcin-kapolka-prv · 2021-02-07T21:29:47Z

.gitignore

@@ -0,0 +1,23 @@
+# Compiled class file


Regenerate git ignore policies using a good ide (suggested - intellij). You may also want to search the internet for good samples.

marcin-kapolka-prv · 2021-02-07T21:30:10Z

.project

@@ -0,0 +1,17 @@
+<?xml version="1.0" encoding="UTF-8"?>


to be removed from repo

marcin-kapolka-prv · 2021-02-07T21:30:32Z

.settings/org.eclipse.jdt.core.prefs

@@ -0,0 +1,11 @@
+eclipse.preferences.version=1


remove from repo (again git sanitization)

marcin-kapolka-prv · 2021-02-07T21:32:39Z

src/validator/CreditCardChecking.java

@@ -0,0 +1,5 @@
+package validator;


This interface is redundant. It is a bad practice to make an interface while there is only a single implementation. (Performance reasons (enforced virtual method tables = extra work for jit), and simply unnecessary lines of code to maintain).

This interface can be removed. Gives nothing.

marcin-kapolka-prv · 2021-02-07T21:34:27Z

src/validator/CreditCardValidator.java

@@ -0,0 +1,62 @@
+package validator;


Move to com.worldremit.validator package. Optionally you can create also "service" subpackage.

marcin-kapolka-prv · 2021-02-07T21:35:58Z

src/validator/CreditCardValidator.java

+		return result;
+	}
+
+}


Run auto-formatter on this file from your ide; automatically tidy up spaces, indents etc.

marcin-kapolka-prv · 2021-02-07T21:42:04Z

src/validator/CreditCardValidator.java

+
+import java.util.ArrayList;
+
+public class CreditCardValidator implements CreditCardChecking {


Suggesting to redesign this class to a service:

Rename this class to CreditCardValidatorService, drop the redundant interface as stated earlier.

Add and inject credit card vendor name to credit card verification map ex: 'private final Map<String, CreditCardVendor> vendors;'

The service will have a single public responsibility: 'public boolean public boolean checkValidity(String vendorName, String number) throws IllegalArgumentException'. (Should throw on unknown vendor name)

marcin-kapolka-prv · 2021-02-07T21:43:34Z

src/validator/CreditCardValidator.java

+public class CreditCardValidator implements CreditCardChecking {
+
+	@Override
+	public boolean checkValidity(CreditCardVendor ccv, String number) {


This would become private. Could be called if the public method had found the vendor by name. @OverRide obviously to be dropped.

marcin-kapolka-prv · 2021-02-07T21:46:15Z

src/validator/CreditCardValidator.java

+
+	@Override
+	public boolean checkValidity(CreditCardVendor ccv, String number) {
+		ArrayList<Integer> chars = ccv.getNumberOfChars();


Specific type should be only used explicitly if it states its exact purpose. Otherwise use base interfaces (Example: like sorted entries in TreeMap vs Map). These 2 can be simply Lists<> or Collections<>

marcin-kapolka-prv · 2021-02-07T21:51:31Z

src/validator/CreditCardValidator.java

+		ArrayList<Integer> chars = ccv.getNumberOfChars();
+		ArrayList<String> masks = ccv.getIIN();
+
+		boolean result1 = checkLengthCorrectness(number, chars);


These 3 booleans should be named more precisely if they stay. (ex lengthCorrect).

Their existence is actually redundant unless you want to debug the code and step over. In other cases this code will benefit on performance if rewritten to return checkLengthCorrectness(...) && checkIINMaskCorrectness(...) && checkLuhnalgorithmCorrectness(...);

Java && operator evaluation will save us from unnecessary calls and evaluations if we fail earlyin the chain.

marcin-kapolka-prv · 2021-02-07T21:52:28Z

src/validator/CreditCardValidator.java

+		return result1 && result2 && result3;
+	}
+
+	private boolean checkLengthCorrectness(String number, ArrayList<Integer> chars){


List instead of ArrayList (see prev comments)

marcin-kapolka-prv · 2021-02-07T21:52:40Z

src/validator/CreditCardValidator.java

+		return result;
+	}
+
+	private boolean checkIINMaskCorrectness (String number, ArrayList<String> masks){


Same comments as in the previous routine.

marcin-kapolka-prv · 2021-02-07T21:53:37Z

src/validator/CreditCardValidator.java

+	private boolean checkLengthCorrectness(String number, ArrayList<Integer> chars){
+		boolean result = false;
+		for (int i = 0; i<chars.size();i++){
+			if (chars.get(i) == number.length())


Bail out here with return false if not equal; performance

Refactor to : (do not leave code branches without bracketed scopes - potential risk of logic breaking by mistake or some code merges.

if () {
return false;
}

marcin-kapolka-prv · 2021-02-07T21:55:32Z

src/validator/CreditCardValidator.java

+	}
+
+	private boolean checkLengthCorrectness(String number, ArrayList<Integer> chars){
+		boolean result = false;


reduntant flag

marcin-kapolka-prv · 2021-02-07T21:55:44Z

src/validator/CreditCardValidator.java

+			if (chars.get(i) == number.length())
+				result = true;
+		}
+		return result;


return true

marcin-kapolka-prv · 2021-02-07T21:59:15Z

src/validator/CreditCardVendorReading.java

@@ -0,0 +1,7 @@
+package validator;


Another redundant interface (see prev comments). Can be removed from the project.

marcin-kapolka-prv · 2021-02-07T22:01:10Z

src/validator/CreditCardVendor.java

+
+import java.util.ArrayList;
+
+public class CreditCardVendor {


This class should have a better name for what it does. CreditCartVendorParameter perhaps?

If using a new java, use java record. Otherwise, this class should be final/immutable.

Preferably factorize all lists with immutable utils from guava

marcin-kapolka-prv · 2021-02-07T22:03:57Z

src/validator/CreditCardValidator.java

+
+		return result1 && result2 && result3;
+	}
+


The following functions can be extracted to another util class and made public statics perhaps. Suggesting com.worldremit.validator.utils.CCVUtils.

marcin-kapolka-prv · 2021-02-07T22:04:57Z

src/validator/CreditCardValidatorProject.java

@@ -0,0 +1,60 @@
+package validator;


move to com.worldremit.validator

marcin-kapolka-prv · 2021-02-07T22:06:10Z

src/validator/CreditCardValidatorProject.java

+import java.io.InputStreamReader;
+import java.util.ArrayList;
+
+public class CreditCardValidatorProject {


Redesign to a spring boot application starter. Use swagger for "browser gui / menu"

https://www.baeldung.com/swagger-2-documentation-for-spring-rest-api

Rename to CreditCardValidatorAppRunner

Create RestController class which would call upon the CreaditCallValidatorService with the recived request. https://spring.io/guides/tutorials/rest/

Inject the service to that controller.

Create configuration class which will create other beans - CreditCardService, and VendorsLoader and Maping (As mentioned in the serice class)

https://www.baeldung.com/spring-boot-custom-auto-configuration

marcin-kapolka-prv · 2021-02-07T22:10:19Z

src/validator/CreditCardVendor.java

+import java.util.ArrayList;
+
+public class CreditCardVendor {
+	private String name;


marcin-kapolka-prv · 2021-02-07T22:11:32Z

src/validator/CreditCardVendor.java

+
+public class CreditCardVendor {
+	private String name;
+	private ArrayList<Integer> numberOfChars;


can change to private final int[] - will be easier on gc. linear memory, no autoboxing (simply performacne)

marcin-kapolka-prv · 2021-02-07T22:12:04Z

src/validator/CreditCardVendor.java

+public class CreditCardVendor {
+	private String name;
+	private ArrayList<Integer> numberOfChars;
+	private ArrayList<String> IIN;


private final List

marcin-kapolka-prv · 2021-02-07T22:13:32Z

src/validator/CreditCardVendor.java

+		return IIN;
+	}
+
+	public CreditCardVendor(String vendorName, ArrayList<Integer> charsNumbers, ArrayList<String> masks){


stor shoulb be before public methods, after fields.

Optionally, lines with more than 2 parameters can be restructured to 1 line - param mode. easier to compare on diffs, prs, better reflection of data structure which the class represents

marcin-kapolka-prv · 2021-02-07T22:15:02Z

src/validator/CreditCardVendor.java

+		IIN = masks;
+	}
+
+	@Override


Optionally generate == and # code contracts using ide

marcin-kapolka-prv · 2021-02-07T22:16:23Z

src/validator/Tests.java

@@ -0,0 +1,55 @@
+package validator;


move to appropriate package and tests directory.

marcin-kapolka-prv · 2021-02-07T22:18:29Z

src/validator/Tests.java

+
+import org.junit.Test;
+
+public class Tests {


Rename and split into individual test classes associated with parentcode. Ex CreditCardValidatorServiceTest, CreditCardUtilsTest

marcin-kapolka-prv · 2021-02-07T22:20:13Z

src/validator/Tests.java

+
+	@Test
+	public void testLengthOfNumberWhenCorrect() {
+		String number = "5584239583699571";


write the test having "Given" "When" "Then" declarative approach. Ex:

@Test public void testLengthOfNumberWhenIncorrect() {

//Given
String number = "55842395";

//When var result = validator.checkValidity(vendorMasterCard, number); //Then assertFalse(result); }

marcin-kapolka-prv · 2021-02-07T22:21:38Z

src/validator/Tests.java

+	@Test
+	public void testLengthOfNumberWhenIncorrect() {
+		String number = "55842395";
+		assertEquals(false, validator.checkValidity(vendorMasterCard, number));


use assertTrue/assertFalse for clean looks

marcin-kapolka-prv · 2021-02-07T22:27:50Z

src/validator/CreditCardVendorsReader.java

@@ -0,0 +1,58 @@
+package validator;


move to com.worldremit.validator.utils package.

marcin-kapolka-prv · 2021-02-07T22:28:35Z

src/validator/CreditCardVendorsReader.java

+import java.io.IOException;
+import java.util.ArrayList;
+
+public class CreditCardVendorsReader implements CreditCardVendorReading {


Rename to CreditCartVendorParameterLoader, use in the spring configuration bean to load the map of venrods

marcin-kapolka-prv · 2021-02-07T22:29:46Z

src/validator/CreditCardValidator.java

+		return result;
+	}
+
+	private boolean checkLuhnalgorithmCorrectness(String number){


function name casing is wrong - checkLuhnAlgorithmCorrectness

marcin-kapolka-prv · 2021-02-07T22:31:24Z

src/validator/CreditCardValidator.java

+	private boolean checkLuhnalgorithmCorrectness(String number){
+		boolean result = false;
+
+		String[] characters = number.split("");


gnerally inefficient and messy code. It would be easier to rely on this https://gist.github.com/mdp/9691528

(digit parsing is extremely inefficient here, readability of code is also poor)

marcin-kapolka-prv · 2021-02-07T22:33:30Z

src/validator/CreditCardVendorsReader.java

+
+public class CreditCardVendorsReader implements CreditCardVendorReading {
+	@Override
+	public ArrayList<CreditCardVendor> returnVendorArray() {


return Map<String, CCVPArameters> here, rename to loadVendors()

marcin-kapolka-prv · 2021-02-07T22:34:01Z

src/validator/CreditCardVendorsReader.java

+		        list.add(new CreditCardVendor(vendorName,vendorNumbersOfChars,vendorMasks));
+
+		    }
+		} catch (FileNotFoundException e) {


should not consume exceptions during app startup. log the exception and rethrow, the framework will handle rest

marcin-kapolka-prv · 2021-02-07T22:34:14Z

src/validator/CreditCardVendorsReader.java

+	     }
+	}
+
+}


run formatter on this file

marcin-kapolka-prv · 2021-02-07T22:34:53Z

src/validator/CreditCardVendorsReader.java

+	private void parseMasks(String value,  ArrayList<String> vendorMasks){
+	     String[] masks = value.split("/");
+	     for(int i=0;i<masks.length; i++){
+	     	if (masks[i].split("-").length == 1){


inefficient code, do split once, remember it in a var

marcin-kapolka-prv

Apart from the previous comments, this code to be prod ready should use some logging in importat places - incoming requests, data construction, results etc.

Also,application and jvm metrics should be collected and exposed to company's infrastructure. As the first MVP phase to accepted to go live

marcin-kapolka-prv · 2021-02-07T22:39:09Z

Base_of_credit_card_vendors.txt

@@ -0,0 +1,2 @@
+Visa 13/16 4 


This file should be moved to resources, and accessed via LoadResource functions for MVP phase.

Eventually, the app setup should load this from your company's config server infrastructure via spring utils

mrybak and others added 10 commits August 6, 2020 16:20

Recruitment task upload

b3629e8

More thorough description of candidate's task

65afa17

More thorough description of candidate's task - after review

3984d49

Merge pull request Worldremit#4 from Worldremit/more-thorough-descrip…

fd4c027

…tion-of-candidates-task More thorough description of candidate's task

Added straightforward tasks description

46791e8

Straightforward task description - typo fix

f28c99c

Straightforward task description - capital plus bold

9c0cde8

Merge pull request Worldremit#8 from Worldremit/straightforward-tasks…

7cc2f38

…-description Added straightforward task description

Updated straightforward description of the task

7a29ba7

Merge pull request Worldremit#9 from Worldremit/straightforward-descr…

34d4f37

…iption-of-the-task-update Updated straightforward description of the task

marcin-kapolka-prv commented Feb 7, 2021

View reviewed changes

.classpath

@@ -0,0 +1,7 @@

<?xml version="1.0" encoding="UTF-8"?>

Copy link

Author

marcin-kapolka-prv Feb 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this file from the repo (ide specific)

marcin-kapolka-prv commented Feb 7, 2021

View reviewed changes

.project

@@ -0,0 +1,17 @@

<?xml version="1.0" encoding="UTF-8"?>

Copy link

Author

marcin-kapolka-prv Feb 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be removed from repo

marcin-kapolka-prv commented Feb 7, 2021

View reviewed changes

src/validator/CreditCardValidator.java

if (chars.get(i) == number.length())

result = true;

}

return result;

Copy link

Author

marcin-kapolka-prv Feb 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return true

marcin-kapolka-prv commented Feb 7, 2021

View reviewed changes

src/validator/CreditCardValidatorProject.java

@@ -0,0 +1,60 @@

package validator;

Copy link

Author

marcin-kapolka-prv Feb 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to com.worldremit.validator

marcin-kapolka-prv commented Feb 7, 2021

View reviewed changes

src/validator/CreditCardVendor.java

import java.util.ArrayList;

public class CreditCardVendor {

private String name;

Copy link

Author

marcin-kapolka-prv Feb 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final

marcin-kapolka-prv commented Feb 7, 2021

View reviewed changes

src/validator/CreditCardVendor.java

IIN = masks;

}

@Override

Copy link

Author

marcin-kapolka-prv Feb 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optionally generate == and # code contracts using ide

marcin-kapolka-prv commented Feb 7, 2021

View reviewed changes

src/validator/Tests.java

@@ -0,0 +1,55 @@

package validator;

Copy link

Author

marcin-kapolka-prv Feb 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to appropriate package and tests directory.

marcin-kapolka-prv commented Feb 7, 2021

View reviewed changes

src/validator/CreditCardVendorsReader.java

}

}

}

Copy link

Author

marcin-kapolka-prv Feb 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

run formatter on this file

marcin-kapolka-prv commented Feb 7, 2021

View reviewed changes


		import java.util.ArrayList;

		public class CreditCardValidator implements CreditCardChecking {

+              	     }
+              	}
+              }

Credit card validator 66 #11

Are you sure you want to change the base?

Credit card validator 66 #11

Conversation

marcin-kapolka-prv commented Feb 7, 2021

marcin-kapolka-prv commented Feb 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcin-kapolka-prv Feb 7, 2021 • edited Loading

Choose a reason for hiding this comment

marcin-kapolka-prv Feb 7, 2021 • edited Loading

Choose a reason for hiding this comment

marcin-kapolka-prv Feb 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcin-kapolka-prv Feb 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcin-kapolka-prv Feb 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcin-kapolka-prv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcin-kapolka-prv Feb 7, 2021 •

edited

Loading

marcin-kapolka-prv Feb 7, 2021 •

edited

Loading

marcin-kapolka-prv Feb 7, 2021 •

edited

Loading

marcin-kapolka-prv Feb 7, 2021 •

edited

Loading

marcin-kapolka-prv Feb 7, 2021 •

edited

Loading