Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

Please review my Log parser for general problems. I am planning to move all of the log reading code into a class and rearrange things on the OO side to make it easier to understand. I hobbled the wxPython code together so I am not 100% sure it is the best way to handle the log output which is a big worry (so far testing it seems to work just fine). Exception handling is strange as it opens a new window which is very undesirable.

The parser is in one file at: https://github.com/h3lls/FFXIV-Log-Parser/blob/master/logparse.py

I am using cx_freeze without difficulty to provide a Windows executable for end users.

share|improve this question
1  
Welcome to the site. Please post the main portion of the code you want reviewed in the post itself. We are requiring this so that there is a reduced dependence on outside resources. Please consult the FAQ: codereview.stackexchange.com/faq – Mark Loeser Feb 7 '11 at 3:28
@Lennart These are all excellent suggestions. I love the function map for each log item. I will be moving the multi-line strings into documentation for those curious about parsing, had it there for reference as I was coding. I'm still boggled by the hyperlink not being part of wx like the rest of the UI elements but unit tests are TBD. Hobby projects like this always end up without tests because I don't know what I want it to do when I start. – HBR Feb 8 '11 at 10:08
@Mark thanks for the heads up, I will do a better job with the next one. – HBR Feb 8 '11 at 10:12
Actually writing tests often helps you to decide what it should do. So my hobby projects are often more well tested than my professional ones, for that reason. :-) I recommend Kent Becks "Test-Driven Development: By Example". – Lennart Regebro Feb 9 '11 at 9:46

3 Answers

up vote 1 down vote accepted

You do a lot of:

if logitem.startswith("51::"):
    logitem = logitem.strip("51::")

Type of code. You might want to instead do something like this:

code, logitem = logitem.split('::', 1)

To extract the "51" part from the log item. And instead of having a long list of

if code == '51': ... elif code == '22': ... elif code == '12': ...

and so on, you might want to make each code block into separate functions, and do a mapping from the code to the function:

FUNCTIONMAP = {'51': increasehits '22': monsterhit '12': loopdeloop }

In this case you probably also need to gather all the variables you have in one object, like:

exptotal = 0
damagepermob = 0
damageavgpermob = 0
craftingcomplete = 0
share|improve this answer
1  
Thanks for the push in the right direction, the new class is quite a bit better here's a pastebin of the new classes pastebin.com/PrmFAsF1 I will have a class for each language I parse out an inherit the base class that does the mapping. I will need to write a unit test for each language class to verify that it contains the methods for the parser but I think it is much better than previous. – HBR Feb 14 '11 at 7:33
up vote 0 down vote

Multiple answers, as per Should one person have multiple answers?

You use multi-line string constants for comments:

'''0020::You have received the required materials.
0047::Mie Miqolatte creates a trapper's tunic.
001B::Zilbelt Stanford bows courteously to Maisenta.
0020::You use Standard Synthesis. The attempt succeeds!
0020::Progress increases by 10%.
0020::Durability decreases by 4.
0020::Quality increases by 3.
0020::You use Standard Synthesis. The attempt succeeds!
0020::Progress increases by 17%.
0020::Quality increases by 5.
...
'''

This is mainly a style issue, but I think you should use actual comments instead.

share|improve this answer
up vote 0 down vote

Here you import one library from two different places dependning on how it's installed. That's fine, but they end up with two different names:

try:
    from agw import hyperlink
except ImportError: # if it's not there locally, try the wxPython lib.
    import wx.lib.agw.hyperlink as hl

The first line should be from agw import hyperlink as hl. This is clearly nothing you have tests for. In fact, you seem to have no tests at all. Writing tests for different environments can be tricky, I would recommend that you have two Python installs or virtualenvs, one with agw locally, and one not, and run your tests with both. That's not a perfect solution, but it's easy.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.