Due to some constraints I had to manually parse an HTTP response coming out of a stream.
I could not use buffered reader or scanner because the perform buffering and end up buffering bytes beyond the response header itself.
I'll be hitting my AWS S3 and only expect 200 or 206 status response.
This seems to work, but I'd appreciate some review.
final InputStream reader = socket.getInputStream();stringBuilder.setLength(0);//first fetch headerwhile (true) { final char read = (char) reader.read(); if (read < 1) { //fail MiscUtils.closeAndIgnore(socket, reader); return false; } stringBuilder.append(read); if (read == '\n') break; //status header fetched}//check statusfinal String status = stringBuilder.toString();Log.i("Downloader", status);if (!(status.contains("200") || status.contains("206"))) { //fail MiscUtils.closeAndIgnore(socket, reader); return false;}//now consume the headerstringBuilder.setLength(0);while (true) { final char read = (char) reader.read(); if (read < 1) { MiscUtils.closeAndIgnore(socket, reader); return false; } stringBuilder.append(read); final int currentCount = stringBuilder.length(); if (currentCount > 4 && stringBuilder.charAt(currentCount - 1) == '\n' && stringBuilder.charAt(currentCount - 2) == '\r' && stringBuilder.charAt(currentCount - 3) == '\n' && stringBuilder.charAt(currentCount - 4) == '\r') { break; //response consumed }}//print the headerfinal String completeResponse = stringBuilder.toString();Log.i("Downloader", completeResponse);
1 Answer1
This is a bit surprising:
stringBuilder.setLength(0);
Seeing the posted code starting with this is fishy,as it suggests that there is more code before it,and that the method is not decomposed enough,and should be multiple methods instead.
This is a bit surprising:
final char read = (char) reader.read(); if (read < 1) {
Theread() method returns -1 in case of EOF.I suggest to change this check toread < 0 orread == EOF to avoid making reviewers ponder.
MiscUtils.closeAndIgnore(socket, reader);
A common name for this kind of thing iscloseQuietly,usually in a utility class calledIOUtils (in Apache commons, for example).I suggest to follow that good example.MiscUtils is a bad idea.
if (read == '\n') break; //status header fetched
It's recommended to use braces always, even with single-statement expressions like this.
if (!(status.contains("200") || status.contains("206"))) {
I'd go for the more strict and compact regular expression check:
if (!status.matches("HTTP/.* 20[06] .*")) {It would be good to split the posted code to two methods:
- A method that consumes the status line (the first line)
- A method that consumes the remaining lines
You mustlog in to answer this question.
Explore related questions
See similar questions with these tags.