code cleanup along with ws 2.0 #7

SameerShaikhAcc · 2023-04-13T15:47:02Z

The code improvements and changes are based on the JIRA ticket https://angelbrokingpl.atlassian.net/browse/SSS-13 and the feedback from the pull request kadam-mithun#1.

The following changes are implemented:

Replaced the use of + notation with StringBuilder for better performance.
Renamed attributes to follow camelCase convention and added SerializedName separately for improved readability.
Removed commented code.
Eliminated redundant usage of SerializedName annotation.
Implemented Lombok for getter and setter methods.
Moved constants to a separate class for better organization.
Used the HTTP library for status code messages.
Removed redundant return statements to simplify the code.
handled test cases
updated readme.md

NakshArora · 2023-04-20T10:25:27Z

README.md

+    public void getOrder(SmartConnect smartConnect) throws SmartAPIException, IOException {
+        List<Order> orders = smartConnect.getOrderHistory(smartConnect.getUserId());
+            for (Order order : orders) {
+            logger.info(order.orderId + " " + order.status);


We should use logger.info("{} {}", order.orderId, order.status)

have replaced String concatenation with place holder
log.info("{} {}", order.orderId, order.status);

NakshArora · 2023-04-20T10:25:37Z

README.md

+            // Returns tradebook.
+            List<Trade> trades = smartConnect.getTrades();
+            for (Trade trade : trades) {
+            logger.info(trade.tradingSymbol + " " + trades.size());


have replaced String concatenation with place holder
log.info("{} {}", trade.tradingSymbol, trades.size());

NakshArora · 2023-04-20T10:25:52Z

src/main/java/com/angelbroking/smartapi/Routes.java

 import java.util.Map;

 /**
 * Generates end-points for all smart api calls.
- *
+ * <p>


have removed the "< p >" from comments

NakshArora · 2023-04-20T10:26:36Z

src/main/java/com/angelbroking/smartapi/SmartConnect.java


+@Data


Why do we need this? Ideally the request body for smartConnect and the methods should be segregated into 2 files.

have segregated SmartConnect into 2 files SmartConnect and SmartConnectParams
POJO methods have been moved to SmartConnectParams

NakshArora · 2023-04-20T10:26:41Z

src/main/java/com/angelbroking/smartapi/SmartConnect.java

-		}
-	}
+
+    private static final Logger logger = LoggerFactory.getLogger(SmartConnect.class);


Please use logger from lombok @slf4j

have used @slf4j instead of private static final Logger logger = LoggerFactory.getLogger(SmartConnect.class);

NakshArora · 2023-04-20T10:34:34Z

src/main/java/com/angelbroking/smartapi/http/exceptions/DataSmartAPIException.java


    // initialize 2fa exception and call constructor of Base Exception
-    public DataException(String message, String code){
+    public DataSmartAPIException(String message, String code){


Why the above comment is required.

comment is been removed

NakshArora · 2023-04-20T10:35:12Z

src/main/java/com/angelbroking/smartapi/models/Gtt.java

-	public Integer timePeriod;
-
+	private Integer timePeriod;
+
 	@Override
 	public String toString() {


In almost all the model classes we see this toString, Are we using this toString somewhere?

unnecessary toString is been removed

NakshArora · 2023-04-20T10:36:00Z

src/main/java/com/angelbroking/smartapi/sample/Examples.java

+    public void getHolding(SmartConnect smartConnect) throws SmartAPIException {
+        // Returns Holding.
+        JSONObject response = smartConnect.getHolding();
+        logger.debug("getHolding {}", response);


As mentioned last time, please remove these debug statements. Please review the dentire code for these debug statements.

have removed some debug statements and replaced some with log.info or log.error if necessary.

NakshArora · 2023-04-20T10:36:14Z

src/main/java/com/angelbroking/smartapi/sample/LoginWithTOTPSample.java


 public class LoginWithTOTPSample {
-
+	private static final Logger logger = LoggerFactory.getLogger(LoginWithTOTPSample.class);


Use logger from @slf4j

have used @slf4j

NakshArora · 2023-04-20T10:36:47Z

src/main/java/com/angelbroking/smartapi/sample/SmartStreamListenerImplTest.java

+                (quote.getLowPrice() / 100.0),
+                (quote.getClosePrice() / 100.0),
+				getExchangeTime(quote.getExchangeFeedTimeEpochMillis()),
+                Instant.now().toEpochMilli() - quote.getExchangeFeedTimeEpochMillis());


We are not making any asserts in these tests. What are we really testing?

SmartStreamListenerImpl is not a test class it is used in SmartStreamTicker.
SmartStreamTicker is used for connecting with the webSocket

NakshArora

Please review the comments..

kadam-mithun · 2023-04-26T06:05:15Z

README.md

-
-		JSONObject requestObejct = new JSONObject();
+		
+        JSONObject requestObejct = new JSONObject();


Please use Model class instead of JSONObject

kadam-mithun · 2023-04-26T06:08:02Z

doc/com/angelbroking/smartapi/Routes.html

@@ -100,7 +100,7 @@ <h2 title="Class Routes" class="title">Class Routes</h2>
 <li>java.lang.Object</li>
 <li>
 <ul class="inheritance">
-<li>com.angelbroking.smartapi.Routes</li>
+<li>com.angelbroking.smartapi.routes.Routes</li>


Why did you change this?

kadam-mithun · 2023-04-26T14:19:29Z

src/main/java/com/angelbroking/smartapi/SmartConnect.java

+        } catch (IOException ex) {
+            log.error("{} {}", IO_EXCEPTION_OCCURRED, ex.getMessage());
+            throw new IOException(String.format("%s: %s", IO_EXCEPTION_ERROR_MSG, ex.getMessage()));
+        } catch (JSONException ex) {


Do you think we should catch JSONException, not able see any json inside method

kadam-mithun · 2023-04-26T14:38:19Z

src/main/java/com/angelbroking/smartapi/sample/Examples.java

+        orderParams.setSymbolToken("3045");
+        orderParams.setProductType("INTRADAY");
+        orderParams.setOrderType("LIMIT");
+        HttpResponse order = smartConnect.placeOrder(orderParams, "NORMAL");


Map to model instead of HttpResponse

moizm89 · 2023-05-29T16:54:53Z

src/main/java/com/angelbroking/smartapi/utils/Constants.java

+    public static final String LOGIN_URL = "https://apiconnect.angelbroking.com/rest/auth/angelbroking/user/v1/loginByPassword";
+    public static final String WSURI = "wss://wsfeeds.angelbroking.com/NestHtml5Mobile/socket/stream";
+//    public static final String SMARTSTREAM_WSURI = "ws://smartapisocket.angelone.in/smart-stream";
+    public static final String SMARTSTREAM_WSURI = "ws://mds-uat.angelone.in/smart-stream";


this is UAT URL. Please switch to prod URL

have changed SMARTSTREAM_WSURI from ws://mds-uat.angelone.in/smart-stream to ws://smartapisocket.angelone.in/smart-stream

doc/com/angelbroking/smartapi/Routes.html

doc/com/angelbroking/smartapi/class-use/Routes.html

NakshArora · 2023-05-30T18:43:57Z

src/main/java/com/angelbroking/smartapi/SmartConnect.java

+            return ResponseParser.parseResponse(smartAPIRequestHandler.getRequest(smartConnectParams.getApiKey(), url, smartConnectParams.getAccessToken()));
+
+        } catch (IOException ex) {
+            log.error("{} {}", IO_EXCEPTION_OCCURRED, ex.getMessage());


log messages should clearly denote that the error occurred while doing which operation?
In case an issue arises in the future, how can we find out that the error occurred while doing which operation?
Log should look something like "Error occurred while getting user profile" with ex.getMessage()

This needs to be addressed at all places.

have done changes in SmartConnect class for all log messages as well as for exception

NakshArora · 2023-05-30T18:45:04Z

src/main/java/com/angelbroking/smartapi/SmartConnect.java

+        try {
+            HttpResponse httpResponse = smartAPIRequestHandler.postRequest(smartConnectParams.getApiKey(), routes.getLoginUrl(), Utils.createLoginParams(clientCode, password, totp));
+            responseDTO = new ObjectMapper().readValue(httpResponse.getBody(), UserResponseDTO.class);
+            String url = routes.get(SMART_CONNECT_API_USER_PROFILE);


Can we please split this into 2 try catch blocks. One for postRequest and the other one for parsing the object. This is creating unnecessary confusion.

As discussed, will be using a single-try catch block.

NakshArora · 2023-05-30T18:46:21Z

src/main/java/com/angelbroking/smartapi/SmartConnect.java

+    public HttpResponse placeOrder(OrderParams orderParams, String variety) throws SmartAPIException, IOException {
+        try {
+            Validators validator = new Validators();
+            orderParams.setVariety(variety);


Please put the line which is required to be in the try-catch block. L141-L144 need not be inside the try catch block.

This needs to be done at all places.

have removed unnecessary code from try-catch block

NakshArora · 2023-05-30T18:56:55Z

src/main/java/com/angelbroking/smartapi/smartticker/SmartWebsocket.java

+        Routes routes = new Routes();
+        try {
+            StringBuilder sb = new StringBuilder();
+            sb.append(routes.getSWsuri()).append("?jwttoken=").append(this.jwtToken).append("&&clientcode=").append(this.clientId).append("&&apikey=").append(this.apiKey);


Please move constants to separate file.

have moved to constants

NakshArora · 2023-05-30T18:58:13Z

src/main/java/com/angelbroking/smartapi/smartstream/ticker/SmartStreamTicker.java

+                    switch (mode) {
+                        case LTP: {
+                            ByteBuffer packet = ByteBuffer.wrap(binary).order(ByteOrder.LITTLE_ENDIAN);
+                            LTP ltp = ByteUtils.mapByteBufferToLTP(packet);


We can simply write it as mapByteBufferToLTP(packet) instead of ByteUtils.mapByteBufferToLTP(packet);

Done for LTP,Quote,Snap_quote

NakshArora · 2023-05-30T18:58:41Z

src/main/java/com/angelbroking/smartapi/utils/ResponseParser.java

+//    Added a private constructor to hide the implicit public one.
+
+
+    private ResponseParser() {


do you want me to remove the constructor or comment or both?

as discussed have removed the comments

NakshArora · 2023-05-30T18:59:38Z

src/main/java/com/angelbroking/smartapi/utils/Utils.java

+                if (macAddressBytes != null) {
+                    StringBuilder macAddressStr = new StringBuilder();
+                    for (int i = 0; i < macAddressBytes.length; i++) {
+                        macAddressStr.append(String.format("%02X%s", macAddressBytes[i], (i < macAddressBytes.length - 1) ? "-" : ""));


Please move constants.

NakshArora · 2023-05-30T18:59:50Z

src/main/java/com/angelbroking/smartapi/utils/Utils.java

+            log.error("Error loading configuration file: {}", e.getMessage());
+            throw new IOException("Failed to load configuration file");
+        }
+        URL urlName = new URL(properties.getProperty("url"));


NakshArora · 2023-05-30T19:03:20Z

Please add meaningful log messages for easy debugging in case of issues.
Please review the statements to be kept in the try-catch blocks.
Review Exceptions.
Move constants to separate file.
Remove Html files.
Remove default constructors.
Meaningful variable names.

on error method added

Sameer Shaikh added 24 commits March 30, 2023 10:17

websocket 2.0 enhancement

5fefe6a

code improvements till 40

df69911

websocket 2.0 enhancement #3

b5a25fe

websocket 2.0 enhancement #4

e742326

websocket 2.0 enhancement #5

4529716

websocket 2.0 enhancement #6

e5a33db

websocket 2.0 enhancement #7

3905781

websocket 2.0 enhancement #8

24dfac2

websocket 2.0 enhancement #9

ca63fc2

websocket 2.0 enhancement #9

a4ffd8b

websocket 2.0 enhancement #9

798da5a

websocket 2.0 enhancement #9

9bf3bab

websocket 2.0 enhancement #10

5e0a8b5

websocket 2.0 enhancement angel-one#11

0ba4baf

websocket 2.0 enhancement angel-one#12

bc52f7e

websocket 2.0 enhancement angel-one#13

1b9b2d5

websocket 2.0 enhancement angel-one#13

416e3b1

websocket 2.0 enhancement #1

7f3be2a

websocket 2.0 enhancement #1

63d1f37

websocket 2.0 implementation #1

9bfff06

websocket 2.0 implementation #2

d4f9d68

websocket 2.0 implementation #3

ef140b5

added doc folder in .gitignore

b2ccab5

ResponseParser.java changes

ccabfcc

SameerShaikhAcc force-pushed the websocket-2-implementation branch from 81432a6 to ccabfcc Compare April 17, 2023 05:50

Sameer Shaikh added 3 commits April 17, 2023 11:26

Removing doc as it is no longer needed.

34e1baf

websocket 2.0 implementation #4

40fb3d8

websocket 2.0 implementation #5

d5f3b8e

NakshArora suggested changes Apr 20, 2023

View reviewed changes

working 12.0

888f048

kadam-mithun reviewed Apr 26, 2023

View reviewed changes

SameerShaikhAcc added 5 commits April 26, 2023 12:55

working 12.0

edef508

working 13.0

fcc34e4

working 14.0

e0691fc

working 15.0

fcf7c2f

working 16.0

d48a3fd

kadam-mithun reviewed Apr 26, 2023

View reviewed changes

SameerShaikhAcc added 8 commits April 27, 2023 10:33

working 17.0

1b0a3f0

working 18.0

a337f71

working 19.0

62df4ee

working 20.0

c161583

constant changes

9cc5fbf

changes in historical candle data

a717ea2

add tests in SmartWebsocket

949650c

Changes in readMe.md and handled test cases

592d1e2

moizm89 reviewed May 29, 2023

View reviewed changes

SMARTSTREAM_WSURI prod url changed

6d4c4dc

NakshArora suggested changes May 30, 2023

View reviewed changes

SameerShaikhAcc added 4 commits May 31, 2023 13:31

changes requested in PR Done

c8e3d13

changes requested in PR Done

bdb9427

changes in ltp ,quote , snapquote

239c6c9

changes in sample test class

0201d83

moizm89 added the DO-NOT-MERGE label Jun 21, 2023

moizm89 changed the title ~~Websocket 2 implementation~~ code cleanup along with ws 2.0 Jun 21, 2023

added testing details

c06dea1

anurag-amx pushed a commit that referenced this pull request Nov 28, 2023

Merge pull request #7 from angel-one/main

154386d

on error method added


		public class LoginWithTOTPSample {

		private static final Logger logger = LoggerFactory.getLogger(LoginWithTOTPSample.class);


		JSONObject requestObejct = new JSONObject();

		JSONObject requestObejct = new JSONObject();

		// Added a private constructor to hide the implicit public one.


		private ResponseParser() {


		@Data

code cleanup along with ws 2.0 #7

Are you sure you want to change the base?

code cleanup along with ws 2.0 #7

Conversation

SameerShaikhAcc commented Apr 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SameerShaikhAcc Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SameerShaikhAcc Apr 25, 2023 • 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

NakshArora left a 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

SameerShaikhAcc May 30, 2023 • 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

NakshArora commented May 30, 2023

SameerShaikhAcc commented Apr 13, 2023 •

edited

Loading

SameerShaikhAcc Apr 25, 2023 •

edited

Loading

SameerShaikhAcc Apr 25, 2023 •

edited

Loading

SameerShaikhAcc May 30, 2023 •

edited

Loading