#Roast my beginner code! or well.. maybe just tickle it a little?

1 messages · Page 1 of 1 (latest)

waxen hillBOT
#

@chilly shore

File Attachments Not Allowed

For safety reasons we do not allow files with certain file extensions.

Code Formatting

You can share your code using triple backticks like this:
```
YOUR CODE
```

Large Portions of Code

For longer scripts use Hastebin or GitHub Gists and share the link here

Ignored these files due to them having disallowed file extensions
  • message.txt
old field
#

a little code review

chilly shore
#

one sec

old field
#

How strict and opinionated do you want the review to be?

rugged ermine
#

Go all out lol

old field
#
  • No point in at least half of those comments (they tell me literally nothing)
image.png
#
  • Dataclasses might make sense here, would probably clean the code up quite a bit
#
  • Why do we have a type: ignore in line 40 when we don't even have proper type hints in the code?
  • Add nice type hints
#
  • Why does the function in line 25 return different types (None vs str), looks a bit weird
#
  • No idea why line 62 is indented twice?
#
  • Line 164 and 173 have variables that are returned immediately -> why not return the value directly here?
#
  • Nice use of list comprehensions
#
  • We don't actually need the else in line 154
  • The function in 151 probably doesn't work as expected, if we have items in the library then we just return the info for the first item (-> returning in the first iteration of the loop) instead of listing all items
#

just my first couple of thoughts @rugged ermine feel free to join in on the hating 🗣️

rugged ermine
#

I just call it standards 🤷🏻

chilly shore
#

Thanks alot!! I will review these things, some of these ignores and type-defintitions are because of a whiny Pylamce

old field
#

Just make all PRs touch 2k+ lines and people will automatically just approve

rugged ermine
old field
#

you are out of your mind 😩

chilly shore
#

Why does the function in line 25 return different types (None vs str), looks a bit weird:

I used to have a boolean controlling if item was borrowed or not, but realized if i set _borrowed_by to None when returned it will return False and True if it set to a user

#

mayb bad praxis?

old field
#

No I think the Optional[LibraryUser] is a very nice way to store the borrower info

#

I guess it maybe makes sense if I consider the string an error and just assume nothing happens in the happy case

rugged ermine
#

What version of python are you using?

old field
#

swedes together strong 🗣️

images.png
chilly shore
rugged ermine
#

Where's the swedes coming from lol

chilly shore
#

oh we're all swedes here?

old field
old field
rugged ermine
#

I'm a kiwi lol

chilly shore
#

Well... :$

#

I live in malmö! I have OPINIONS!

#

But i love the bashing of my code! That was great

rugged ermine
#

Imma have a look soon

old field
#

Started to speed up after that and eventually I didn't get too many comments anymore

chilly shore
#

But i've made some corrections, gonna check in to the list_items-function as yes, it does'nt work properly!

Also i'll read up on type hints and why avoid nested ifs :D

old field
#

The video on nesting is pretty nice

rugged ermine
#

Cool, I agree with everything not rob said. But I also have things I'd add

#

My linter hates your code btw

chilly shore
#

but about the str/None-confusion..

I was under the impression that if a string is None it returns false? And returns True if it's declared? In that sense it would work with a check if string is True?

old field
old field
#

We got big screens now let the people have 200 characters in a line

rugged ermine
chilly shore
#

VSC does word wrapping ^_^

old field
rugged ermine
#
  • As you're using python 3.10+, I would not import Optional from typing as that's an extra import and it's less clear than T | None
  • I am a huge believer in type hinting all function arguments and returns
  • You have a lot of trailing whitespace (line 1, 16, 21, ...)
  • I prefer calling datetime functions with a timezone argument so that they are timezone aware objects
  • Line too long (debatable)
  • Line 25's and 151's function should have an explicit return at the end as it's able to return non-none values
  • Line 190 and beyond should be in a if __name__ == "__main__" block
  • I like to organise my imports based on pep8 and in alphabetical order
  • It feels weird for the item ABC to add itself to the it's from
  • Definitely use dataclasses or at least define a __repr__ and __str__
  • I think Item.borrow should raise an exception instead of a string message
  • Item.return_item is kind of doing two jobs, returning the item and calculating fines. Your function should probably not calculate the fine in the return_item method
  • Item.calculate_fine calls datetime.now().date() twice. Instead it should be saved as a variable
  • Why is LibraryUser.library a public attribute but others aren't?

I kinda got bored after reading LibraryUser's __init__. I can have another look if you make changes to what I've mentioned

#

oh what enter doesn't create new line

#

Scam

#

I'll just edit it and ping you when done

chilly shore
#

(look i learned Git 1 week ago!! DONT SCARE ME!)

old field
#

editing messages is eeeeeeasy (see?)

rugged ermine
#

I meant my message

chilly shore
#

oh

#

good ol /s/change/change

#

yay

#

But again thanks alot! Its these interactions that pushes me to learn more :)

rugged ermine
#

not rob, before you say I contradicted myself by not reading it all, I've been out all day and want to play video games

chilly shore
nocturne plume
#

you so brave, showing your code

#

if people saw my code, they would set me on fire, especially when i started coding

#

i had comments like #what am i doing, #idk what this is and stuff

#

i still do that, cause it's silly

chilly shore
#

also im very naive :(

old field
#

@rugged ermine I actually really really disagree on the typing.Optional, I find them much clearer to read than the unions 🤨

nocturne plume
#

smart people are talking

old field
#

Rest sounds like good feedback

rugged ermine
#

I don't like the extra import. Also, some people might interpret it as an optional argument which union None doesn't have

old field
#

You can make it optional by just using = None, guess it depends on the rest of the codebase

I prefer the Optional because it's closer to what I would write in C++ or Rust so that probably is an influence on my preference lol

#

also @chilly shore I see we have another funny server in common, very cool 😎

chilly shore
#

Its nice! a very nice site for trying out pico-builds

rugged ermine
old field
#

Ah fair enough

rugged ermine
#

Contrast that to def foo(x: int | None = 5)

#

Pretty clear that None can be passed in and that it's optional due to a default

old field
#

We gatingkeeping our functions fr

rugged ermine
#

I'm glad we agree on everything else tho haha

#

Would've been awkward if we started fighting or smth

chilly shore
#

also littlebit interesting!

#

😳

old field
#

lines ain't too long your screen is just too short

rugged ermine
#

Preference lol. I like splitting my windows and also I find it easier to scroll up and down compared to scanning left and right

old field
#

just put everything in one line tbf

rugged ermine
#

Pep8 recommends 79 characters which is worse than what I use

rugged ermine
chilly shore
#

thats why we do list comprehension no?

rugged ermine
#

Not necessarily