Redundancy of parameters removed by introducing a Constants class

[dwamara]

Each new Gateway implementation will have to have a inner class in the new Constants class and have all the constants (parameters etc) listed there as public static final String variables

[ilyasdotdev]

Hey defining constant class is not a good idea its creating complexity for new comer who would like to integrate new gateway Also there is an idea to implement more than 100 gateways and for that constant class will be a pain.

Yes but we can discus about XML creation.

[dwamara]

You can view an example of what happens with using Freemarker for creating the requests, I have already pushed them in the pull request. Regarding the constants class, it is not the main goal to offer people a solution that works, the quality of the work and the code in this case has to be as good as possible and with the way it was before, the quality wasn’t that good, believe my 20+ years of using Java. If you want to work on the quality of the code and gain tips that will become intrinsic to your way of working, I will suggest that you use a solution like Sonarqube to check the quality of your code.

Sent with Unibox

On May 9, 2018, at 10:30 AM, Muhammad Ilyas notifications@github.com wrote:

Hey defining constant class is not a good idea its creating complexity for new comer who would like to integrate new gateway Also there is an idea to implement more than 100 gateways and for that constant class will be a pain.

Yes but we can discus about XML creation.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub , or mute the thread .

[ilyasdotdev]

I can understand but defining constants for just one time use is not a good idea.

[tousifhasanzai]

yes Ilyas is right, as we are expanding this library by adding no. of gateways, so to define constant for each payment gateway in this file seems not a good idea. each payment gateway is a separate entity so it's all configuration and variables must remains separate

[shanmarif]

@ilyas2016 @tousifhasanzai I agree with @dwamara its a good approach to use constants although it will create difficulties in Integrations but still we have to meet coding standards as well, if we do not use constants and directly bind parameters/property to a class in the future for any property level change it will be necessary to modify the class which can increase chances of mistakes aka Human Error

[dwamara]

The constants is just an approach to get things in the right direction. I don’t pretend on having the all knowing Graal on this, so if you can suggest a better approach, I’m glad to hear and abide by it and will make the necessary modifications to the code that I have written so far for this project.

Sent with Unibox

On May 9, 2018, at 10:47 PM, shanmarif notifications@github.com wrote:

@ilyas2016 @tousifhasanzai I agree with @dwamara its a good approach to use constants although it will create difficulties in Integrations but still we have to meet coding standards as well, if we do not use constants and directly bind parameters/property to a class in the future for any property level change it will be necessary to modify the class which can increase chances of mistakes aka Human Error

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub , or mute the thread .

[lukkystunt]

@ilyas2016 @tousifhasanzai i agree with @dwamara having a constant class helps the code base much easier to refactor and also this is the best time to define the coding standard for the project since its relatively still new.

To add more to what @dwamara refactored, i think it would be much better to move all the payment gateways to a separate package, with this you can have more of a schematic structure for each gateway and in the future we can create a script that auto generate schematic structure for new gateways. examples

-----package com.tranxactive.j2pay
            .gateways
                 .billprogateway
                       ---Constant.java
                       ---BillproGateway.java
-----package com.tranxactive.j2pay
            .gateways
                 .authorizegateway
                       ---Constant.java
                       ---AuthorizeGateway.java

The above structure makes the code more modular and easier to maintain and enforce coding standards.

[tousifhasanzai]

seems acceptable option, @dwamara what do you think about @lukkystunt review?

[dwamara]

That seems OK, but for the package name under "gateways", you can remove "gateway" at the end because as it is supposed to be a gateway, there is no need to repeat it again. And as there will be more than one constant value in the class, the class name has to be in plural form. If it's ok, I can make the change and let it pass through sonarqube again to be sure that the quality hasn't been affected. And as @lukkystunt said, that will be a good time to create and enforce standards now. Those standards can be in the future integrated with a solution like checkstyle to be sure that no one is pushing code that doesn't conform to the standards.

[dwamara]

But some packages have also to be moved like "com.tranxactive.j2pay.gateways.responses", "com.tranxactive.j2pay.gateways.util". The classes under "com.tranxactive.j2pay.net" are util classes and could be relocated under a more convenient package.

[ilyasdotdev]

yes @dwamara we can go for it.

[tousifhasanzai]

great recommendation @dwamara and @lukkystunt , I am agree with that approach as well

[dwamara]

I have just made the changes with the relocation of the classes under a new package with a single Constants class per Gateway and the quality looks like this:

[dwamara]

Made some other changes and now I am at

[dwamara]

I will push that and go out, it's a bank holiday in Germany so I want to enjoy it.

[ilyasdotdev]

[ilyasdotdev]

if its failed? then why we implement that?

[dwamara]

If what failed?

[dwamara]

And BTW someone should think about adding Junit tests.

[ilyasdotdev]

your given snapshot says its failed.

[dwamara]

It failed to pass the quality gate. It's exactly what I meant yesterday when I wrote that it's not enough to give people a code that works, it's important to give them a code of good quality and on the quality side, the actual code fails to pass the quality threshold because locally I still have 11 vulnerabilities, 14 code smells, 7h code debt, 7.7% duplications spread between 20 code duplications. It says nothing to the functionality of the code, just its quality. For the functionalities, you have to have unit tests that have to be run and this part is still missing. I know most of the coders think that writing code is enough and don't care about writing tests, and I fell sometimes in that category because I do most functional and integration tests of my code and not unit tests but when you want to give people a library that they have to use, in the future surely in productive systems, you have to assure them that your code is working and that enough unit tests have been made.

[dwamara]

But it's a great improvement compared to the code I had when I first checked it out because they were something like 75% duplicated code, 4 days of debt, nearly 60 vulnerabilities and a little more than 300 code smells.

[ilyasdotdev]

And what you think about this.

[dwamara]

My quality standards are very high because I am very rigorous on the code that I write so what you have on your side won't be considered of a good quality for me.

[dwamara]

Here what my quality gate looks like

[dwamara]

Another thing that I will be adding is a logger, but to have as less as dependencies as possible, I will be doing the logging in a file with the java.util.logging.Logger

[ilyasdotdev]

We are still thinking what will be the beat way to handle constants for large data. Like 50+ gateways. Did you find any good way instead of defining in single class.??

[dwamara]

I haven't found a better solution than to have a single class per Gateway implementation in a single package as proposed by @lukkystunt

[ilyasdotdev]

Alright so your code with "separate packages for each gateway" is ready. can i merge?/

[dwamara]

It’s already been pushed since yesterday, you should be able to merge, all the last coding that I made are in the pull request.

[ilyasdotdev]

Why some files are .xml while other are .ftl?

[dwamara]

Sorry forgot to rename some, I needed to create first XML files to get them validated before turning them to ftl and I forgot some. Renaming and making another pull request

[ilyasdotdev]

and also we should not create two inner classes in constants class like RequestParameters and ResponseParameters instead, we should define directly on constants class level. since you are using ResponseParameters.NAME in request parameters thats not making sense to me.

[dwamara]

if you want to do it, then you have to prefix the constants with either REQUEST or RESPONSE in order to be able to differentiate and know which ones are those for the Request and those for the RESPONSE. I made it that way in order to be able in the future to easily create a documentation (either through Javadoc or something else) where one can directly know and see which parameters for example belong to requests and which not.

[ilyasdotdev]

using ResponseParameters.NAME in request not making any sense how will you sort that out?

[dwamara]

I don’t quite get your question

[ilyasdotdev]

what if request and response have same parameter name? like both have name it normally happens like a gateway accept last transaction id as transnum and returning new transaction id as same i.e. transnum since we are using static import and if we defined two variables with same name in each inner class that will raise ambiguous reference error.

[ilyasdotdev]

Also please double check your code all implement gateways are fully tested and dont want to test them again.

caught this line in authorize gateway

input.put(CREDIT_CARD_EXPIRATION_DATE, "XXXX"); // refundParameters.getString(ParamList.CARD_EXPIRY_MONTH.getName())).append(refundParameters.getString(ParamList.CARD_EXPIRY_YEAR.getName()).substring(2)

which is passing customers provided card as xxxx.

[dwamara]

That’s not my code, check the code in the master before I did the very first pull request, it’s the code that was there. As I said, until now, I haven't and will not change anything in the logic of the code,  just changing the necessary places where syntactic changes are possible as I won’t be doing any change in the logic if there is not tests or unit tests that I can use. If you have tested them, please provide the necessary tests under a src/test directory.

[ilyasdotdev]

test payment gateways are little tricky we would not be able to refund a transactions instantly until it is settled. I am wondering how we could create test cases for that.

[dwamara]

If you can’t use a remote system for testing your code, you have to resort to using a mock system or mocking the responses you can get from the system for various use cases (success, failure, failure because system is not reachable, failure because credits are missing, etc). You can research mocking frameworks for Java on the net but you have for example EasyMock or Mockito that are very well know. So it’s not that tricky after all, you just have to take your time to learn how to mock and mock.

[dwamara]

And BTW unit tests are supposed to be implemented to work offline, so they shouldn't rely on external systems, be it a database or something else. As soon as you start using external systems, you move concept wise from unit tests to integration tests.

[ilyasdotdev]

@dwamara please stop any further work. Now library seem more complicated to use or implement new gateways. I will let you when we will decide for changing core of library. thanks.