fixes from code review part 1 #1

kasunsiyambalapitiya · 2017-03-30T19:33:49Z

Added the changes asked in the code review 2,

Separate Github communication and PMT communication to separate classes.
Use a utility class for accessing APIs of both PMT and Github.
Use Gson for manipulating JSON responses.
Avoid using the exact field names in the API responses as variable names ( Ex patchInformation_svnRevisionpublic in PMT API).
Log the the exceptions occurred in closing resources than throwing them.
Avoid use of multiple exit codes and avoid exiting in multiple places in the program instead throw an exception and use a definite place to exit the program.
Convert all logger.info to logger.debug messages.
Use meaningful variable names inside the lamda expression.
Use lists in all possible times instead of using arrays.
Use forEach() directly for Lists instead of using stream().forEach()
Swap if statements to the maximum possible.
Use a bean class to create the JSON structure required for calling github graphql API.

except using Feing for calling APIs and adding test cases. They will be available in the next pull request.

Add log messages for all classes

in the pull request kasunsiyambalapitiya/CodeQualityMatricesProject#1 format the code

added streams for saveReviewersToList method added streams for savePrNumberAndRepoName method replaced for loop with for each loop in readTheReviewOutJSON method

add constants for Reviewers.java

Handle the response code recieved from Graphql API Correct the message thrown in Exception

NisalaNiroshana · 2017-03-31T03:47:24Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/matrices/ApiUtility.java

+
+    }
+
+


Please remove unnecessary new line

Thanks,
fixed in 2ce10d0

NisalaNiroshana · 2017-03-31T03:47:36Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/matrices/ApiUtility.java

+                }
+                // creating a JSON object from the response
+                jsonText = stringBuilder.toString();
+


Please remove unnecessary new line

Thanks,
fixed in 2ce10d0

NisalaNiroshana · 2017-03-31T03:49:35Z

...ualitymatrices/src/main/java/com/wso2/code/quality/matrices/CodeQualityMatricesExecutor.java

+                    }
+                }
+            }
+            logger.debug("The commit hashes are: " + commitHashes);


It is better to use isDebug option here and log the debug message.

Thanks,
fixed in 2ce10d0

NisalaNiroshana · 2017-03-31T03:51:22Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/matrices/GithubApiCaller.java

+        String url = "https://api.github.com/search/issues?q=" + commitHashToBeSearched;
+        try {
+            httpGet = new HttpGet(url);
+            httpGet.addHeader("Accept", "application/vnd.github.mercy-preview+json");


Can't we create constants for these Strings ? "Accept" and "Authorization" are used in multiple places.

Thanks,
fixed in 2ce10d0

imesh · 2017-03-31T04:55:14Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/matrices/model/Repository.java

+ * Pojo class used for parsing JSON response received from github REST API
+ */
+public class Repository {
+    private String full_name;


Better to use camel notation when naming variables.

camel case notation cannot be used in here as we need to use the same field name in the JSON for mapping with GSON. For example in https://api.github.com/repos/wso2/wso2-axis2-transports/commits/e3c3457149b109178d510aac965d5a85cc465aa0 "html_url", "comments_url" fields are named with having " _"

fix it as discussed by using "@SerializedName" annotation in each pojo class

imesh · 2017-03-31T04:58:56Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/matrices/PmtApiCaller.java

+     * @throws CodeQualityMatricesException
+     */
+    public String callApi(String accessToken, String patchId) throws CodeQualityMatricesException {
+        String pmtUrl = "http://umt.private.wso2.com:9765/codequalitymatricesapi/1.0.0//properties?path=/_system/" +


Might be better to read this URL via a configuration parameter, otherwise if the PMT URL change at some point, this code would need to be re-compiled.

+1 . We should get these type of values through a separate configuration.

Thanks, fixed it in 25560d7

NisalaNiroshana · 2017-03-31T04:56:16Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/matrices/ApiUtility.java

+            if (responseCode == 200) {
+                //success
+                bufferedReader = new BufferedReader(new InputStreamReader(httpResponse.getEntity().getContent(),
+                        "UTF-8"));


I think we can use StandardCharsets.UTF_8 for "UTF-8"

Thanks fixed it

DilanUA · 2017-03-31T05:12:56Z

org.wso2.codequalitymatrices/pom.xml

+        <artifactId>wso2</artifactId>
+        <version>5</version>
+    </parent>
+    <groupId>org.wso2.codeQualityMatrices</groupId>


I think the practice is to use lower case, not uppercase

Thanks, fixed it

DilanUA · 2017-03-31T05:14:49Z

org.wso2.codequalitymatrices/pom.xml

+        <artifactId>wso2</artifactId>
+        <version>5</version>
+    </parent>
+    <groupId>org.wso2.codeQualityMatrices</groupId>
    <artifactId>CodeQualityMatrices</artifactId>


I think the practice is to use lower case, not uppercase and shall we use hyphen(-) to separate out different words, for ex: code-quality-matrices?

Thanks, fixed it

DilanUA · 2017-03-31T05:32:30Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/matrices/ApiUtility.java

+     * @return String representation of the json response
+     * @throws CodeQualityMatricesException
+     */
+    public static String callGraphQlApi(HttpPost httpPost) throws CodeQualityMatricesException {


we can move this method to GithubApiCaller.

DilanUA · 2017-03-31T05:33:29Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/matrices/GithubApiCaller.java

+ * @since 1.0.0
+ */
+public class GithubApiCaller {
+    private HttpGet httpGet;


You can make this instance variable a local one and make this class a static one, WDYT?

DilanUA · 2017-03-31T05:34:49Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/matrices/PmtApiCaller.java

+ *
+ * @since 1.0.0
+ */
+public class PmtApiCaller {


As same as GithubApiCaller, we can make PmtApiCaller, a static one too, reason -> as we do not see any use of instance variables, WDYT?

DilanUA · 2017-03-31T05:39:33Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/matrices/model/Repository.java

+
+/**
+ * Pojo class used for parsing JSON response received from github REST API
+ */


no since tag

Thanks, fixed it

DilanUA · 2017-03-31T05:39:39Z

...odequalitymatrices/src/main/java/com/wso2/code/quality/matrices/model/ReviewApiResponse.java

+
+/**
+ * Pojo class used for parsing JSON response received from github REST API
+ */


no since tag

Thanks, fixed it

DilanUA · 2017-03-31T05:39:45Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/matrices/model/Reviewer.java

+
+/**
+ * Pojo class used for parsing JSON response received from github REST API
+ */


no since tag

Thanks, fixed it

DilanUA · 2017-03-31T05:41:27Z

...odequalitymatrices/src/main/java/com/wso2/code/quality/matrices/model/SearchApiResponse.java

+
+/**
+ * Pojo class used for parsing JSON response received from github REST API
+ */


no since tag

Thanks, fixed it

DilanUA · 2017-03-31T05:41:34Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/matrices/model/SearchItem.java

+
+/**
+ * Pojo class used for parsing JSON response received from github REST API
+ */


no since tag

Thanks, fixed it

NipunaMarcus · 2017-03-31T05:56:54Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/matrices/ApiUtility.java

+            int responseCode = httpResponse.getStatusLine().getStatusCode();
+            if (responseCode == 200) {
+                bufferedReader = new BufferedReader(new InputStreamReader(httpResponse.getEntity().getContent(),
+                        "UTF-8"));


Here also please use the given constants as nisala mentioned above instead of using string.

Thanks, fixed it

NipunaMarcus · 2017-03-31T06:03:24Z

...ualitymatrices/src/main/java/com/wso2/code/quality/matrices/CodeQualityMatricesExecutor.java

+            throw new CodeQualityMatricesException("Error occurred while calling PMT API", e);
+        }
+        Gson gson = new Gson();
+        if (jsonText != null) {


can we move gson initialization into if(jsonText != null){} block? so if the condition failed no need to create such a instance as it is used only inside that IF block. (Its a "nice to have" one. But that won't be much of a different to the performance though.)

Thanks, fixed it 25560d7

kasunsiyambalapitiya and others added 30 commits March 10, 2017 09:33

Correct the year in copyrights

72bb076

added .idea direcotory to gitignore file

2f68078

add entries for gitignore

3a24417

removed unwanted files from the repository

9b8be3f

Add log messages

2440d9f

Add log messages

9424e91

Add log messages for all classes

Merge branch used for adding logs to the classes

cfaa17f

Remove the CodeQualityMatrices.iml from remote

dda1a46

Get the patch details for a file made by the given commit

1c39bb0

Complete upto getting authors of the buggly lines from SDK

5699fb9

Added docs

c77dcd0

Pmt.java convert to use java8

df622e0

convert BlameCommit.java to java8

e3d9da5

create seperate class for calling graphql API

a9df118

Change the Logger initiliation in classes as suggested

9ee6d5f

in the pull request kasunsiyambalapitiya/CodeQualityMatricesProject#1 format the code

fix the copyright header

f4c2571

add CodeQualityMatricesException class

ebfef31

Add exceptions for classes

3c76e95

Remove invalid user input handling in RestApiCaller.java

0efc4b2

Rename log object and fixed possible null value passings

960a859

fix the changes asked by DilanUA in PR 2

57592c6

Changed method names to have a good English flow

bc8c7b8

change the part where it skips taking blme of new files

03c8563

replaced code with java 8 features

47e168e

added streams for saveReviewersToList method added streams for savePrNumberAndRepoName method replaced for loop with for each loop in readTheReviewOutJSON method

Replaced variables with contants

49c87dc

add constants for Reviewers.java

Correct Exceptional Handling

5fdedc4

Delete replaced codes from Java 8

43c52d1

fix formatting in java 8 stream API codes

772a100

Handle response codes

d28a361

Handle the response code recieved from Graphql API Correct the message thrown in Exception

fix getting graphql blame issue

81db43d

kasunsiyambalapitiya added 14 commits March 27, 2017 18:50

Use Gson for processing Github Search commit API response

1308407

Add model classes

9dc9c9d

use Gson till getting till getting author commits

5e58bc3

Change name of Graphql Pojo classes

3930dc2

Add pojo classes for github Issue API

e5baf70

Use Gson for processing review API output

92c7ee3

Add java docs

aca2f83

Delete unused classes

a9cb37c

Fix bugs found in running Idea Analyse

501ef19

Fix groupId in pom.xml

d10fd0c

Fix errors received by adding wso2 parent pom

1c9947f

Remove .classpath and .project created from Eclipse IDE

5364583

Moved .gitignore a level up in directory

184062b

Change the repo name

7ec687e

kasunsiyambalapitiya changed the title ~~few changes from code review 2~~ fixes from code review part 1 Mar 31, 2017

NisalaNiroshana suggested changes Mar 31, 2017

View reviewed changes

fix changes asked by Nisala in PR 1

2ce10d0

kasunsiyambalapitiya requested a review from uthaiyashankar March 31, 2017 04:46

imesh reviewed Mar 31, 2017

View reviewed changes

NisalaNiroshana suggested changes Mar 31, 2017

View reviewed changes

DilanUA suggested changes Mar 31, 2017

View reviewed changes

NipunaMarcus reviewed Mar 31, 2017

View reviewed changes

kasunsiyambalapitiya added 4 commits March 31, 2017 11:46

fix changes asked in PR 1

5d3ae06

Merge branch 'annotatePojoClass' into changesFromCodeReview2

673a71a

Rename the package

123494b

Use a Configuration file

25560d7

kasunsiyambalapitiya force-pushed the changesFromCodeReview2 branch from 6d4c965 to 25560d7 Compare April 3, 2017 06:15

kasunsiyambalapitiya force-pushed the master branch from 8622a82 to 2c3e00e Compare September 4, 2020 12:00

fixes from code review part 1 #1

Are you sure you want to change the base?

fixes from code review part 1 #1

Conversation

kasunsiyambalapitiya commented Mar 30, 2017

Choose a reason for hiding this comment

kasunsiyambalapitiya Mar 31, 2017 • 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

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

NipunaMarcus Mar 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasunsiyambalapitiya Mar 31, 2017 •

edited

Loading

NipunaMarcus Mar 31, 2017 •

edited

Loading