Removing not needed parameter

[wiiitek]

As there is no embedded proxy we don't need a port configuration for IT.
REST proxy has its own configuration (with a port number).

[toniedzwiedz]

String proxyType = usedProxy;
if ("true".equals(usedProxy)) {
//...
proxyType = DEFAULT_PROXY_MANAGER;

The values that you assign to the proxyType variable here can be "true" and "embedded"... That's stringly-typed and I find it difficult to follow.

Did you actually mean to use the same variable? If so, consider using an explicit enum type to cover the possible values. If not, use separate variables for these values and use boolean when actually handling boolean variables.

[wiiitek]

Do I understand correctly you don't like having "true" passed as sting type value?
I have renamed the variables in order to make the code more clear.
However:

• it may be bad idea to use enum for proxy types, as we don't know how many type there will be in an AET system. someone could add new ProxyManager service with new type.
• the "true" value is passed from suite XML configuration but the useProxy attribute could have non-boolean values.

I can't make this code better :( Could you provide some code samples / commits / pull requests?

[toniedzwiedz]

There are two things I don't like

1. Passing boolean values as Strings. IMO, as soon as the value is read from the file, it should be converted to the most specific type possible in a given context.
2. Mixing values with seemingly disparate semantics.

Let's have a look at the current code.

String proxyType;
if ("true".equals(proxyTypeConfig)) {
    proxyType = DEFAULT_PROXY_MANAGER;
} else {
    proxyType = proxyTypeConfig;
}

I'm not sure I understand the meaning of the name proxyTypeConfig. What exactly does it mean? That the default config should be used? If so, rename it to something along the lines of useDefaultProxyType. Can you show me an example of the XML file where this is configured?

Looking at the whole code, it looks like the same variable (proxyTypeConfig)can have the values of "true", "embedded" or "rest". The first suggests it's a boolean, the latter two are different.

public ProxyServerWrapper createProxy(String proxyTypeConfig) throws ProxyException {
    String proxyType;
    if ("true".equals(proxyTypeConfig)) {
        proxyType = DEFAULT_PROXY_MANAGER;
    } else {
        proxyType = proxyTypeConfig;
    }

Perhaps the string literal "true" should be replaced with something that doesn't suggest boolean semantics. Maybe "default"?

On a side note, you could get rid of the null initialisation by converting the if-else block to a simple assignment:

String proxyType = "true".equals(proxyTypeConfig) ? DEFAULT_PROXY_MANAGER : proxyTypeConfig;

[wiiitek]

Thank you @toniedzwiedz . I have removed the changes related to webProxy config.
They should be consulted and addressed in another pull request in future.

[malaskowski]

This is clearly some configuration legacy. Before Browsermob, AET used another proxy (embedded) and then useProxy configuration could have 2 values: true or false. When the browsermob came, because of a backward compatibility, configuration was extended to rest and embedded.
In time, embedded proxy option was removed from the system as it didn't work well.
Thank you @wiiitek for noticing this.
I think we have 2 paths now:

• lets leave useProxy possible values as true and rest which are equivalent (true indicates that the default proxy will be used).
• there will be only named type of a proxy (remove true as a possible value) after deprecating it first for e.g. 2 minor versions?

What do you think?

[toniedzwiedz]

@Skejven

• lets leave useProxy possible values as true and rest which are equivalent (true indicates that the default proxy will be used).

Why not default and rest then?

• there will be only named type of a proxy (remove true as a possible value) after deprecating it first for e.g. 2 minor versions?

Sounds good too.

[wiiitek]

I have created new issue for that: #56

Could you please approve / update the description when you have time?

[wiiitek]

---

I hereby agree to the terms of the AET Contributor License Agreement.