Add Python Implementation of Huffman Encoding #98
Conversation
june128
added
the
Implementation
There was a problem hiding this comment.
Hey @foldsters! Thank you for contributing!
The code works fine in Python 2 and 3, but there are a few things about it that I think should be changed. Most of them are related to code style.
The code also doesn't show up in the chapter automatically. It has to be imported manually. You can either leave that out and let someone else do it for you, or you can go into "huffman.md", scroll to the bottom and add this before the {% endmethod %} line:
{% sample lang="py" %}
### Python
[import, lang:"python"](code/python/huffman.py)
| trees.append((new_tree,new_weight)) | ||
|
|
||
| # sort the trees list by weight | ||
| trees = sorted(trees, key=lambda n: n[1], reverse=True) |
There was a problem hiding this comment.
You don't have to sort the entire trees list after each iteration. I know that it will always be pretty small, but I think it would be nicer here to find the right place in the list and use list.insert() instead of list.append() and list.sort() here.
# Find the first tree that has a weight smaller than new_weight and returns its index in the list
# If no such tree can be found, use len(trees) instead to append
index = next((i for i, tree in enumerate(trees) if tree[1] < new_weight), len(trees))
# Insert the new tree there
trees.insert(index, (new_tree, new_weight))There was a problem hiding this comment.
I was thinking of doing an insert, but I thought it would be a little harder to explain and detract from the point of the code. I'll replace it with your code (thanks!) and do the more efficient option from now on.
| # encodes the message | ||
| def encode(mapping,message): | ||
|
|
||
| encoding = "" |
There was a problem hiding this comment.
You use double quotes here and in a few other places as well, while you used single quotes in others. You should stick to one or the other and since single quotes are more common in Python and because other code examples in the AAA already use them, I recommend you change all your double quotes to single quotes.
There was a problem hiding this comment.
Whoops, I didn't even notice that I did that! Fixing that up now.
| return tree | ||
|
|
||
| # constructs the mapping with recursion | ||
| def build_mapping(tree,code=''): |
There was a problem hiding this comment.
You seem to not like spaces between comma-separated identifiers. To stay consistent with other code examples and code outside the AAA you should probably put spaces between function parameters, list items, etc.
There was a problem hiding this comment.
This one is personal preference, because I usually use white space to indicate order of operations, so ((v,k) for k,v in mapping) tells me that k and v are on the same step, while (((v, k) for k, v in mapping) looks like (v, k) for k, is one step and v in mapping is another, but I see where you're coming from and I'll use sentence syntax from now on.
There was a problem hiding this comment.
I forgot to mention that this goes for pretty much all operators, too (a + b instead of a+b).
I can see how it makes sense in your example and maybe it's okay to omit the space in some cases if it really improves readability. But we generally like spaces here. :D
There was a problem hiding this comment.
Sure, I've never programmed in an environment that other people needed to look at my code, so I appreciate the pointers!
There was a problem hiding this comment.
Very good. Code review is weird because I'm always afraid of sounding like "YOU'RE DOING IT WRONG! YOU SHOULD DO IT LIKE ME AND YOUR CODE IS BAD!" but we seem to be on the same page!
| # encodes the message | ||
| def encode(mapping,message): | ||
|
|
||
| encoding = "" |
There was a problem hiding this comment.
The variable name of this confused me for a second. Maybe code or encoded are better names here?
There was a problem hiding this comment.
True, I'll make the variables more descriptive
| # constructs the tree | ||
| def build_tree(message): | ||
|
|
||
| # get sorted list of character,frequency pairs |
There was a problem hiding this comment.
Sorry, I'm super nitpicky here because this is a comment, but commas are usually followed by spaces.
I mentioned the spaces after commas in another comment already. Oops!
|
Okay so I've made the edits, and I think it has updated. Still finding my way around github. |
|
Looks pretty good. I'd merge it, but I can't. Guess there's still something to figure out with the permissions. |
No description provided.