18

if (condition == true) Instead of if (testDataGenerated == true) { just write if (testDataGenerated) { Rewriting ifs You can move your checks to the top of the method like this to avoid nested ifs: if (!testDataGenerated) { WriteTo.testgoal(nameTestcase, TestgoalState.NOK, "testdata is not generated"); throw new ...


15

As the code is similar, they can be merged together. I'll suggest you to break down the functions into smaller reusable chunks which will follow the Single Responsibility Principle (SRP). These functions will be testable and reusable. Both functions are Adding a delay of 500 milliseconds Selecting elements from provided page Clicking the first element ...


12

Are you sure that you need a Boolean? Considering how your code looks, currentPage and testDataGenerated cannot be null (if they would be then they would throw a NullPointerException. Use boolean instead which cannot be set to null Try to avoid throwing Exception, create or use a subclass of Exception instead. Speaking of "make the code read easier", there ...


11

Your IsElementVisible method can be condensed to: public bool IsElementVisible(IWebDriver driver, By element) { return driver.FindElements(element).Count > 0 && driver.FindElement(element).Displayed; } I guess the method IsElementNotVisible should return boolean negation of result of the IsElementVisible method: public bool ...


9

I have no familiarity with any of the libraries used here, so I can't comment on their usage. What I will mention though is the giant chunk of profile.set_preference calls in the middle of the script. It would be much cleaner and less repetitive to save the string/bool pairs of options as a dictionary (or another "paired" structure), then just iterate over ...


8

Test review It is not clear from the code in your upper code block, whether you are doing three separate tests, or if that is one test method doing three assertions. Anyway it is common to strive for that one test method has only one assertion. Having multiple tests in one test method is considered a bad test regime. Using Assert.Fail() is also somewhat ...


7

You don't have to recreate the Select instances, define once and reuse: currency = Select(self.driver.find_element_by_id("currencyid")) excur = Select(self.driver.find_element_by_id("excur")) currency.select_by_visible_text("AUD") excur.select_by_visible_text("AUD") assert currency.first_selected_option.text == "USD" # should not be "AUD"? assert excur....


7

Code Flow: you are redefining links on every iterations of the while loop - you basically need to do it once as a while loop exit condition, we can use the fact that there are line numbers in the company list grid - we can simply wait for the number 1000 to show up while scrolling I would also create a class to have the driver and WebDriverWait instance ...


7

Here is a collection of things to improve. Code Style Notes Overall, there is a number of PEP8 Code Style violations, like lack of spaces around operators, blank lines, spaces before the # comment character etc. Make sure to address them. You can also use tools like flake8 or pylint to statically analyze code for common PEP8 and other code quality issues. ...


6

Python I know you're used to Java, which is probably why you've put everything in classes in separate files. @SuperBiasedMan commented on this, but I want to stress this. Python is not a strictly object oriented language like Java, you can mix different approaches as you see fit. ListOfFilesInDirectory is a good candidate to be a function, because it doesn'...


6

The method appears to have some pointless operations. You loop over a List<WebElement>, to build a Map<String, WebElement>, where the key of the map is the text in a WebElement in the list. But you use only one key in the map, the String filter parameter received by the method. As such, you don't need a map at all. This implementation is (...


6

Style Case: Python's official style guide, PEP8, states you should use snake_case naming for your variables instead of camelCase: getMostRecentActivityTwitter Magic numbers: You should avoid unexplained numbers through your code, like this: if '40' this is confusing to read and would be better explained through a simple temporary variable. Spacing ...


6

Overall, it looks clean, but here are some potential improvements: XPaths, generally, don't handle multi-valued class attributes well - you would have to workaround it with concat to make reliable. A better way would be a CSS selector - here are all 3 XPaths replaced with a relevant CSS selector: wait.until(EC.visibility_of_element_located((By.CSS_SELECTOR,...


6

I will make a few points about the code as code, but the first thing to consider in any sort of data collection is respecting the rights of the owners of the data. In particular the terms of service on the NHL site state You may not access or use, or attempt to access or use, the Services to take any action that could harm us or any other person or ...


5

Handling HTTP errors This code seems to be intended to handle 404 Not Found response when downloading a url: # Grab the page, raising an exception if it doesn't work response = requests.get(url) if '40' in str(response): raise Exception('Couldn''t find page!') Unfortunately this is very inappropriate, for several reasons: The code is counting on str(...


5

This could just as easily be me being too unused to OOP but I don't see the point of your class. You only call it once, it only contains two methods and __init__ could easily be supplanted by just taking the directory and extension as parameters to list_of_files. Really I'd change it to just a function that accepts the parameters directly. Also you can ...


5

Your code looks hard to read and not like Python code. The main reason for this is that every second line is a comment. Most of these comments just repeat what the surrounding code does. These comments don't have any positive value and should therefore be removed. The other reason is that you didn't structure the code into paragraphs. After each chunk of ...


5

Sub Fortune_parser() The procedure is implicitly Public; I'd make it explicitly so. It's also using a rather uncommon casing style I don't even have a name for. In every single VBA type library out there[citation needed], public members are PascalCase. Note that classes that expose public members that have underscores in their names, cannot be used with ...


5

I would improve the following things: implement a "crawler" class that will store the "driver" instance as a variable and will also have a better separation of concerns - a separate method to log in, a separate method to get friends etc stop using time.sleep() for waits with selenium - hardcoded time delays are seriously unreliable - use Explicit Waits ...


5

I would recommend a more modular design - having a LinkedInScraper class, initialized with a login and password and with separate methods for logging in and getting profile links. Also, I think you are overusing the XPaths overall - whenever possible, first explore if you can use "by id", "by name" or "by css selector" locators and fall back to XPath only ...


5

The spider is readable and understandable. I would only extract some of the things into separate methods for readability. For example, the "infinite scroll" should probably be just defined in a separate method. And, the bare except can be replaced with handling a more specific TimeoutException: def scroll_until_loaded(self): check_height = self.driver....


5

There is a room for improvement - like there always is: avoid bare exception handling. Instead of having an except clause without specifying exception classes to handle, you can handle TimeoutException (which you even already have imported) use with context manager when dealing with files instead of using the find_elements_by_css_selector() method and then ...


5

You can try approaching the problem without using an actual browser and sending HTTP requests using requests and parsing HTML using, say, BeautifulSoup. This though would involve diving into how the page is formed, is more error-prone and would be far more suspicious and easier to detect in general. Of course, with no browser overhead, this would be much ...


5

A few more comment to add after @Josiah good answer: You can simplify the driver management in scrape_nhl_standings by using a context manager. It seems like selenium doesn't support it natively but you can make use of contextlib.closing: from contextlib import closing def scrape_nhl_standings(csv_dump_path,start_year, end_year): with closing(...


4

Judging by the overall structure it looks quite good, I would change a few things though with respect to formatting: Your public static void selectHeader() { line seems to be off, might've been due to putting the code here, you should remove 4 blanks. Take as example this line: wait.until(ExpectedConditions.visibilityOfElementLocated(By .xpath("//*[...


4

Names of variables should be chosen well. If you name a variable currentPage, I would assume that it contains a page number (1, 2, 3, 4, etc). If you want a boolean variable that is true if we are on the current page, you could call it isCurrentPage. If you want a boolean variable that is true if synchronization to the current page succeeded (I have no idea ...


4

The other answers have provide some good suggestions about the code formatting and styling, so here's mine on the logic. The way I see it, you only need to call verifyEventDetailFull() when the test data is generated and currentPage is true (I'm not quite sure why the title containing a certain value is 'current page', but that's beyond my point). ...


4

The Page Object Model will help to remove information about the page from your tests. To use this pattern well, you have to think of it more like a "widget object model" as you don't want to represent the whole page with one class, but more like a related widget, like the "search feature". So you would create a class that has private members for each of the ...


4

The method names are consistent, but usually camelcase is advocated. Indentation is inconsistent, but maybe that's from pasting it here? Variables like usern and userp are bad, why not name and password or something that's readable? Why is the window maximised? The site doesn't look that's necessary for anything. Consider having a login method instead of ...


4

Bug This was probably a posting or a copy/paste bug - in create_file() you meant to name the first parameter to be filename instead of title. Code Style and PEP8 notes use more descriptive variable names - r, s, c, v are not good variable names remove unused TimeoutException import, organize imports in groups and alphabetically (PEP8 reference) move the ...


Only top voted, non community-wiki answers of a minimum length are eligible