Dev: Lots of repeated code
cbortz opened this issue · 4 comments
While working on the creature harvesting feature (thanks for merging it by the way!), I noticed there's a lot of repeated code around the app. I've created this issue as a place to open a discussion about possibly cleaning some of this up.
Take for instance in 73b610d, the following line is repeated 7 times:
$("#typeSel").load(BASE_SCRIPT_URL + "getResourceTypeList.py", {galaxy:$("#galaxySel").val()});@pwillworth What are your thoughts here? Do you think some time should be spent refactoring some of the existing code to not be so repetitive? If so, do you have any preference or methodology for doing so?
There may be some instances of repeated code that could be refactored, but my approach has been to do a little incidental cleanup in whatever area I'm working in when working on new features. Development of this app has been very much a "slow roll", so a lot of the code is very old and not following many practices that might be second nature today.
As to the specific instance you are calling out, maybe there is some practice I'm not aware of or have not been following, but I don't see how you would refactor that in a beneficial way. It is a single line of code that is repeated in separate web pages. When I think of refactoring repetition, examples would be more like you are using the same block of several lines of code across the application in various places and you refactor that into a function that can be shared. Or if you are repeating the same line or literal value in the same file, you might consolidate that into one declaration in the file and refer to it.
@pwillworth Sorry it's taken me so long to respond to this!
There may be some instances of repeated code that could be refactored, but my approach has been to do a little incidental cleanup in whatever area I'm working in when working on new features. Development of this app has been very much a "slow roll", so a lot of the code is very old and not following many practices that might be second nature today.
I totally understand!
As for refactoring approaches, I'm not 100% sure of the best way to approach refactoring at this stage. One idea is to extract code like this into a shared function and replace the existing repeated code with a call to the function. That way, if change ever needs to happen, we can update that single function without the risk of possibly forgetting to update a particular instance of the code.
Cleaning up this one isolated instance may not do much, but in practice, I think it might make things more manageable/approachable, especially for potential new contributors.
Thoughts?
Yeah, sure I can see how that might make it easier to do certain updates. Not sure how it would make it more approachable for new contributors though... it would just make people have to drill down to look into another separate function to see what's going on for what could have been 1 line of code right there. It's borderline I guess, just depends on the person maybe. I'd be fine with it either way.
Closing for now.