Alon Bar-Lev has posted comments on this change.

Change subject: uutils: Extract connection class
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.ovirt.org/#/c/33479/4/backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/http/Connection.java
File 
backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/http/Connection.java:

> Not sure if HttpURLConnectionFactory is the correct name.
We produce HttpURLConnection, right? so this class is a factory of creating 
these with pre-defined settings... no?
Line 1: package org.ovirt.engine.core.uutils.http;
Line 2: 
Line 3: import java.io.FileInputStream;
Line 4: import java.io.IOException;


Line 146:     public HttpURLConnection getURLConnection() {
Line 147:         if (!(connection instanceof HttpsURLConnection)) {
Line 148:             throw new RuntimeException("The connection is not an HTTP 
or HTTPS connection");
Line 149:         }
Line 150:         return (HttpURLConnection) connection;
> This is the inner member Connection wraps.
I still think at least the create() should return HttpURLConnection... even if 
you want to use chaining.

 HttpURLConnection con = new 
HttpURLConnectionFactory().setX().setY.setZ().createHttpURLConnection();

 HttpURLConnectionFactory factory = 
HttpURLConnectionFactory().setX().setY.setZ();
 con1 = factory.createHttpURLConnection();
 con2 = factory.createHttpURLConnection();

there is no reason to hold connection as member.
Line 151:     }
Line 152: 
Line 153: 


-- 
To view, visit http://gerrit.ovirt.org/33479
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I85ea4e7301b3a018b0438fff25cefad80ebd7256
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: [email protected]
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to