19
Awlex
7y

Toughest about this function was (obviously) naming.

Suggestions are (highly) appreciated

Comments
  • 7
    A get function that does that many things?
  • 10
    Seems like it should be at least 2 functions.
  • 17
    Split into:
    getSong()
    getBackgroundPath()
    deleteOldFiles()
  • 3
    Had to couple it like this, because otherwise, it would not be that performant.

    I think I have to rethink some aspects anyway. I just realized, I messed up
  • 3
    Why not just adding a comment block above function name, that lists what it does, and just use simple name?
  • 1
    Name works. 😆👌🏼
  • 1
    // getRefreshedPath gets required paths and clears all other files.
    fun getRefreshedPaths() {
    getSongPath();
    getBackgroundPath();
    cleanUp();
    }
  • 1
    Depends on how big your system is? In future if you can have more things to clean up, e.g song lyricist, composer, lyricist info path and related paths with lyrics and some other paths, you will know where to go. Keeping everything in main may be a good idea if you don't have to maintain/modify the code much.
  • 1
    Definitely needs a refactor! 😉
  • 2
    If it is performant to do everything in one step, how about a holder class with an initialisation methode that is gathering all the things needed and two other methods to get them in the format you want to (deleting other files is not needed because it is done in initialisation).
    On this way you even could add new things and types for file handling very easily in the future.
  • 2
    This clearly violates SRP. Like others said split it up in more functions. It'll be easier to name those functions and I'll bet you can make them perform good too if you take some time for proper design.
  • 2
    Just do as my colleagues do.

    Name it Gablork();

    And make sure you delete all comments.
  • 0
    how about theDescriptiveNameWasTooLongSoPleaseCheckTheCodeToUnderstandWhatTheFunctionDoes()?
  • 1
    After getting so much constructive criticism I ended up with this:

    getSong(file, [fileText])
    getBackground(file, [fileText])
    deleteFiles(folder, preserveList)
    getPairedSongsAndBackgrounds(folder)

    The last function exists, because one song and one background path are written inside the same (immutable) file and should be grouped.
    Also the same song can have multiple backgrounds, so I needed a function that returned a 1:n mapping between them.

    getSong and getBackground get the file as parameter because the paths are written relatively to the file. The optional parameter fileText exists to avoid reading the file multiple times.

    Thank you!
  • 0
    Btw what language is this?
  • 0
Add Comment