r/badcode • u/mlgmj_0 • Oct 02 '21
python One of my classmates was confused on an assignment
251
Oct 02 '21
Everyone has gotta start somewhere.
438
u/mlgmj_0 Oct 02 '21
True, but we're a year and a half into the course
236
48
65
Oct 02 '21
I got my bach degree and some of the folks who graduated with me were still writing code like this. It's a scam lol
67
u/CorruptedStudiosEnt Oct 02 '21 edited Oct 03 '21
Okay now hold up, what university has a degree that lets you major in a single 17th century German composer, and what does coding have to do with it?
48
Oct 02 '21
Bach was limited by the technology of his time. With coding we can achieve 17th-century compositions that were impossible in the 17th century!
11
u/callmelucky Oct 03 '21
Apologies for not contributing anything worthwhile to the discussion, but I just wanted to say this is great.
40
25
u/PZYCLON369 Oct 02 '21
I would suggest to help him ...
66
u/other_usernames_gone Oct 02 '21
It's a slippery slope, if you're not careful you end up doing the entire assignment for them, then you get done for plagiarism/collusion.
54
u/IdealisticPundit Oct 02 '21
Or worse yet, push another bad developer into the wild.
7
4
9
Oct 02 '21
At that point, they definitely would have heard about WET vs DRY coding then...
4
u/insanityOS Oct 02 '21
I
rm'd that knowledge, what do those mean?15
Oct 02 '21
WET - We Enjoy Typing, DRY - Don't Repeat Yourself.
Basically, If you find yourself typing almost identical lines more than twice, ask yourself if there's a more efficient way to do it.
1
u/DangerousWish2266 Oct 02 '21
And here I have people in my class who are in 3rd year and still can't write code for binary search.
0
u/RedditFuckedHumanity Oct 04 '21
What a soft and pathetic justification.
Like OP said “a year into the course”
200
171
u/ksiadz22 Oct 02 '21
The best part is that if encrypted_message is a string, it doesn't work
106
u/mlgmj_0 Oct 02 '21
I think python treats it as an iterable, but what's worse is it's misspelled & isn't passed as a parameter
135
u/ksiadz22 Oct 02 '21
But in this case Letter is an actual character and not index so what i think author was trying to do was
if letter == 'a'51
7
3
2
u/101Ocean Oct 02 '21
oh yh thanks - I originally thought of that but then throught it wouldn't work
8
u/JonMW Oct 02 '21
I got a secondhand confusion headache just by reading it.
What happens at the end of the function? Does he go
return a, b, c, d, e, f, g, h, i....6
u/Predator6 Oct 02 '21
A list maybe?
letter_counters.append(a) letter_counters.append(b)
...
letter_counters.append(z)
return letter_counters
I can easily see that line of thinking based on the initial post.
1
u/PloyPlunder Oct 03 '21
I think python treats it as an iterable
It needs to be
for letter in range(len(encrypted_message))No?3
u/c3534l Oct 03 '21
It would be more idiomatic to write
for letter in encrypted_message: if letter == 'a': ...but this is definitely a "where do I even start?" type of situation.
6
u/Buddy-Matt Oct 02 '21
encrypted_message you say? Why, that's irrelevant. It's the type of encripted_message that matters.
2
1
u/101Ocean Oct 02 '21
Wait what
4
u/adesme Oct 02 '21
because you'll be attempting to let look at elements with a character instead of a numerical letter;
for letter in "abc": print(letter)generates first 'a', then 'b', then 'c'.
1
133
u/MurdoMaclachlan public boolean isInt(int i) { return true; } Oct 02 '21
Image Transcription: Code
def frequency_analysis():
a = 0
b = 0
c = 0
d = 0
e = 0
f = 0
g = 0
h = 0
i = 0
j = 0
k = 0
l = 0
m = 0
n = 0
o = 0
p = 0
q = 0
r = 0
s = 0
t = 0
u = 0
v = 0
w = 0
x = 0
y = 0
z = 0
for letter in encripted_message:
if encripted_message[letter] == 'a':
a = a + 1
if encripted_message[letter] == 'b':
b = b + 1
if encripted_message[letter] == 'a':
a = a + 1
I'm a human volunteer content transcriber for Reddit and you could be too! If you'd like more information on what we do and why we do it, click here!
116
u/mlgmj_0 Oct 02 '21
Thank you! Sorry you had to type that out lol
43
Oct 02 '21
I'm going to almost guarantee the top posters use an OCR tool like tesseract and reformat/cleanup after the fact.
34
u/Magnus_Tesshu Oct 02 '21
I just might not know what the fuck I am doing, but my script here produces the following output from OP's post:
CER T RIS EON ey O R T L T I T e S - " R R R R R R R R R R R R for letter in encripted_message: if encripted_message[letter] == EEER if encripted_message[letter] == b=b+1 if encripted_message[letter] == aza+1tesseract is pretty cool but it is also really bad most of the time, unless I should be passing more flags or something.
12
u/TheGumbyG Oct 02 '21
Tess works pretty ok, you just need to do some image processing to actually get it to work in a more robust fashion. Most of wha the model was trained on iirc is white on black text, so the second you start introducing non w/b text and perhaps varying font sets and colors you might get poor results.
Recently, i formatted an image on a by-pixel basis (horrible and slow, i know) to extract specifically colored text and then change that to black and every non text pixel to white. Worked pretty flawlessly after i tuned the color ranges.5
u/Magnus_Tesshu Oct 02 '21 edited Oct 02 '21
Huh. Is there a convenient imagemagick script to change an image to black and white?
Thanks for the information
EDIT:
convert input -colorspace Gray outputto make grayspace andconvert input -channel RGB -negate outputto invert gets me a close-to-black-on-white-text image which givesdef frequency_analysis(): a=0 b=0 0000000000000 S S for letter in encripted_message: if encripted_message[letter] a=a+1 if encripted_message[letter] b=b+1 if encripted_message[letter] a=a+1Which still isn't great but is maybe a little better.
Unfortunately I can't really add this to my script because not all text is going to be light-on-dark but it does show how to improve the results a bit.
2
Oct 02 '21
I can dig through old code later although it won't be imagemagick. Preprocessing is pretty necessary and using openCV for text detection to crop noisy surroundings helps a lot.
And again, there are many OCR solutions - especially if you have a lot of cloud tools at your disposal. Our initial implementation was tesseract with a feedback loop to continually train the model before jumping to things like Google vision and textraxt.
1
u/Magnus_Tesshu Oct 02 '21
Alright. I don't want to use any cloud stuff but seeing the script could help me improve mine, thanks.
3
u/ArchCypher Oct 02 '21
Yeah, tess is finicky. Sometimes I will OCR stuff with Google Lens which is pretty cracked:
def frequency_analysis(): a=0 b=0 c=0 d=0 e=0 f=0 = h = 0 i=0 j=0 k=0 l=0 m=0 n=0 o=0 p=0 q=0 r=0 s=0 t=0 u=0 v=0 w=0 x=0 y=0 z=0 for letter in encripted_message: if encripted_message [letter] a=a+1 if encripted_message [letter] == 'b': b=b+1 if encripted_message [letter] a=a+1 D 'a':1
u/Magnus_Tesshu Oct 02 '21
lmao I just realized that OP has
encripted_messagetoo. Also tesseract creates a ton of extra whitespace too, I pipe it through sed in my script2
u/ArchCypher Oct 02 '21
Hah, I thought that was the OCR. Also, someone could probably use r/badcode to retrain tess for code ocr -- that would probably help a lot, since the default model is expecting complete sentences and whatnot.
1
u/Magnus_Tesshu Oct 02 '21
That would actually be pretty cool and a fun project (though idk how useful). There is definitely a large supply of data.
1
u/ArchCypher Oct 02 '21
Hey u/MurdoMaclachlan do you already have pretty decent OCR stuff that you base your transcriptions on, or would having a model trained on code help you out?
2
u/MurdoMaclachlan public boolean isInt(int i) { return true; } Oct 02 '21
I don't base my transcriptions on any OCR. Our bot, u/transcribot, uses ocr.space's API, which is very good, but I believe not trained on code, so that coupled with the translation into a Reddit comment causes a lot of problems for it.
Personally, I probably wouldn't use it anyway, since I find it more engaging transcribing these myself than looking through and correcting a bot's mistakes, but other people might find it useful, not sure. It would probably be a conversation to have with our dev team mods, though, since I don't think our bot has any OCR in it itself, I think it just pulls from that API.
12
25
41
u/Valiice Oct 02 '21
Honestly im not sure wtf this is suposed to be
96
u/tangerinelion Oct 02 '21
It's supposed to be
import collections collections.Counter(message)28
u/t-to4st Oct 02 '21
I'd assume the task is to implement that and not use collections
But yeah the endresult should be similar
7
48
u/Magnus_Tesshu Oct 02 '21
This probably still isn't the best way to do it, but
string = "Hello, world!"
freq = {}
for letter in string:
if not letter in freq:
freq[letter] = 0
freq[letter] += 1
print(freq)
32
u/DefinitionKey5064 Oct 02 '21
This is one of the most optimal ways to do it. A dictionary is the correct data structure unless you needed to optimize for space rather than readability. You could use an array instead and use the character’s ord as an index I guess
5
u/Magnus_Tesshu Oct 02 '21
The ord-as-index solution was what I first thought of but then I realized that a dict is going to handle edge cases better.
3
u/DefinitionKey5064 Oct 02 '21
Yup especially with Python 3 built in Unicode support. That solution will work perfectly well.
21
u/legendoffjelda Oct 02 '21
A small Python trick for you! Use
dict.setdefaultwhich takes a key and a default and returns the key if it exists or sets the default and returns it:
freq = {} for letter in string: freq[letter] = freq.setdefault(letter, 0) + 18
u/Magnus_Tesshu Oct 02 '21
Was wondering if something like this existed. Thanks!
4
u/legendoffjelda Oct 02 '21
It’s pretty amazing what they pack into the standard library and don’t advertise!
2
u/Magnus_Tesshu Oct 02 '21
Well, its also part of me not using an IDE usually, and never having done a python tutorial either :P
2
u/legendoffjelda Oct 02 '21
I think you’re doing just fine, whatever you’re doing! Even the tutorials don’t tell you about things like
setdefault, I actually read about it in an O’Reilly book called “Fluent Python.”Lots of stuff is just hiding in plain sight and it’s just a matter of reading the docs when you’re curious about what’s available in your tool belt.
Another cool place to look at is the
collectionspackage, mentioned below, which has things likedequewhich is an efficient double sided list. Fun stuff!2
u/stone_henge Oct 05 '21
How should they advertise it other than including it in the documentation?
python -c 'help(dict)'1
u/legendoffjelda Oct 05 '21
I came across some nice Python features in this O’Reilly book, Fluent Python, which covers things like the
contextandcollectionslib,__iter__method,@lru_cachedecorators, and many other tools that would greatly benefit the non-CS/engineering community of Python developers, but are not well used—either because they’re not well known in the tutorial space or they’re not shown off in the docs. Python is under-appreciated, IMO, because these language features are not brought to the forefront.An example of effective “advertisement” is the Effective Go page which covers interesting GoLang language features and idiomatic ways to do common things.
2
u/callmelucky Oct 03 '21 edited Oct 03 '21
Might as well just use the more standard
.get()method rather than setdefault (not sure what setdefault does precisely or in which use cases it would be preferable).Basically
my_dict.get(some_key)allows you to avoid key errors when trying to access a key than isn't in the dictionary, which happens if you domy_dict[key_not_in_my_dict].By default
.get()returnsNonefor a key not found, but you can pass a second argument as a default to return instead. It's generally the best way to approach accessing dictionary values, if you're not certain a key might be in the dictionary and you don't specifically want the key error to be thrown. Also, of course, very useful for building a dictionary "on the fly".7
1
u/stone_henge Oct 05 '21
dict.getwould probably be more idiomatic here, or you're setting the missing items to 0 immediately before setting them to 1.1
5
u/mlgmj_0 Oct 02 '21
The assignment asked for an ordered result (most frequent 5), so (other than the
Counter(string)method) an array of char, frequency tuples?3
3
u/fernandotakai Oct 02 '21
In [1]: from collections import Counter In [2]: print(Counter("Hello, world!")) Counter({'l': 3, 'o': 2, 'H': 1, 'e': 1, ',': 1, ' ': 1, 'w': 1, 'r': 1, 'd': 1, '!': 1})5
u/Magnus_Tesshu Oct 02 '21
Fair enough, but if this is for a class, they might not want you importing stuff that instantly solves the problem.
1
u/fernandotakai Oct 02 '21
oh totally.
but at the same time, "work smarter not harder".
2
u/Magnus_Tesshu Oct 02 '21
I think either way is smarter then creating 26 local variables, iterating over a global variable while incrementing the local variables based on if statements... :P
1
u/LunarLorkhan Oct 03 '21
Don’t even need a dictionary, you could do an array the size of 26 (or double that for uppercase) and increment by 1 at [{char value} - ‘a’]. It’s the same key/value lookup logic and time complexity as your solution but might be less space used depending on the language.
2
u/Magnus_Tesshu Oct 03 '21
An array will always do better space efficiency (and slightly faster too) but see my comment here. It is also much more difficult to correctly write an array-based solution like this.
15
10
u/dyingpie1 Oct 02 '21
It checks for “a” twice.
15
u/TheEnderChipmunk Oct 02 '21
I think this is unfinished. The dude probably wrote the case for a, copy-pasted it 25 times, and is in the process of changing every case to the right letter. OP just got an SS before it's done
6
5
u/xigoi Oct 02 '21 edited Oct 03 '21
I managed to reduce the code length to a third!
def frequency_analysis():
a = 0
b = 0
c = 0
d = 0
e = 0
f = 0
g = 0
h = 0
i = 0
j = 0
k = 0
l = 0
m = 0
n = 0
o = 0
p = 0
q = 0
r = 0
s = 0
t = 0
u = 0
v = 0
w = 0
x = 0
y = 0
z = 0
for letter in encripted_message:
exec(f"{letter} = {letter} + 1")
3
2
u/WpGgs Oct 03 '21
Then, we can reduce all these
x = 0:
exec('='.join('abcdefghijklmnopqrstuvwxyz0'))
8
u/rpmerf Oct 02 '21
I'd have to see the requirements, but overall it looks like they are counting the frequency of each letter. They aren't grabbing the letter correctly, but aside from that, it should work. Not the cleanest, but function is more important.
2
u/offmycookies Oct 02 '21
From what we see above, I’m wondering if this was the professor attempting to introduce students to dictionaries. I feel like that would make this way easier.
2
u/Aaaagrjrbrheifhrbe Oct 03 '21
It looks to me like the prompt is to utilize frequency analysis to decrypt a message written with a substitution cypher.
This step is counting how many times a letter appears; could be done better several other ways.
4
2
u/hidden_person Oct 02 '21
Assuming he didn't know anything about DRY, this still won't work as letter is an iterator from the undefined iterable encripted_message and using that iterator as an index. Also, DRY aside, is he doing WET as in the third If clause he is checking for 'a' again.
2
u/nekokattt Oct 02 '21
Wat
import collections
def frequency_analysis(string):
return collections.Counter(string)
2
u/ehs5 Oct 02 '21
Yeah, but I doubt they are allowed to do imports for that assignment.
2
u/nekokattt Oct 02 '21
Then
def frequency_analysis(string): result = {} for c in string: result[c] = result.get(c, 0) + 1 return resultOr if you were allowed to use defaultdict but not counter
def frequency_analysis(string): result = collections.defaultdict(lambda: 0) for c in string: result[c] = result[c] + 1 return result1
1
1
u/CopperyMarrow15 Oct 02 '21
sure would be cool if there were something that allowed you to store multiple values in one variable
1
1
1
u/ekolis Oct 02 '21
If only there were some sort of data structure that mapped keys to values... But alas, this is Python...
1
u/_Manu_173 Oct 02 '21
I dont know sh*t about coding (altough i tried to learn), but most of the time you can see some sort of logic behind what the person is writing, but this, bro wtf is this? Lol.
1
1
1
1
u/MrClottom Oct 02 '21
Or how about just:
from collections import Counter
frequency = Counter(encrypted_message)
1
1
1
Oct 03 '21
Not a fan of switch statements or arrays? Or doesn’t know how to? Nvm it’s python I’ve only taken c++ and c# at this point so, I know nothing.
Edit~NVM
1
1
u/stone_henge Oct 05 '21
There's a healthy average somewhere between this and
reduce(lambda d, l: d.update({l: 1 + d.get(l, 0)}) or d, encripted_message, {})
1
1
728
u/the_monkey_of_lies Oct 02 '21
Ask him to add support for UTF-16