4
\$\begingroup\$

This is part of a bigger project I'm doing to get better at JavaScript. It's basically the beginnings of a combat log parser for World of Warcraft. It looks a bit messy right now and I could really use some feedback at this point since I'm not really sure if I'm moving in the right direction here. The code doesn't feel very readable either and I'm not entirely sure how I could improve it yet.

jsFiddle

HTML:

<textarea id = "cl_input">
 11/18 05:27:37.903  SWING_DAMAGE,Player-1596-07F94B14,"Blome-TheMaelstrom",0x511,0x0,Creature-0-1596-1116-125-86727-00006A614C,"Thorncoat Talbuk",0x10a48,0x0,0000000000000000,0,0,0,0,0,0,0,0,0.00,0.00,0,1140,-1,1,0,0,0,nil,nil,nil,nil
11/18 05:27:37.903  SPELL_AURA_REMOVED_DOSE,Creature-0-1596-1116-125-86727-00006A614C,"Thorncoat Talbuk",0x10a48,0x0,Creature-0-1596-1116-125-86727-00006A614C,"Thorncoat Talbuk",0x10a48,0x0,158056,"Gift of the Talbuk",0x1,BUFF,2
11/18 05:27:37.903  SPELL_AURA_APPLIED_DOSE,Creature-0-1596-1116-125-86727-00006A614C,"Thorncoat Talbuk",0x10a48,0x0,Creature-0-1596-1116-125-86727-00006A614C,"Thorncoat Talbuk",0x10a48,0x0,158056,"Gift of the Talbuk",0x1,BUFF,3
11/18 05:27:38.359  SWING_DAMAGE,Creature-0-1596-1116-125-86727-00006A614C,"Thorncoat Talbuk",0x10a48,0x0,Player-1596-07F94B14,"Blome-TheMaelstrom",0x511,0x0,0000000000000000,0,0,0,0,0,0,0,0,0.00,0.00,0,6599,-1,1,0,0,0,nil,nil,nil,nil
11/18 05:27:38.874  SWING_DAMAGE,Creature-0-1596-1116-125-86727-00006A615D,"Thorncoat Talbuk",0xa48,0x0,Player-1596-07F94B14,"Blome-TheMaelstrom",0x511,0x0,0000000000000000,0,0,0,0,0,0,0,0,0.00,0.00,0,6421,-1,1,0,0,0,nil,nil,nil,nil
11/18 05:27:40.396  SWING_DAMAGE,Creature-0-1596-1116-125-86727-00006A614C,"Thorncoat Talbuk",0x10a48,0x0,Player-1596-07F94B14,"Blome-TheMaelstrom",0x511,0x0,0000000000000000,0,0,0,0,0,0,0,0,0.00,0.00,0,7891,-1,1,0,0,0,nil,nil,nil,nil
11/18 05:27:40.921  SWING_DAMAGE,Creature-0-1596-1116-125-86727-00006A615D,"Thorncoat Talbuk",0xa48,0x0,Player-1596-07F94B14,"Blome-TheMaelstrom",0x511,0x0,0000000000000000,0,0,0,0,0,0,0,0,0.00,0.00,0,9394,-1,1,0,0,0,nil,nil,nil,nil
11/18 05:27:41.082  SWING_DAMAGE,Player-1596-07F94B14,"Blome-TheMaelstrom",0x511,0x0,Creature-0-1596-1116-125-86727-00006A614C,"Thorncoat Talbuk",0x10a48,0x0,0000000000000000,0,0,0,0,0,0,0,0,0.00,0.00,0,1150,-1,1,0,0,0,nil,nil,nil,nil
11/18 05:27:41.082  SPELL_AURA_REMOVED_DOSE,Creature-0-1596-1116-125-86727-00006A614C,"Thorncoat Talbuk",0x10a48,0x0,Creature-0-1596-1116-125-86727-00006A614C,"Thorncoat Talbuk",0x10a48,0x0,158056,"Gift of the Talbuk",0x1,BUFF,2
11/18 05:27:41.082  SPELL_AURA_APPLIED,Player-1596-07F94B14,"Blome-TheMaelstrom",0x511,0x0,Player-1596-07F94B14,"Blome-TheMaelstrom",0x511,0x0,133630,"Exquisite Proficiency",0x40,BUFF
11/18 05:27:41.082  SPELL_AURA_APPLIED_DOSE,Creature-0-1596-1116-125-86727-00006A614C,"Thorncoat Talbuk",0x10a48,0x0,Creature-0-1596-1116-125-86727-00006A614C,"Thorncoat Talbuk",0x10a48,0x0,158056,"Gift of the Talbuk",0x1,BUFF,3</textarea>

JavaScript:

function HHMMSStoMS(HHMMSS){ // convert "hh:mm:ss" to milliseconds

    var arr = HHMMSS.split(':'); 
                //  HH                MM               SS
    var ms = (arr[0]*3600000) + (arr[1]*60000) + (arr[2]*1000 ) ;

    return ms;
}

parseCL();

/// Simple World Of Warcraft combat log parser

function parseCL() {
    var cl_events = []; 

    var combatlog_raw = document.getElementById('cl_input').value; // get raw combat log data from html textarea
    combatlog = combatlog_raw.split('\n'); // make it an array with 1 element per line.
    var currentLine;


    var logStart = HHMMSStoMS(combatlog[0].slice(6, 14)); // set log start to first lines timestamp, looks really ugly tho.


    for (line in combatlog) {

        currentLine = combatlog[line];
        evt_data = currentLine.slice(20).split(',');

        event_obj = {
            id:            line,
            timestamp:     HHMMSStoMS(currentLine.slice(6, 14)) - logStart,
            type:          type = evt_data[0],
            source:        evt_data[2],
            destination:   evt_data[6],
            amount:        Number(evt_data[21]) || 0,
            data:          evt_data

        };

        cl_events.push(event_obj);


        console.log(line + "--- Timestamp(ms) --- " + event_obj.timestamp + "--- Event Type --- " + event_obj.type + " --- Source: --- " + event_obj.source + " --- Dest.--- " + event_obj.destination + " --- Amount --- " + event_obj.amount);


    }

    console.log("Parsing complete without error!(maybe)");
}
\$\endgroup\$
3
\$\begingroup\$

Interesting question,

I would not over-engineer this code since in essence it is just a log parser. Some tips I have are:

  • Separating parts of the new lines is good practice, but double newlines is overkill
  • Think before creating helper variables, in this case you are using combatlog_raw only once:

    var combatlog_raw = document.getElementById('cl_input').value; // get raw combat log data from html textarea
    combatlog = combatlog_raw.split('\n'); // make it an array with 1 element per line.
    

    you could simply do

    //Split raw combat log from cl_input into separate lines
    combatlog = document.getElementById('cl_input').value.split('\n');
    
  • Comments should not document the obvious, however in your case nothing much might be obvious. Use your judgement.
  • type: type = evt_data[0], <- because of the type= ev_data[0] part you are creating type in the global namespace, I assume this is a simple typo/bug
  • Number(evt_data[21]) || 0 <- I would go for +evt_data[21] || 0, you made me learn that if +evt_data[21] is NaN that the value will evaluate as falsy and then pick up the zero. That is pretty awesome
  • Back to comments; avoid comments to the right of statements if you can put them above the statement, it stops the horizontal stretching which sometimes makes your code too hard to read. Also the very long console.log call should be split over multiple lines of code.
  • It is more idiomatic JavaScript to have 1 var statement with comma separated variables.
  • The only 2 times you call HHMMSStoMS are coupled with a slice command, perhaps you should internalize the slice part inside that function

My counter proposal would be

//Convert "hh:mm:ss" to milliseconds
function extractMilliseconds(line){ 

    var timestamp = line.slice(6, 14),
        arr = timestamp.split(':'),
        ms = (arr[0]*3600000) + //Hours
             (arr[1]*60000) + //Minutes
             (arr[2]*1000 ); //Seconds
    return ms;
}

parseCombatLog();

//World Of Warcraft combat log parser
function parseCombatLog() {
    var events = [],
        //Split raw combat log from cl_input into separate lines
        combatlog = document.getElementById('cl_input').value.split('\n'),
        logStart = extractMilliseconds(combatlog[0]),
        event,
        line,
        data;

    for (line in combatlog) {

        line = combatlog[line];
        //Cut out the date/time part which is 20 chars
        data = line.slice(20).split(',');

        event = {
            id:            line,
            timestamp:     extractMilliseconds(line) - logStart,
            type:          evt_data[0],
            source:        evt_data[2],
            destination:   evt_data[6],
            amount:        +evt_data[21] || 0,
            data:          evt_data
        };
        events.push(event);

        console.log( line + "--- Timestamp(ms) --- " + event.timestamp + 
                            " --- Event Type --- " + event.type + 
                            " --- Source: --- " + event.source + 
                            " --- Dest.--- " + event.destination + 
                            " --- Amount --- " + event.amount );
    }
    console.log("Parsing complete");
}
\$\endgroup\$
  • \$\begingroup\$ Thanks alot for this detailed review! Appriciate it. Great idea putting the .slice in the millisecond funcction aswell :) \$\endgroup\$ – coding Nov 19 '14 at 12:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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