Showing posts with label Code Quality. Show all posts
Showing posts with label Code Quality. Show all posts
Eclipse - Run Code Clean Up Manually + Save Action
The Scenario
Eclipse shows compiler warnings in Problems view.
Some may be trivial such as unused import, but some may be more serious such as null access.
But if we don't fix trivial issues, there may be too many warnings in the project; This may cause us just ignore all these warnings, which can lead us ignore vital/important warnings and potential bugs.
So usually I don't like see any compile warning in current editor or the whole project - We can find this in Problems view.
How Eclipse can Help
First we can configure compiler at Preferences -> Java -> Compiler -> Errors/Warnings.
Save Action
We can configure Eclipse "Save Action" at Java -> Editor -> Save Action to auto format code, organize imports and a lot of things.
- We can also configure save action for Javascript and Scala or other langs.
But sometimes, when we only modify a few lines of the file, we don't want to change other parts otherwise when others review the change, it's difficult for them to figure out what changed.
So usually I only configure "Save Action" to format edited lines and organize imports.
Run Code Cleanup Manually
First we assign a shortcut key such as Ctrl+Alt+Command+C in Preferences(Command+,) -> Key
- We can also configure this for Javascript.
Then we configure what Code Clean Up does at Preferences -> Java -> Code Style -> Clean Up
Eclipse shows compiler warnings in Problems view.
Some may be trivial such as unused import, but some may be more serious such as null access.
But if we don't fix trivial issues, there may be too many warnings in the project; This may cause us just ignore all these warnings, which can lead us ignore vital/important warnings and potential bugs.
So usually I don't like see any compile warning in current editor or the whole project - We can find this in Problems view.
How Eclipse can Help
First we can configure compiler at Preferences -> Java -> Compiler -> Errors/Warnings.
We can configure Eclipse "Save Action" at Java -> Editor -> Save Action to auto format code, organize imports and a lot of things.
- We can also configure save action for Javascript and Scala or other langs.
But sometimes, when we only modify a few lines of the file, we don't want to change other parts otherwise when others review the change, it's difficult for them to figure out what changed.
So usually I only configure "Save Action" to format edited lines and organize imports.
We can also configure General -> Editors -> AutoSave to save dirty editors every X seconds.
Run Code Cleanup Manually
First we assign a shortcut key such as Ctrl+Alt+Command+C in Preferences(Command+,) -> Key
- We can also configure this for Javascript.
Then we configure what Code Clean Up does at Preferences -> Java -> Code Style -> Clean Up
It can do things(more than 20) such as format code, organize imports, add @Override, final, serial ID, add unimplemented methods, remove trailing space, correct indentation and much more.
If I change most part of the current file, or I think it's necessary, I will click Ctrl+Alt+Command+C to tun Code Clean Up manually.
What to Look for in Code Review
Good PR
Small Change/Diff/Pr
Stacked Change
Review often, merge often
What we want to achieve via Code review
Capture design and code issues
Sharing of knowledge
What to focus in Code review
Business logic
High level design
Coding sytle
Code review should be productive
– both for reviewers and the developer
Usually code review should be finished timely – within 3 to 5 days, not in weeks or months
- Unless we don’t need the function or have disagreement about the design and the code
- should be a pleasure for reviewers and the developer
- help the author to capture mistakes, bugs
- Reviewer can learn somethings such as:
- domain knowledges, the new feature or others
Who do the code review?
How to pick code reviewers?
Who merge the code?
Bug always happens, how to prevent the developer and code reviewers make same mistake?
Before Send Code to Review
Only one change in one pr
Build succeed
Include new unit tests, integration tests
All tests passed
Existing functions work
New functions work
Use tools to analyze code first - eclipse compile warnings, findbugs, pmd, mvn site report.
Carefully reviewed your PR first
- Fix common issues such as commented-out code, indentation
- Code looks clean and readable to yourself
- Use tools such as FindBugs or Eclipse itself to check and clean your code
- It saves time for you and reviewers
- The reviewers don't pay attention to the trivial stuff and can focus on logic or things that can't be easily found out by tools
PR
Your PR should be pleasant for others to review.
Every PR should have clear description what has changed, and why (if needed)
Write descriptions/comments
- for reviewer
- for clients who is going to call your APIs
- share knowledge
Add comments to your code - such as why the code is removed
Highlight potential issues, or things you want reviewers to pay attention
- Don't hide them, instead highlight them
- it's for your won good as reviewers help improve your code, find potential issues, also this makes yourself a better engineer
Your Code
Your code should be pleasant for you and reviewers to read
- We not only write code, we write better code every day, write code that others want to use/call or read and maintain.
Write code for maintainer, reviewers, you in the future
Add comments to your code
- why you made the change this way: especially when there re other obvious approach, but maybe not work
- how you find the bug (internet link)
- when you think others(maybe yourself in the future) maybe confused
- write comments for your client/caller, how they should use it, what's the trick part
When others/reviewers ask questions about the code or when they feel difficult to understand it
It may be because others lack of the specific knowledge of the domain(logic) or it may (more likely) be a sign that we should improve the code, make it more readable
Share your knowledge to the team
- Such as in order to implement the PR, you learned some things new
- You don't have to share if it's something basic or we can find it via simple google search
- It's good to share if it takes you a lot of time, or you just want to share :)
During Code Review
Prefer to use PR to communicate
- as others would also know the conversation
- unless it's difficult to describe in words
- transparent and share knowledge
- What bugs we made, and how someone detect in code review
- how the reviewer reviews the code and finds out the issue
Don't forget the big picture
- What's the goal/new function of the PR
- Whether it solves the problem, implements the new function
What questions to ask?
- Not trivial things such as What is XX?
- Google it first, unless you still didn't find your answers
- Or you are the manager, tech skills/knowledge is not your primary goals any more
Use IDE to help understand the code if necessary
- Check the code change in Eclipse can help you to understand the code if you are not familiar with the function,
What to Look For
What may go wrong, what code may look suspicious, dangerous, have potential bugs, need pay extra attention, common source of bugs.
Whether the change improves code quality or make it worse?
Is there enough tests?
- Unit/Integration tests
- Happy, bad paths or corner cases
Admin what you don't know and ask others to review that part.
Ask for clarification/explanation if can't understand it.
Does it need documentation or notify other teams?
Using new features in the language?
- such as JDK8 time api, stream, new methods: getOrDefault, compute, putIfAbsent, computeIfAbsent etc
Don't invent here
- Whether we should use some well-known libraries, instead write our own code.
- Things that looks strange, that I don't understand or can't understand easily
Whether the new code is consistent (with existing code)
-- such as method name, order of parameters.
Focus on Main Function/Logic
-- Focus on the code that looks complicated, difficult to understand
-- Don't skip it if you can't understand it.
Performance
Whether the api is performance sensitive?
- Like database/networks calls
- If so, how long it usually takes in normal/extreme case?
- Is there duplication? do we need cache? or save the result first into a variable or a map?
--Did the code do enough optimization? such as use thread pool, call third part services asynchronously, in parallel etc.
Do we need provide timeout for block call?
If so, whether the timeout value makes sense or is configurable?
Thread safe
Concurrent programming is hard, so pay more attention.
Example: Don't Use Mutable Variables in Multiple Threads
Variables may be Changed Unexpectedly in Multiple Threads
- Use mutable instance fields in services.
-- SimpleDateFormat
Does it reuse threadpool, is it configured correctly?
- Don't recreate threadpool and shutdown in each method, reuse it.
Is there really benefit to use multiple threads?
Is there race condition?
Is lock used correctly?
Code Quality
SOLID
Whether the method/class does only one thing?
Big interfaces: split them
Postconditions cannot be weakened in a subtype.
Review the data model
- As it's much harder to change data model after deployed to production.
-- is there any not needed fields?
How it is going to be used?
-- use redundant field/data to boost performance (avoid join query?)
-- will we search on it, define it as separate field, or store it as part of json object?
Is it using Right Data structures?
What's the frequent operations on the data?
If need search often: uses Set/Map instead of List
If there is a Key in the data, use Map instead of List.
Don't use mutable object inSet, or as Key in Map
Consider using Guava Multimap instead of Map<K, List<V>>
-- Multimap put: the case when the key doesn't exist, get returns empty collection instead of null.
Is class in right place - module/application or package?
Security
Do we need protect the API?
- If so, which role?
Are we storing sensitive info?
Should we audit the operation?
Code Quality
Bad Smell
Magic Number/String
- change to use constant/enum and give a meaningful name.
Limit the number of parameters per unit to at most 4.
- extract parameters into objects.
Parameters
Too many parameters
Parameters are easily to be mixed
Use boolean as parameter -- consider to use enum or use different names.
Big Class/Methods
Method
Usually method should be short and easy to read, no too many nests.
Complex methods
-- extract small methods
Separate Concerns to different classes/functions
API Design
Think from client perspective.
Immutability
Unmodifiable of parameter, return value
- defensive copy if needed
Whether API is easier to use, test/rollback in production
Code Readability
Whether the code is easy to read and understand?
-- If not, ask the developer to clarify or add comments to the code, or refactor the code.
Bad signs - use boolean parameters, return Pair, long parameter lists
Naming
Naming(variable/function/class name) is important to make the code readable.
Whether names of variables, method, class are meaningful?
-- 20 Tips for Better Naming
Good names should answer all the big questions, be precise and thorough, reveal intent, and follow conventions.
-- Avoid use vague variable names.
waitTime -> waitTimeInMs
Arguments
Number of Method Arguments should be as small as possible.
- smaller is better, remove unused parameters
Order of arguments
- especially when the methods/constructors are overloaded.
Order of statements
-- Put related statement together
-- whether some commands/functions should be always last one? If so, is it documented?
API design
Unnecessary code
Is there any unnecessary code(field/methods/function)?
Configuration Driven
Should it be define as constant, in property file or stored in db?
Prefer to store in db, so we can change it dynamically.
Do we need wire on/off feature?
- Consider adding turn-on flag for for new features
Code Duplication - Don't repeat yourself
Does it cause duplication?
Is there anything that can be extracted and reused in future?
Whether this is common/util function?
Whether the function is put in right place?
Tools like PMD/CPD can help find copy-paste code.
Similar Methods:
Extract common logic?
Use Holder object to contain all data?
Before create a new constant/util methods, search the code base first to make sure it's not already there.
Common Bugs
NPE
- Change to use Optional, or use annotations: such as @Notnull, @CheckforNull
-- if(anint == anInteger), anint == null,
anInteger = boolean?anint:anInteger
-- Use NullSafe API:
Objects.equals, MoreObjects.firstNonNull, StringUtils...
< or <+, off-by-one error
isEmpty or isNotEmpty?
Dangling If/Missing else
Missing default in switch
Allocate Unlimited Resources
-- Executors.newCachedThreadPool();
-- Create threads, make db/network calls in loop
Call expensive operations in for loop or for every record.
Look for signs of questionable code
Deep nesting of loops and conditionals.
AutoDetectParser instead of JpegParser
Not Needed Code
Remove commented code
YAGNI
Developer may write code that is not needed - such unused field/functions/features
Don't guess at future functionality.
Be cautious of clever implementation.
Is it over-engineered?
Is there enough Logging?
Whether there is enough logging to help debug if the function doesn't work as expected or to prove the code work.
Logging more info in lower log level.
Monitoring
Should we add monitor info such as API's latency/frequency?
Don't
Don't return array
Mics
- Non-static inner class, anonymous class, lambda uses method or field from the enclosing class
- Prefer to use static inner class
- Whether it may cause memory leak
- Use Optional instead return null
- Use Collections.emptyList/List.of()/ etc instead of new empty ArrayList
- Use new features unnecessary or inappropriately
- In some cases, the code is more efficient to just use regular for loop instead of stream
- we can collect/update all variables in one loop rather than use multiple streams(traverse) to update them separately
- Miss @Override
Testing
Json and Java serialization and deserialization of Http API response data
Resources
How to write the perfect pull request
thoughtbot - Code Review
Small Change/Diff/Pr
Stacked Change
Review often, merge often
What we want to achieve via Code review
Capture design and code issues
Sharing of knowledge
What to focus in Code review
Business logic
High level design
Coding sytle
Code review should be productive
– both for reviewers and the developer
Usually code review should be finished timely – within 3 to 5 days, not in weeks or months
- Unless we don’t need the function or have disagreement about the design and the code
- should be a pleasure for reviewers and the developer
- help the author to capture mistakes, bugs
- Reviewer can learn somethings such as:
- domain knowledges, the new feature or others
Who do the code review?
How to pick code reviewers?
Who merge the code?
Bug always happens, how to prevent the developer and code reviewers make same mistake?
Only one change in one pr
Build succeed
Include new unit tests, integration tests
All tests passed
Existing functions work
New functions work
Use tools to analyze code first - eclipse compile warnings, findbugs, pmd, mvn site report.
Carefully reviewed your PR first
- Fix common issues such as commented-out code, indentation
- Code looks clean and readable to yourself
- Use tools such as FindBugs or Eclipse itself to check and clean your code
- It saves time for you and reviewers
- The reviewers don't pay attention to the trivial stuff and can focus on logic or things that can't be easily found out by tools
PR
Your PR should be pleasant for others to review.
Every PR should have clear description what has changed, and why (if needed)
Write descriptions/comments
- for reviewer
- for clients who is going to call your APIs
- share knowledge
Add comments to your code - such as why the code is removed
Highlight potential issues, or things you want reviewers to pay attention
- Don't hide them, instead highlight them
- it's for your won good as reviewers help improve your code, find potential issues, also this makes yourself a better engineer
Your code should be pleasant for you and reviewers to read
- We not only write code, we write better code every day, write code that others want to use/call or read and maintain.
Write code for maintainer, reviewers, you in the future
Add comments to your code
- why you made the change this way: especially when there re other obvious approach, but maybe not work
- how you find the bug (internet link)
- when you think others(maybe yourself in the future) maybe confused
- write comments for your client/caller, how they should use it, what's the trick part
When others/reviewers ask questions about the code or when they feel difficult to understand it
It may be because others lack of the specific knowledge of the domain(logic) or it may (more likely) be a sign that we should improve the code, make it more readable
Share your knowledge to the team
- Such as in order to implement the PR, you learned some things new
- You don't have to share if it's something basic or we can find it via simple google search
- It's good to share if it takes you a lot of time, or you just want to share :)
During Code Review
Prefer to use PR to communicate
- as others would also know the conversation
- unless it's difficult to describe in words
- transparent and share knowledge
- What bugs we made, and how someone detect in code review
- how the reviewer reviews the code and finds out the issue
Don't forget the big picture
- What's the goal/new function of the PR
- Whether it solves the problem, implements the new function
What questions to ask?
- Not trivial things such as What is XX?
- Google it first, unless you still didn't find your answers
- Or you are the manager, tech skills/knowledge is not your primary goals any more
Use IDE to help understand the code if necessary
- Check the code change in Eclipse can help you to understand the code if you are not familiar with the function,
What may go wrong, what code may look suspicious, dangerous, have potential bugs, need pay extra attention, common source of bugs.
Whether the change improves code quality or make it worse?
Is there enough tests?
- Unit/Integration tests
- Happy, bad paths or corner cases
Admin what you don't know and ask others to review that part.
Ask for clarification/explanation if can't understand it.
Does it need documentation or notify other teams?
Using new features in the language?
- such as JDK8 time api, stream, new methods: getOrDefault, compute, putIfAbsent, computeIfAbsent etc
Don't invent here
- Whether we should use some well-known libraries, instead write our own code.
- Things that looks strange, that I don't understand or can't understand easily
Whether the new code is consistent (with existing code)
-- such as method name, order of parameters.
Focus on Main Function/Logic
-- Focus on the code that looks complicated, difficult to understand
-- Don't skip it if you can't understand it.
Performance
Whether the api is performance sensitive?
- Like database/networks calls
- If so, how long it usually takes in normal/extreme case?
- Is there duplication? do we need cache? or save the result first into a variable or a map?
--Did the code do enough optimization? such as use thread pool, call third part services asynchronously, in parallel etc.
Do we need provide timeout for block call?
If so, whether the timeout value makes sense or is configurable?
Thread safe
Concurrent programming is hard, so pay more attention.
Example: Don't Use Mutable Variables in Multiple Threads
Variables may be Changed Unexpectedly in Multiple Threads
- Use mutable instance fields in services.
-- SimpleDateFormat
Does it reuse threadpool, is it configured correctly?
- Don't recreate threadpool and shutdown in each method, reuse it.
Is there really benefit to use multiple threads?
Is there race condition?
Is lock used correctly?
Code Quality
SOLID
Whether the method/class does only one thing?
Big interfaces: split them
use polymorphism to remove if.
Preconditions cannot be strengthened in a subtype.Postconditions cannot be weakened in a subtype.
Review the data model
- As it's much harder to change data model after deployed to production.
-- is there any not needed fields?
How it is going to be used?
-- use redundant field/data to boost performance (avoid join query?)
-- will we search on it, define it as separate field, or store it as part of json object?
Is it using Right Data structures?
What's the frequent operations on the data?
If need search often: uses Set/Map instead of List
If there is a Key in the data, use Map instead of List.
Don't use mutable object inSet, or as Key in Map
Consider using Guava Multimap instead of Map<K, List<V
-- Multimap put: the case when the key doesn't exist, get returns empty collection instead of null.
Is class in right place - module/application or package?
Whether it should be an instance field?
Security
Do we need protect the API?
- If so, which role?
Are we storing sensitive info?
Should we audit the operation?
Code Quality
Bad Smell
Magic Number/String
- change to use constant/enum and give a meaningful name.
Limit the number of parameters per unit to at most 4.
- extract parameters into objects.
Parameters
Too many parameters
Parameters are easily to be mixed
Use boolean as parameter -- consider to use enum or use different names.
Big Class/Methods
Method
Usually method should be short and easy to read, no too many nests.
Complex methods
-- extract small methods
Separate Concerns to different classes/functions
API Design
Think from client perspective.
Immutability
Unmodifiable of parameter, return value
- defensive copy if needed
Whether API is easier to use, test/rollback in production
Whether the code is easy to read and understand?
-- If not, ask the developer to clarify or add comments to the code, or refactor the code.
Bad signs - use boolean parameters, return Pair, long parameter lists
Naming
Naming(variable/function/class name) is important to make the code readable.
Whether names of variables, method, class are meaningful?
-- 20 Tips for Better Naming
Good names should answer all the big questions, be precise and thorough, reveal intent, and follow conventions.
-- Avoid use vague variable names.
waitTime -> waitTimeInMs
Arguments
Number of Method Arguments should be as small as possible.
- smaller is better, remove unused parameters
Order of arguments
- especially when the methods/constructors are overloaded.
Order of statements
-- Put related statement together
-- whether some commands/functions should be always last one? If so, is it documented?
API design
Unnecessary code
Is there any unnecessary code(field/methods/function)?
Should it be define as constant, in property file or stored in db?
Prefer to store in db, so we can change it dynamically.
Do we need wire on/off feature?
- Consider adding turn-on flag for for new features
Code Duplication - Don't repeat yourself
Does it cause duplication?
Is there anything that can be extracted and reused in future?
Whether this is common/util function?
Whether the function is put in right place?
Tools like PMD/CPD can help find copy-paste code.
Similar Methods:
Extract common logic?
Use Holder object to contain all data?
Before create a new constant/util methods, search the code base first to make sure it's not already there.
Common Bugs
NPE
- Change to use Optional, or use annotations: such as @Notnull, @CheckforNull
-- if(anint == anInteger), anint == null,
anInteger = boolean?anint:anInteger
-- Use NullSafe API:
Objects.equals, MoreObjects.firstNonNull, StringUtils...
< or <+, off-by-one error
isEmpty or isNotEmpty?
Dangling If/Missing else
Missing default in switch
Allocate Unlimited Resources
-- Executors.newCachedThreadPool();
-- Create threads, make db/network calls in loop
Call expensive operations in for loop or for every record.
Look for signs of questionable code
Deep nesting of loops and conditionals.
AutoDetectParser instead of JpegParser
Not Needed Code
Remove commented code
YAGNI
Developer may write code that is not needed - such unused field/functions/features
Don't guess at future functionality.
Be cautious of clever implementation.
Is it over-engineered?
Whether there is enough logging to help debug if the function doesn't work as expected or to prove the code work.
Logging more info in lower log level.
Monitoring
Should we add monitor info such as API's latency/frequency?
Don't
Don't return array
Mics
- Non-static inner class, anonymous class, lambda uses method or field from the enclosing class
- Prefer to use static inner class
- Whether it may cause memory leak
- Use Optional instead return null
- Use Collections.emptyList/List.of()/ etc instead of new empty ArrayList
- Use new features unnecessary or inappropriately
- In some cases, the code is more efficient to just use regular for loop instead of stream
- we can collect/update all variables in one loop rather than use multiple streams(traverse) to update them separately
- Miss @Override
Testing
Json and Java serialization and deserialization of Http API response data
How to write the perfect pull request
thoughtbot - Code Review
Using Java Code Quality Tools to Identify Bugs
The Problem
The following code will return NPE, when the instance Integer field - integerFlag is null, but it's difficult to capture the error when review the code.
The fix is to change the == to: Objects.equals(integerFlag, 0)
-- Use Objects.equals to compare equals as it's null safe.
-- Use common util libraries such as CollectionUtils.isEmpty etc.
But how can we utilize code analysis tool to capture this kind of errors for us?
In Java, we can integrate findbugs, pmd, Sonar in maven, then run mvn site:site site:stage, the developers have to scan changed code and fix reported problem if needed before send it out for review.
This will make developers and reviewers life easier.
Tools to help detect bugs
Github link: https://github.com/jefferyyuan/code-quality-mvn
FindBugs
findbugs:gui, findbugs:gui, findbugs:check
Extensions
fb-contrib
PMD
pmd:pmd, pmd:cpd
http://pmd.sourceforge.net/pmd-4.3.0/rules/basic.html
@SuppressWarnings("PMD.AvoidUsingHardCodedIP")
mvn site:site site:stage
Integrate findbugs, pmd into mvn.
Facebook Infer
brew upgrade opam
brew update && brew upgrade opam
./build-infer.sh java
-- If it fails due to missing packages, uses opam install.
Infer on maven project
mvn clean && infer --debug -- mvn compile -o
too many open files on osx
sudo sysctl -w kern.maxfiles=20480
sudo sysctl -w kern.maxfilesperproc=22480
sudo ulimit -S -n 2048
Google Error Prone
https://github.com/google/error-prone/issues/376
Sonar
Code Analysis with SonarQube Plugin
Install and run Sonar server
mvn clean verify sonar:sonar
mvn verify -Pcoverage,jenkins -Dsonar.host.url=http://localhost:9000 sonar:sonar
Install plugins
http://localhost:9000/updatecenter/installed
https://wiki.jenkins-ci.org/display/JENKINS/Static+Code+Analysis+Plug-ins
Checker Framework
Run Maven example first.
Install checker eclipse plugin.
Use annotation in comments
/*>>>
import org.checkerframework.checker.nullness.qual.*;
import org.checkerframework.checker.regex.qual.*;
*/
Configure Eclipse Compiler Warnings
Enable null analysis, unbox conversion, missing default in switch etc
Leveraging JSR-305 null annotations to prevent NullPointerExceptions
Use @CheckForNull, @Nonnulls
Misc && Issues
Use -X to print more log and check the log
maven-compiler-plugin Unsupported major.minor version 52.0
Some plugins may only work with jdk8 or jdk7, use export to change JAVA_HOME to JDK8/7 and rerun.
The following code will return NPE, when the instance Integer field - integerFlag is null, but it's difficult to capture the error when review the code.
public int method() {
if (integerFlag == 0) { //or BoolanFlag == true
return;
}
//...
}
The fix is to change the == to: Objects.equals(integerFlag, 0)
-- Use Objects.equals to compare equals as it's null safe.
-- Use common util libraries such as CollectionUtils.isEmpty etc.
But how can we utilize code analysis tool to capture this kind of errors for us?
In Java, we can integrate findbugs, pmd, Sonar in maven, then run mvn site:site site:stage, the developers have to scan changed code and fix reported problem if needed before send it out for review.
This will make developers and reviewers life easier.
Tools to help detect bugs
Github link: https://github.com/jefferyyuan/code-quality-mvn
FindBugs
findbugs:gui, findbugs:gui, findbugs:check
Extensions
fb-contrib
PMD
pmd:pmd, pmd:cpd
http://pmd.sourceforge.net/pmd-4.3.0/rules/basic.html
@SuppressWarnings("PMD.AvoidUsingHardCodedIP")
mvn site:site site:stage
Integrate findbugs, pmd into mvn.
Facebook Infer
brew upgrade opam
brew update && brew upgrade opam
./build-infer.sh java
-- If it fails due to missing packages, uses opam install.
Infer on maven project
mvn clean && infer --debug -- mvn compile -o
sudo sysctl -w kern.maxfiles=20480
sudo sysctl -w kern.maxfilesperproc=22480
sudo ulimit -S -n 2048
https://github.com/google/error-prone/issues/376
Sonar
Code Analysis with SonarQube Plugin
Install and run Sonar server
mvn clean verify sonar:sonar
mvn verify -Pcoverage,jenkins -Dsonar.host.url=http://localhost:9000 sonar:sonar
Install plugins
http://localhost:9000/updatecenter/installed
https://wiki.jenkins-ci.org/display/JENKINS/Static+Code+Analysis+Plug-ins
Checker Framework
Run Maven example first.
Install checker eclipse plugin.
Use annotation in comments
/*>>>
import org.checkerframework.checker.nullness.qual.*;
import org.checkerframework.checker.regex.qual.*;
*/
Configure Eclipse Compiler Warnings
Enable null analysis, unbox conversion, missing default in switch etc
Leveraging JSR-305 null annotations to prevent NullPointerExceptions
Use @CheckForNull, @Nonnulls
Misc && Issues
Use -X to print more log and check the log
maven-compiler-plugin Unsupported major.minor version 52.0
Some plugins may only work with jdk8 or jdk7, use export to change JAVA_HOME to JDK8/7 and rerun.
How to Write Quality Code
Familiar with existing code in current project.
Familiar with common libraries or related projects in your company.
Look around before you make change
When you add field or function into a class which is extended or inherit from other classes, check whether it already existed in the class hierarchy, if so, reuse it and refactor the code.
Continuously refactor the code.
At least run every path in the changed code.
-- During dev, we may have to use debugger to change value or force throw exception.
More test.
Automate stuff.
Design data schema first
We can always refactor code but it's hard to change existing data and maintain data compatibility.
- Use ID(not name) as reference(in no-sql or solr).
- If there is some data is in-compatible(schema changed etc) in the query response, log it(and fix it later), but still return other data to client.
Readability
From Clean Code
Meaningful Names
Small function, classes.
Avoid long parameter lists.
Don’t add random check - only add a check if it may happen
Don't use boolean as parameter - never use three-state booleans
- use enum or split to different methods for constructor
- use static factory method
Don't return null
- Return Optional
- Or provide methods like getOrDefault()
Don't change input parameters
Put related code together
Don't swallow exceptions
Exceptions should be exceptional.
Always provide timeout
Use NPE-safe utils
- such as Objects.equals, apache commons
Use advanced data structures
- Use Multimap<K, V> instead of Map<K, List<V>>
Robust
Ask: What else?
Notice missing else or default in switch.
Ask: What may go wrong?
Be conservative or be liberal ?
Throw exception and reject the request or hide it and still service the request?
-- Depend on application and business, but be sure to think about it.
Security
Always validate input, set max length of input.
OWASP Top Ten Project
What should every programmer know about security?
A Guide to Building Secure Web Applications
Logging
Log what may help you trouble shoot the issue.
Logging for audit.
Practice
For example, to implement the function to upload image to CDN.
At first, build the basic version that works.
Then check the code to improve it.
What may go wrong?
Check uploaded file size.
Use Jersey @HeaderParam("Content-Length") and reject big file.
Use limitinputstream and throw exception when read more than max bytes.
Normalize file name and extension by replacing special character.
Limit length of file name.
Whitelist file types allowed
-- Use Tika to check file type.
Design Principles
S.O.L.I.D
Robustness Principle
Be conservative in what you send, be liberal in what you accept
Be conservative in what you do, be liberal in what you accept from others
Familiar with common libraries or related projects in your company.
Look around before you make change
When you add field or function into a class which is extended or inherit from other classes, check whether it already existed in the class hierarchy, if so, reuse it and refactor the code.
Continuously refactor the code.
At least run every path in the changed code.
-- During dev, we may have to use debugger to change value or force throw exception.
More test.
Automate stuff.
Design data schema first
We can always refactor code but it's hard to change existing data and maintain data compatibility.
- Use ID(not name) as reference(in no-sql or solr).
- If there is some data is in-compatible(schema changed etc) in the query response, log it(and fix it later), but still return other data to client.
Readability
From Clean Code
Meaningful Names
Small function, classes.
Avoid long parameter lists.
Don’t add random check - only add a check if it may happen
Don't use boolean as parameter - never use three-state booleans
- use enum or split to different methods for constructor
- use static factory method
Don't return null
- Return Optional
- Or provide methods like getOrDefault()
Don't change input parameters
Put related code together
Don't swallow exceptions
Exceptions should be exceptional.
Always provide timeout
Use NPE-safe utils
- such as Objects.equals, apache commons
Use advanced data structures
- Use Multimap<K, V> instead of Map<K, List<V>>
Robust
Ask: What else?
Notice missing else or default in switch.
Ask: What may go wrong?
Be conservative or be liberal ?
Throw exception and reject the request or hide it and still service the request?
-- Depend on application and business, but be sure to think about it.
Security
Always validate input, set max length of input.
OWASP Top Ten Project
What should every programmer know about security?
A Guide to Building Secure Web Applications
Logging
Log what may help you trouble shoot the issue.
Logging for audit.
Practice
For example, to implement the function to upload image to CDN.
At first, build the basic version that works.
Then check the code to improve it.
What may go wrong?
Check uploaded file size.
Use Jersey @HeaderParam("Content-Length") and reject big file.
Use limitinputstream and throw exception when read more than max bytes.
Normalize file name and extension by replacing special character.
Limit length of file name.
Whitelist file types allowed
-- Use Tika to check file type.
Design Principles
S.O.L.I.D
Robustness Principle
Be conservative in what you send, be liberal in what you accept
Be conservative in what you do, be liberal in what you accept from others
Subscribe to:
Posts (Atom)