https://github.com/SeanDaBlack/KelloggBot/blob/main/req.py
Edit: The code is not PERFECT as MANY people have picked it apart below.
But if I had to work with this code on a daily basis I would certainly not mind.
If you ever need a code review just post your code on hacker news and say it's well written....
https://www.selenium.dev/documentation/test_practices/encour...
public class Login {
public void testLogin() {
// fill login data on sign-in page
driver.findElement(By.name("user_name")).sendKeys("userName");
driver.findElement(By.name("password")).sendKeys("my supersecret password");
driver.findElement(By.name("sign-in")).click();
// verify h1 tag is "Hello userName" after login
driver.findElement(By.tagName("h1")).isDisplayed();
assertThat(driver.findElement(By.tagName("h1")).getText(), is("Hello userName"));
}Into this?:
public class SignInPage { protected WebDriver driver;
// <input name="user_name" type="text" value="">
private By usernameBy = By.name("user_name");
// <input name="password" type="password" value="">
private By passwordBy = By.name("password");
// <input name="sign_in" type="submit" value="SignIn">
private By signinBy = By.name("sign_in");
public SignInPage(WebDriver driver){
this.driver = driver;
}
/**
* Login as valid user
*
* @param userName
* @param password
* @return HomePage object
*/
public HomePage loginValidUser(String userName, String password) {
driver.findElement(usernameBy).sendKeys(userName);
driver.findElement(passwordBy).sendKeys(password);
driver.findElement(signinBy).click();
return new HomePage(driver);
}/** * Tests login feature */ public class TestLogin {
@Test
public void testLogin() {
SignInPage signInPage = new SignInPage(driver);
HomePage homePage = signInPage.loginValidUser("userName", "password");
assertThat(homePage.getMessageText(), is("Hello userName"));
}
}All of the Java codebases I've worked with, that have 20 levels of hierarchy to 'clean up' the code bases and make it easier to edit, turn into giant balls of spaghetti at some point.
Abstraction can be very useful, but it can also definitely lead to spaghetti if it gets out of hand or is done incorrectly.
1. undescriptive variable names (eg. data2)
2. inconsistent formatting
3. copy pasted fragments everywhere
- Complicated flakey in-line xpaths '//*[@id="content"]/div/div[2]/div/div[1]/div[1]/div/div/button' ?!
- Time.sleep(x) is flakey, you should wait for the element to appear
- case first_name | last_name both uses a first_name() generator, should just be generate_name()
- Opening and closing the driver 10000 times
Can you be more clear on this? I don't see where this is happening. There's a start_drive function that they call once in main and use that reference the entire time.
https://github.com/SeanDaBlack/KelloggBot/blob/main/req.py#L...
- Complicated flakey in-line xpaths '//*[@id="content"]/div/div[2]/div/div[1]/div[1]/div/div/button'
How else would you get around navigating the DOM which this has to match precisely?
- case first_name | last_name both uses a first_name() generator, should just be generate_name()
Very subjective?
- Time.sleep(x) is flakey, you should wait for the element to appear.
Agree with this but maybe it's not possible for some reason in regards to how the webpage is configured?
https://github.com/SeanDaBlack/KelloggBot/blob/main/req.py#L...
They run start_driver() inside a while loop with 10000 iterations
> How else would you get around navigating the DOM which this has to match precisely?
For that specific one I'd probably try "//*[contains(text(), 'Apply now')]", if they add a single element to that page the entire xpath will fail in the code example
> Very subjective?
Do you have two first names?
> Agree with this but maybe it's not possible for some reason in regards to how the webpage is configured?
Maybe! Still, I bet you it's possible to wait for an element even if it's your own action taking effect.
I mean wouldn't that allow 10,000 parallel operations? If you waited for each one to complete, then close each one, it would take forever.
EDIT: Gruez(below) made the point that the code isn't written to be async so this is definite issue.
> For that specific one I'd probably try "//*[contains(text(), 'Apply now')]", if they add a single element to that page the entire xpath will fail in the code example
You're prob right about this to be honest.
However, there's two elements with 'Apply now' on the page. A button with 'Apply now' and a dropdown with 'Apply now' that opens when you click that button. Lol
https://jobs.kellogg.com/job/Lancaster-Permanent-Production-...
Welcome to scraping hell!
> Do you have two first names?
It's generating fake names. It doesn't matter.
You can chain them! It's still better to [0] the text one and then click the pop-up than use div chains the whole way down. Yes it's not perfect, if I was testing the site as part of Kellogg I'd ask for test ids to make them unique, but it's still immune to additional divs.
> It's generating fake names. It doesn't matter.
This hurts my soul
>https://github.com/SeanDaBlack/KelloggBot/blob/main/req.py#L...
the call to "start_driver" is inside the "while (i < 10000)" loop
> 1. undescriptive variable names (eg. data2)
Doesn't matter. The file is 200 lines of code.
> 2. inconsistent formatting
That didn't make the code harder to read/follow.
> 3. copy pasted fragments everywhere
Meh. Really, I've seen code where everything that could be repeated was encapsulated in a function/method/class which was being used only once.
Right, I can follow the code just fine too, but OP's claim was
>well written, rather pretty code
Like you mentioned, the file is only 200 lines, so you can get away with quite a bit and still be understandable/maintainable, but that doesn't mean they're all "well written" or "rather pretty".
- Have you seen Carmack or Torvalds code?
> inconsistent formatting
- Needs more explanation? It's python, consistent formatting is enforced
> copy pasted fragments everywhere
- Nothing wrong with that AT ALL if you understand how it works. It's literally the fundamental premise of Github Co-pilot.
Which means it’s probably at least 80th percentile nowadays.
As far as developer quality goes...I'm afraid I'm not qualified to comment.