6 ms·

It's actually well written, rather pretty code. https://github.com/SeanDaBlack/KelloggBot/blob/main/req.py https://github.com/SeanDaBlack/KelloggBot/blob/main/

It's actually well written, rather pretty code. https://github.com/SeanDaBlack/KelloggBot/blob/main/req.py 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....

gruez 5y ago
It is? It looks pretty middle of the road to me. obvious issues: 1. undescriptive variable names (eg. data2) 2. inconsistent formatting 3. copy pasted fragments everywhere
- edmcnulty101 5y ago
  > undescriptive variable names - 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.
- howdydoo 5y ago
  I mean sure, it's not perfect. It might not get you through a FAANG interview, but it would get you through a Kellogg interview.
  - Kye 5y ago
    I wouldn't survive the interview because I would have to answer "Why do you want to work here?" with "To fund adult art commissions of Tony the Tiger."
    - varelse 5y ago
      I think a more commercially successful venture would be an adult cinematic franchise set in the Dark Frankenberry/Count Chocula universe as they save breakfast (and morning sex) from nefarious threats like the Frosted Lucky Charms Elf and the evil Quisp.
      - edmcnulty101 5y ago
        The M. Night Shamalayan twist....the Lucky Charms Elf ends up with the girl.
  - exdsq 5y ago
    I don't think it would... A one minute review: - 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
    - pmcp 5y ago
      Flakey code might be an advantage in this specific case.
    - edmcnulty101 5y ago
      - 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#L54 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?
      - gruez 5y ago
        >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 https://github.com/SeanDaBlack/KelloggBot/blob/main/req.py#L... the call to "start_driver" is inside the "while (i < 10000)" loop
        edmcnulty101 5y ago
        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.
  - bick_nyers 5y ago
    As someone who worked IT at Kellogg's, I would say the interview was more about confidence and communication skills then it was technical ability. The only "programming" I ever did was VBA/Excel. Although I did dabble with PLC from time to time. I'm not sure if they even have actual software engineers at their company, they seem to like contracting out a lot of that kind of stuff.
  - XOPJ 5y ago
    This joke went over so many heads, it's brilliant.
- sombremesa 5y ago
  It is middle of the road. Which means it’s probably at least 80th percentile nowadays.
  - gruez 5y ago
    Due to selection effects, I wouldn't find it too surprising that the "middle of the road" (ie. 50th percentile) of developers with a job is equal to the 80th percentile of developers seeking a job.
    - sombremesa 5y ago
      Well, the reason I was able to apply both "middle of the road" and "80th percentile" labels to the same code is because the former was regarding code quality and the latter was regarding code prevalence (in the same sense that scoring 50/100 on an exam doesn't necessarily put you in the 50th percentile). As far as developer quality goes...I'm afraid I'm not qualified to comment.
- sdevonoes 5y ago
  I have no experience at all using Selenium (nor automated scripts that POST stuff in the web) but I could understand what the code was doing. I can follow the code just fine. > 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.
  - gruez 5y ago
    >I can follow the code just fine. 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".
exdsq 5y ago
People tend to not be aware of software testing patterns but the Page Object Model would help clean this code up a lot, especially around the hard element ids that could change at any time. https://www.selenium.dev/documentation/test_practices/encouraged/page_object_models/ https://www.selenium.dev/documentation/test_practices/encour...
- edmcnulty101 5y ago
  You're advocating to turn this: 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")); } }
  - exdsq 5y ago
    There are better ways to write the pattern but the idea is having the elements in one place so if the site changes you can update them quickly, and then in your code accessing them by variable name. It's much cleaner, easier to edit if they change the UI (which happens) or if they use a web framework like React that has strange elements to access.

gruez 5y ago

It is? It looks pretty middle of the road to me. obvious issues: 1. undescriptive variable names (eg. data2) 2. inconsistent formatting 3. copy pasted fragments everywhere

edmcnulty101 5y ago
> undescriptive variable names - 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.
howdydoo 5y ago
I mean sure, it's not perfect. It might not get you through a FAANG interview, but it would get you through a Kellogg interview.
- Kye 5y ago
  I wouldn't survive the interview because I would have to answer "Why do you want to work here?" with "To fund adult art commissions of Tony the Tiger."
  - varelse 5y ago
    I think a more commercially successful venture would be an adult cinematic franchise set in the Dark Frankenberry/Count Chocula universe as they save breakfast (and morning sex) from nefarious threats like the Frosted Lucky Charms Elf and the evil Quisp.
    - edmcnulty101 5y ago
      The M. Night Shamalayan twist....the Lucky Charms Elf ends up with the girl.
- exdsq 5y ago
  I don't think it would... A one minute review: - 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
  - pmcp 5y ago
    Flakey code might be an advantage in this specific case.
  - edmcnulty101 5y ago
    - 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#L54 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?
    - gruez 5y ago
      >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 https://github.com/SeanDaBlack/KelloggBot/blob/main/req.py#L... the call to "start_driver" is inside the "while (i < 10000)" loop
      - edmcnulty101 5y ago
        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.
- bick_nyers 5y ago
  As someone who worked IT at Kellogg's, I would say the interview was more about confidence and communication skills then it was technical ability. The only "programming" I ever did was VBA/Excel. Although I did dabble with PLC from time to time. I'm not sure if they even have actual software engineers at their company, they seem to like contracting out a lot of that kind of stuff.
- XOPJ 5y ago
  This joke went over so many heads, it's brilliant.
sombremesa 5y ago
It is middle of the road. Which means it’s probably at least 80th percentile nowadays.
- gruez 5y ago
  Due to selection effects, I wouldn't find it too surprising that the "middle of the road" (ie. 50th percentile) of developers with a job is equal to the 80th percentile of developers seeking a job.
  - sombremesa 5y ago
    Well, the reason I was able to apply both "middle of the road" and "80th percentile" labels to the same code is because the former was regarding code quality and the latter was regarding code prevalence (in the same sense that scoring 50/100 on an exam doesn't necessarily put you in the 50th percentile). As far as developer quality goes...I'm afraid I'm not qualified to comment.
sdevonoes 5y ago
I have no experience at all using Selenium (nor automated scripts that POST stuff in the web) but I could understand what the code was doing. I can follow the code just fine. > 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.
- gruez 5y ago
  >I can follow the code just fine. 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".

exdsq 5y ago

People tend to not be aware of software testing patterns but the Page Object Model would help clean this code up a lot, especially around the hard element ids that could change at any time. https://www.selenium.dev/documentation/test_practices/encouraged/page_object_models/ https://www.selenium.dev/documentation/test_practices/encour...

edmcnulty101 5y ago
You're advocating to turn this: 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")); } }
- exdsq 5y ago
  There are better ways to write the pattern but the idea is having the elements in one place so if the site changes you can update them quickly, and then in your code accessing them by variable name. It's much cleaner, easier to edit if they change the UI (which happens) or if they use a web framework like React that has strange elements to access.