Skip to content
This repository has been archived by the owner on Jul 14, 2023. It is now read-only.

Credit card validator 66 #11

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
7 changes: 7 additions & 0 deletions .classpath
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Author

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)

<classpath>
<classpathentry kind="src" path="src"/>
<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-1.8"/>
<classpathentry kind="con" path="org.eclipse.jdt.junit.JUNIT_CONTAINER/4"/>
<classpathentry kind="output" path="bin"/>
</classpath>
23 changes: 23 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# Compiled class file
Copy link
Author

Choose a reason for hiding this comment

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

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

*.class

# Log file
*.log

# BlueJ files
*.ctxt

# Mobile Tools for Java (J2ME)
.mtj.tmp/

# Package Files #
*.jar
*.war
*.nar
*.ear
*.zip
*.tar.gz
*.rar

# virtual machine crash logs, see http://www.java.com/en/download/help/error_hotspot.xml
hs_err_pid*
17 changes: 17 additions & 0 deletions .project
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Author

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

<projectDescription>
<name>CreditCardValidatorProject</name>
<comment></comment>
<projects>
</projects>
<buildSpec>
<buildCommand>
<name>org.eclipse.jdt.core.javabuilder</name>
<arguments>
</arguments>
</buildCommand>
</buildSpec>
<natures>
<nature>org.eclipse.jdt.core.javanature</nature>
</natures>
</projectDescription>
11 changes: 11 additions & 0 deletions .settings/org.eclipse.jdt.core.prefs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
eclipse.preferences.version=1
Copy link
Author

Choose a reason for hiding this comment

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

remove from repo (again git sanitization)

org.eclipse.jdt.core.compiler.codegen.inlineJsrBytecode=enabled
org.eclipse.jdt.core.compiler.codegen.targetPlatform=1.8
org.eclipse.jdt.core.compiler.codegen.unusedLocal=preserve
org.eclipse.jdt.core.compiler.compliance=1.8
org.eclipse.jdt.core.compiler.debug.lineNumber=generate
org.eclipse.jdt.core.compiler.debug.localVariable=generate
org.eclipse.jdt.core.compiler.debug.sourceFile=generate
org.eclipse.jdt.core.compiler.problem.assertIdentifier=error
org.eclipse.jdt.core.compiler.problem.enumIdentifier=error
org.eclipse.jdt.core.compiler.source=1.8
2 changes: 2 additions & 0 deletions Base_of_credit_card_vendors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Visa 13/16 4
Copy link
Author

Choose a reason for hiding this comment

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

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

MasterCard 16 2221-2720/51-55
26 changes: 26 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
@@ -1 +1,27 @@
# recruitment-tl-task-public

## Description

**Please note: Your task is not to write code, but to perform a code review.**

Let's assume you became a leader of the young, immature team, which produces solutions of questionable quality. Below you'll find one of the projects the team is responsible for. Take a look into the repository, do the code review. Point out all the issues with the code quality and how it conforms to clean code (and any other you can think of) principles. Make sure that you look at the code from all possible angles and describe all the possible improvements. Detail all the changes that would have to be done (once again, from all the perspectives you can think of), for this code to become a production ready service (instead of command line application). You could consider resiliency to high loads, things like ddos attacks, auditing etc.

### Task to be reviewed
#### Task description
Implement a REST service which validates a credit card number from a common credit card vendor (Visa, MasterCard). Validator should be implemented in a way that allows extending it easily when adding new card vendors.

#### Task acceptance criteria
- Request contains only two parameters: card vendor and card number
- First version should support MasterCard and Visa

## Instructions for candidates
- once link to recruitment task branch is received, import repository to your Github account
- create a pull request in your repository
- add pull request comments in places you find appropriate
- also respond to production readiness aspects (resiliency, security, etc.) in a summarizing comment
- make repository public, so that recruiters will be able to review task solution

## Review acceptance criteria
- code review has a form of GitHub pull request comments
- comments are in English
- code review was finished within a week (168 hours), counting from the moment, when link to recruitment task branch was received
5 changes: 5 additions & 0 deletions src/validator/CreditCardChecking.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package validator;
Copy link
Author

Choose a reason for hiding this comment

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

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).

Copy link
Author

Choose a reason for hiding this comment

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

This interface can be removed. Gives nothing.


public interface CreditCardChecking {
public boolean checkValidity(CreditCardVendor ccv, String number);
}
62 changes: 62 additions & 0 deletions src/validator/CreditCardValidator.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package validator;
Copy link
Author

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 package. Optionally you can create also "service" subpackage.


import java.util.ArrayList;

public class CreditCardValidator implements CreditCardChecking {
Copy link
Author

Choose a reason for hiding this comment

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

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)


@Override
public boolean checkValidity(CreditCardVendor ccv, String number) {
Copy link
Author

Choose a reason for hiding this comment

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

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

ArrayList<Integer> chars = ccv.getNumberOfChars();
Copy link
Author

@marcin-kapolka-prv 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.

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<>

ArrayList<String> masks = ccv.getIIN();

boolean result1 = checkLengthCorrectness(number, chars);
Copy link
Author

@marcin-kapolka-prv 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.

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.

boolean result2 = checkIINMaskCorrectness (number, masks);
boolean result3 = checkLuhnalgorithmCorrectness(number);

return result1 && result2 && result3;
}

Copy link
Author

Choose a reason for hiding this comment

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

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

private boolean checkLengthCorrectness(String number, ArrayList<Integer> chars){
Copy link
Author

@marcin-kapolka-prv 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.

List instead of ArrayList (see prev comments)

boolean result = false;
Copy link
Author

Choose a reason for hiding this comment

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

reduntant flag

for (int i = 0; i<chars.size();i++){
if (chars.get(i) == number.length())
Copy link
Author

@marcin-kapolka-prv 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.

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;
}

result = true;
}
return result;
Copy link
Author

Choose a reason for hiding this comment

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

return true

}

private boolean checkIINMaskCorrectness (String number, ArrayList<String> masks){
Copy link
Author

Choose a reason for hiding this comment

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

List<>

Copy link
Author

Choose a reason for hiding this comment

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

Same comments as in the previous routine.

boolean result = false;
for (int i = 0; i<masks.size();i++){
if (number.startsWith(masks.get(i)))
result = true;
}
return result;
}

private boolean checkLuhnalgorithmCorrectness(String number){
Copy link
Author

Choose a reason for hiding this comment

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

function name casing is wrong - checkLuhnAlgorithmCorrectness

boolean result = false;

String[] characters = number.split("");
Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

int[] intCharsReverse = new int[characters.length];
int sum = 0;

for(int i = 0;i<characters.length;i++){
intCharsReverse[characters.length-1-i] = Integer.parseInt(characters[i]);
}

for(int i = 0;i<intCharsReverse.length;i++){
intCharsReverse[i] = (i%2==1) ? 2*intCharsReverse[i] : intCharsReverse[i];
}

for(int i = 0;i<intCharsReverse.length;i++){
sum += (intCharsReverse[i]<10) ? intCharsReverse[i] : (intCharsReverse[i]%10 + intCharsReverse[i]/10);
}

if(sum%10==0)
result = true;

return result;
}

}
Copy link
Author

Choose a reason for hiding this comment

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

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

60 changes: 60 additions & 0 deletions src/validator/CreditCardValidatorProject.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
package validator;
Copy link
Author

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


import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.ArrayList;

public class CreditCardValidatorProject {
Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Rename to CreditCardValidatorAppRunner

Copy link
Author

Choose a reason for hiding this comment

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

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)

Copy link
Author

Choose a reason for hiding this comment

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


private static CreditCardVendorsReader reader = new CreditCardVendorsReader();
private static ArrayList<CreditCardVendor> vendors = reader.returnVendorArray();
private static CreditCardValidator validator = new CreditCardValidator();
private static BufferedReader br = new BufferedReader(new InputStreamReader(System.in));

public static void main(String[] args) {
System.out.println("CREDIT CARD VALIDATOR\n");

int vendorsLength = vendors.size();
int selected = 0;

while (true){
printMenu();
try{
selected = Integer.parseInt(br.readLine());
if(selected < vendorsLength){
System.out.println("Type your credit card number:");
String cardNumber;
try {
cardNumber = br.readLine();
boolean result = validator.checkValidity(vendors.get(selected), cardNumber);
printResultOfValidation(result);
} catch (IOException e) {
System.out.println("Something went wrong!\n Please retry the process");
}

}else{
break;
}
}catch(NumberFormatException | IOException e){
System.err.println("Invalid Format!");
}

}

}

static private void printMenu(){
System.out.println("Select your card vendor from list below("+vendors.size()+" exits):");
for (int i = 0;i<vendors.size();++i)
System.out.println(i+" "+vendors.get(i).toString());
}

static private void printResultOfValidation(boolean result){
if(result){
System.out.println("\n\nCard number is valid.\n");
}else{
System.out.println("\n\nCard number is invalid!\n");
}
}
}
32 changes: 32 additions & 0 deletions src/validator/CreditCardVendor.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package validator;

import java.util.ArrayList;

public class CreditCardVendor {
Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Preferably factorize all lists with immutable utils from guava

private String name;
Copy link
Author

Choose a reason for hiding this comment

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

final

private ArrayList<Integer> numberOfChars;
Copy link
Author

Choose a reason for hiding this comment

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

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

private ArrayList<String> IIN;
Copy link
Author

Choose a reason for hiding this comment

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

private final List


public String getName(){
return name;
}

public ArrayList<Integer> getNumberOfChars(){
return numberOfChars;
}

public ArrayList<String> getIIN(){
return IIN;
}

public CreditCardVendor(String vendorName, ArrayList<Integer> charsNumbers, ArrayList<String> masks){
Copy link
Author

Choose a reason for hiding this comment

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

stor shoulb be before public methods, after fields.

Copy link
Author

Choose a reason for hiding this comment

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

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

name = vendorName;
numberOfChars = charsNumbers;
IIN = masks;
}

@Override
Copy link
Author

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

public String toString(){
return name;
}
}
7 changes: 7 additions & 0 deletions src/validator/CreditCardVendorReading.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package validator;
Copy link
Author

Choose a reason for hiding this comment

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

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


import java.util.ArrayList;

public interface CreditCardVendorReading {
public ArrayList<CreditCardVendor> returnVendorArray();
}
58 changes: 58 additions & 0 deletions src/validator/CreditCardVendorsReader.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package validator;
Copy link
Author

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.utils package.


import java.io.BufferedReader;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;
import java.util.ArrayList;

public class CreditCardVendorsReader implements CreditCardVendorReading {
Copy link
Author

Choose a reason for hiding this comment

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

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

@Override
public ArrayList<CreditCardVendor> returnVendorArray() {
Copy link
Author

Choose a reason for hiding this comment

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

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

ArrayList<CreditCardVendor> list = new ArrayList<CreditCardVendor>();

try(BufferedReader br = new BufferedReader(new FileReader("Base_of_credit_card_vendors.txt"))) {
for(String line; (line = br.readLine()) != null; ) {
String[] values = line.split(" ");

String vendorName = values[0];
ArrayList<Integer> vendorNumbersOfChars = new ArrayList<Integer>();
ArrayList<String> vendorMasks = new ArrayList<String>();

parseLengths(values[1], vendorNumbersOfChars);
parseMasks(values[2], vendorMasks);

list.add(new CreditCardVendor(vendorName,vendorNumbersOfChars,vendorMasks));

}
} catch (FileNotFoundException e) {
Copy link
Author

Choose a reason for hiding this comment

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

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

e.printStackTrace();
} catch (IOException e) {
e.printStackTrace();
}

return list;
}

private void parseLengths(String value, ArrayList<Integer> vendorNumbersOfChars){
String[] values = value.split("/");
for(int i=0; i<values.length; i++){
vendorNumbersOfChars.add(Integer.parseInt(values[i]));
}
}

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){
Copy link
Author

Choose a reason for hiding this comment

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

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

vendorMasks.add(masks[i]);
}else{
long min= Long.parseLong((masks[i].split("-"))[0]);
long max= Long.parseLong((masks[i].split("-"))[1]);
for (long j = min; j<=max;j++)
vendorMasks.add(""+j);
}
}
}

}
Copy link
Author

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

55 changes: 55 additions & 0 deletions src/validator/Tests.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package validator;
Copy link
Author

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.


import static org.junit.Assert.*;

import java.io.BufferedReader;
import java.io.InputStreamReader;
import java.util.ArrayList;

import org.junit.Test;

public class Tests {
Copy link
Author

Choose a reason for hiding this comment

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

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

private static CreditCardVendorsReader reader = new CreditCardVendorsReader();
private static ArrayList<CreditCardVendor> vendors = reader.returnVendorArray();
private static CreditCardValidator validator = new CreditCardValidator();
private static BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
private static CreditCardVendor vendorMasterCard = vendors.get(1);


@Test
public void testLengthOfNumberWhenCorrect() {
String number = "5584239583699571";
Copy link
Author

@marcin-kapolka-prv 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.

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);
}

assertEquals(true, validator.checkValidity(vendorMasterCard, number));
}

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

Choose a reason for hiding this comment

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

use assertTrue/assertFalse for clean looks

}

@Test
public void testIINWhenCorrect() {
String number = "5584239583699571";
assertEquals(true, validator.checkValidity(vendorMasterCard, number));
}

@Test
public void testIINWhenIncorrect() {
String number = "7784239583699571";
assertEquals(false, validator.checkValidity(vendorMasterCard, number));
}

@Test
public void testLuhnAlghoritmWhenCorrect() {
String number = "5584239583699571";
assertEquals(true, validator.checkValidity(vendorMasterCard, number));
}

@Test
public void testLuhnAlghoritmWhenIncorrect() {
String number = "5584239583611111";
assertEquals(false, validator.checkValidity(vendorMasterCard, number));
}

}