Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove explicit boxing and unboxing #84

Merged
merged 3 commits into from
Jul 26, 2017

Conversation

JamieMagee
Copy link
Member

Explicit boxing and unboxing in unnecessary since Java 5. See https://docs.oracle.com/javase/tutorial/java/data/autoboxing.html

This also fixes #13

@ajlam
Copy link
Member

ajlam commented Dec 12, 2016

@JamieMagee - thanks for the suggestion. We need to add a few more test cases for casting. We'll revisit this PR once they've been added.

@v-nisidh v-nisidh added the Under Review Used for pull requests under review label Jan 17, 2017
@v-nisidh v-nisidh added this to the Long Term Goals milestone Feb 5, 2017
@JamieMagee JamieMagee deleted the explicit_boxing_unboxing branch July 5, 2017 21:40
@xiangyushawn
Copy link
Contributor

Hello @JamieMagee , could you please restore your branch if possible? so we can reopen this PR. It was deleted unintendedly.

@JamieMagee JamieMagee restored the explicit_boxing_unboxing branch July 5, 2017 21:59
@JamieMagee
Copy link
Member Author

@v-xiangs Sure, I've restored it now.

@xiangyushawn
Copy link
Contributor

@JamieMagee Thank you very much for the quick response.

@JamieMagee
Copy link
Member Author

I've updated the branch to resolve any merge conflicts.

@codecov-io
Copy link

codecov-io commented Jul 6, 2017

Codecov Report

Merging #84 into dev will increase coverage by 1.28%.
The diff coverage is 24.42%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev      #84      +/-   ##
============================================
+ Coverage     40.16%   41.44%   +1.28%     
- Complexity     1892     1983      +91     
============================================
  Files           107      107              
  Lines         24482    24669     +187     
  Branches       4038     4102      +64     
============================================
+ Hits           9833    10224     +391     
+ Misses        12815    12554     -261     
- Partials       1834     1891      +57
Flag Coverage Δ Complexity Δ
#JDBC41 41.36% <24.42%> (+1.31%) 1977 <7> (+94) ⬆️
#JDBC42 41.34% <24.42%> (+1.33%) 1974 <7> (+89) ⬆️
Impacted Files Coverage Δ Complexity Δ
...n/java/com/microsoft/sqlserver/jdbc/Parameter.java 42.64% <0%> (+6.06%) 49 <0> (+3) ⬆️
...microsoft/sqlserver/jdbc/SQLServerResultSet42.java 4.54% <0%> (ø) 1 <0> (ø) ⬇️
...erver/jdbc/SQLServerPreparedStatement42Helper.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...va/com/microsoft/sqlserver/jdbc/SQLServerBlob.java 33.51% <0%> (+5.85%) 16 <0> (+2) ⬆️
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java 42.11% <0%> (+2.45%) 0 <0> (ø) ⬇️
...va/com/microsoft/sqlserver/jdbc/SQLServerClob.java 28.26% <0%> (+1.73%) 4 <0> (ø) ⬇️
.../com/microsoft/sqlserver/jdbc/SQLServerDriver.java 75.44% <0%> (ø) 25 <0> (ø) ⬇️
...c/main/java/com/microsoft/sqlserver/jdbc/Util.java 49.46% <0%> (+1.94%) 66 <0> (+3) ⬆️
...t/sqlserver/jdbc/SQLServerCallableStatement42.java 2.63% <0%> (ø) 1 <0> (ø) ⬇️
...ava/com/microsoft/sqlserver/jdbc/FailOverInfo.java 52.63% <0%> (ø) 0 <0> (ø) ⬇️
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bd68b5...d09ab27. Read the comment docs.

@xiangyushawn
Copy link
Contributor

@JamieMagee Thank you for your hard works !

@ajlam ajlam modified the milestones: 6.3.0, Long Term Goals Jul 17, 2017
@peterbae
Copy link
Contributor

Hi @JamieMagee, thanks for the contribution! A couple of suggestions:

  1. In FailOverInfo.java, I think we could use Integer.parseInt instead of instantiating another Integer object (and then unboxing it into int again since portNumber is int).
  2. For SqlTypeValue.java, could we try replacing all the tabs (since they're inconsistent across different machines) from lines 19 through 38 with spaces instead, and pad the spaces in between parameters (with spaces) so that the layout of the variables stay the same as before (as in, the 3 parameters for each data type would match the spacing of the minValue, maxValue and nullValue comments on line 19, respectively)? The removal of explicit autoboxing part is good. If the tabs look different on your machine and you're not sure about the original spacing, I can edit the file for you as well.

Other than those 2, the changes look good.

Formatting changes
Change the formatting to previous version without the auto formatting
@JamieMagee
Copy link
Member Author

@peterbae Thanks for the feedback. I've been a little busy, most recently with oneweek, but I'll make the changes as soon as I have some time.

@peterbae
Copy link
Contributor

@JamieMagee Thanks for the reply. We wanted to include this in our next release so I've made the small formatting changes to one file. I will merge this PR after the tests have passed.

@peterbae
Copy link
Contributor

Test results are good. Merging in.

@peterbae peterbae merged commit 55e20ed into microsoft:dev Jul 26, 2017
@JamieMagee
Copy link
Member Author

@peterbae Thanks for all your help with getting this PR merged 👍

@JamieMagee JamieMagee deleted the explicit_boxing_unboxing branch July 27, 2017 10:14
@lilgreenbird lilgreenbird added this to Closed/Merged PRs in MSSQL JDBC Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Under Review Used for pull requests under review
Projects
MSSQL JDBC
  
Closed/Merged PRs
Development

Successfully merging this pull request may close these issues.

Use static factory methods for wrapper types
7 participants