Conversation
lbajolet
left a comment
lbajolet
left a comment
There was a problem hiding this comment.
I did not see any code detecting function calls or maybe I have misunderstood some part of the code, could you confirm if this is missing?
Also, please be consistent with indentation, either choose tabs or spaces, but keep to it.
main.go
Outdated
| } | ||
| res := result{} | ||
| for _, f := range files { | ||
| file_path := path.Join(diff_dir_path, f.Name()) |
There was a problem hiding this comment.
Watch out with the path package as it is not intended for file system path manipulation, filepath is more suited for this
main.go
Outdated
| task_list := []string{"DIFF", "REGION", "UPDATE"} | ||
| for scanner.Scan() { | ||
| if scanner.Text() != "" { | ||
| if len(scanner.Text()) > 0 { |
There was a problem hiding this comment.
I speak for myself, and possibly others will agree that this can be &ed for more linear code, this'll help readability
main.go
Outdated
| if len(scanner.Text()) > 0 { | ||
| for _, task := range task_list { | ||
| success, on_success := execute_task(&task, scanner.Text(), res) | ||
| if success && len(on_success) == 0 { |
There was a problem hiding this comment.
I think this can be simplified by the negative; i.e. if !success { continue }
main.go
Outdated
| if success && len(on_success) == 0 { | ||
| continue | ||
| } else if success { | ||
| task_list = on_success |
There was a problem hiding this comment.
I think I understand what you are doing, is it some state machine?
It may require a bit of commenting since it was rather confusing for me at first.
There was a problem hiding this comment.
Yes it is kind of a state machine where I have mainly tree states, the states determine which pattern to search in priority, like a very simple waterfall where we test the most complex and intricated task when no other task succeded, the tasks being finding clue about state transition, states 'being in a diff headline' (DIFF), 'in a region headline' (REGION) 'in diff content' (UPDATE). When we are in state UPDATE, we have to check all states with priority DIFF then REGION then UPDATE and FUNCTION CALL.
main.go
Outdated
|
|
||
| func execute_task(task *string, line string, res *result) (bool, []string) { | ||
| success := false | ||
| on_success := make([]string, 0) |
There was a problem hiding this comment.
That's a small detail, but this should be declared as nil first
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Adding a fallback for missing future cases may be worth it
main.go
Outdated
| func execute_task(task *string, line string, res *result) (bool, []string) { | ||
| success := false | ||
| on_success := make([]string, 0) | ||
| if *task == "DIFF" { |
There was a problem hiding this comment.
If you're going towards a state-machine approach, I'd suggest using enums for that kind of manipulation, it'll be way lighter than strings for states.
There was a problem hiding this comment.
Thank you, I just found out how to write enums.
main.go
Outdated
| // Extract $file_path_a and $file_path_b if line = 'diff --git a/$file_path_a b/$file_path_b' | ||
| func is_new_diff(line *string, res *result) bool { | ||
| new_diff := false | ||
| if line != nil { |
There was a problem hiding this comment.
This is personal preference (and is something advocated by several open-source projects), but I think you should linearize your code for easier understanding of what's happening.
There was a problem hiding this comment.
Alright, I see that I refined a lot of conditions and indeed there refacto to do.
main.go
Outdated
| new_diff := false | ||
| if line != nil { | ||
| if len(*line) > 10 { | ||
| if (*line)[:11] == "diff --git " { |
There was a problem hiding this comment.
Both those lines could be replaced by a strings.HasPrefix
There was a problem hiding this comment.
It increased a bit performances.
I am currently working on a new version where I read the file character by character and try to apply a combination of expectation maximization on Hidden Markov Models to learn the statistics of an HMM which state would be Thank you for this, and thank you Lucas for your comments, I learned from them and am working on it. Best regards |
3 participants
No description provided.