Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check returns for null #189

Open
occupant23 opened this issue Jun 1, 2021 · 1 comment
Open

Check returns for null #189

occupant23 opened this issue Jun 1, 2021 · 1 comment

Comments

@occupant23
Copy link
Contributor

occupant23 commented Jun 1, 2021

Try to remove returning null where possible to improve our API.
Let's detect and discuss those areas first and change the API after the discussion.

E.g. return empty Strings or empty Collections or consider using Optional

@oomelianchuk
Copy link
Contributor

I found 6 null returns in total. These are:

  • Neodymium class line 187-191
    public static WebDriver getDriver()
    {
        final WebDriverStateContainer wDSC = getContext().webDriverStateContainer;
        return wDSC == null ? null : wDSC.getWebDriver();
    }
  • Neodymium class line 220-224
    public static BrowserUpProxy getLocalProxy()
    {
        final WebDriverStateContainer wDSC = getContext().webDriverStateContainer;
        return wDSC == null ? null : wDSC.getProxy();
    }
  • WebDriverCache class line 48-52
    public WebDriver getWebDriverByBrowserTag(String browserTag)
    {
        WebDriverStateContainer container = cache.get(browserTag);
        return container != null ? container.getWebDriver() : null;
    }
  • DataUtils class line 160 -170
    public static <T> T get(final String jsonPath, final Class<T> clazz)
    {
        try
        {
            return JsonPath.using(JSONPATH_CONFIGURATION).parse(getDataAsJsonObject()).read(jsonPath, clazz);
        }
        catch (PathNotFoundException e)
        {
            return null;
        }
    }
  • YamlProperties class line 163-190
    public static Properties build(final File file)
    {
        try (final Reader reader = new BufferedReader(new InputStreamReader(new FileInputStream(file), Charset.forName("UTF-8"))))
        {
            final Yaml yaml = new Yaml();

            final Map<String, Object> map = yaml.load(reader);

            if (map == null)
            {
                // the localization file is empty
                return null;
            }

            final YamlProperties yamlProperties = new YamlProperties();
            yamlProperties.yamlToJavaProperties(map);

            return yamlProperties.properties;
        }
        catch (final FileNotFoundException e)
        {
            return null;
        }
        catch (final IOException e)
        {
            return null;
        }
    }
  • NeodymiumProxyHttpClientFactory line 130-139
        @Override
        public Request authenticate(Route route, Response response) throws IOException
        {
            if (response.request().header(headerName) != null)
            {
                return null; // Give up, we've already attempted to authenticate.
            }
            return response.request().newBuilder()
                           .header(headerName, credentials)
                           .build();
        }

IMO, we could replace all the null returns with optional ones. The last method (NeodymiumProxyHttpClientFactory line 130-139) wouldn't allow us to do so due to overriding, so I suggest throwing an exception in case of authentication failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants