12
\$\begingroup\$

Here is the GitHub repository for the code if you'd like to look at that instead. It's hardly any larger than what's posted here. Most of the code are in these three modules.

Besides just general suggestions, there is a bit duplication in my code, I've tried to minimize it, but I've hit a point where everything gets scrambled and incomprehensible when I try to reduce it further. Thoughts on this would be great!

mutil.py

I suspect high coupling between the calls to add_argument and the commands module. Is this alright?

import argparse

from mutil import commands


def main():
    parser = init_parser()
    parser.parse_args()


def init_parser():
    """Initiate argparse and return an `argparse.ArgumentParser`
    Returns: An `ArgumentParser`
    """
    parser = argparse.ArgumentParser()

    parser.add_argument(
      '--remove-duplicates', '-d',
      action=commands.RemovePlaylistDuplicatesAction)

    parser.add_argument(
      '--use-absolute-paths', '-A',
      nargs=2,
      action=commands.PlaylistPathsUseAbsolute)

    parser.add_argument(
      '--use-relative-paths', '-R',
      nargs=2,
      action=commands.PlaylistPathsUseRelative)

    return parser


if __name__ == '__main__':
    main()

commands.py

Custom Action classes are required by the argparse library to call your own custom functions. I think this approach not only looks clunky, but will also quickly end up with dozens of classes, one for each possible command.

The *Action classes take values handed to us by argparse and feeds them into the class' corresponding function.

import argparse

from mutil import util


class RemovePlaylistDuplicatesAction(argparse.Action):
    def __call__(self, parser, namespace, values, option_string=None):
        # 'values' is the command-line argument picked up by argparse
        with open(values) as playlist:
            return remove_playlist_duplicates(playlist)


class PlaylistPathsUseRelative(argparse.Action):
    def __call__(self, parser, namespace, values, option_string=None):
        playlist_path = values[0]
        library_path = values[1]
        with open(playlist_path) as playlist:
            return playlist_paths_use_relative(playlist, library_path)


class PlaylistPathsUseAbsolute(argparse.Action):
    def __call__(self, parser, namespace, values, option_string=None):
        playlist_path = values[0]
        library_path = values[1]
        with open(playlist_path) as playlist:
            return playlist_paths_use_absolute(playlist, library_path)


def remove_playlist_duplicates(playlist):
    """Remove entries in a given playlist.
    Args:
        playlist: A file containing a playlist to check.

    Returns: The number of duplicates removed.
    """
    playlist_entries = []
    duplicates = 0
    for entry in playlist:
        entry = entry.strip('\n')
        if entry in playlist_entries:
            duplicates += 1
        else:
            playlist_entries.append(entry)

    new_playlist = '\n'.join(playlist_entries)

    # Overwrite the old playlist
    util.overwrite_and_reset(playlist, new_playlist)

    return duplicates


def playlist_paths_use_relative(playlist, library_path):
    """Modify the playlist format to use relative paths.
    Args:
        playlist: A file-object containing a playlist to check.
        library_path: The path in which your library resides,
                      must end in a '/' and cannot contain a newline
    """
    library_path = library_path.strip()
    playlist_entries = []
    for entry in playlist:
        entry = entry.strip('\n')

        # Change the format, relative
        entry = entry.strip(library_path)

        playlist_entries.append(entry)

    new_playlist = '\n'.join(playlist_entries)

    # Overwrite the old playlist
    util.overwrite_and_reset(playlist, new_playlist)


def playlist_paths_use_absolute(playlist, library_path):
    """Modify the playlist format to use absolute paths.
    Args:
        playlist: A file-object containing a playlist to check.
        library_path: The path in which your library resides,
                      must end in a '/' and cannot contain a newline
    """
    library_path = library_path.strip()
    playlist_entries = []
    for entry in playlist:
        entry = entry.strip('\n')

        # Change the format, absolute
        entry = library_path + entry

        playlist_entries.append(entry)

    new_playlist = '\n'.join(playlist_entries)

    # Overwrite the old playlist
    util.overwrite_and_reset(playlist, new_playlist)

test_commands.py

Just to mention, tests were written before the code, TDD style, although the code may not reflect this ;) Admittedly I didn't do so religiously, I just wrote the tests before I wrote the code.

I'm wondering if these tests are readable at a glance (or two), or if you are currently ripping your hair off. Don't bother with the decorated playlist function, it works and is as concise as it can be. Just know that we get a mocked file object. Functions take the function as a parameter if they would like to use.

import pyfakefs
import pytest

from mutil import commands, util


@pytest.fixture
def playlist(fs):
    """Open a fake file object
    Args:
        fs: A reference to the `pyfakefs` filesystem
    """
    fs.CreateFile('playlist')
    with open('playlist', 'r+') as playlist:
        yield playlist


class TestRemovePlaylistDuplicates:
    def test_duplicates_are_removed(self, playlist):
        playlist.write(util.list_to_string(['a/b/c', 'a/b/c', '1/2/3']))
        playlist.seek(0)
        expected = util.list_to_string(['a/b/c', '1/2/3'])

        duplicates = commands.remove_playlist_duplicates(playlist)

        assert duplicates == 1
        assert playlist.read() == expected

    def test_non_duplicated_playlist_behaves_as_expected(self, playlist):
        playlist.write(util.list_to_string(['a/b/c', '1/2/3']))
        playlist.seek(0)
        expected = util.list_to_string(['a/b/c', '1/2/3'])

        duplicates = commands.remove_playlist_duplicates(playlist)

        assert duplicates == 0
        assert playlist.read() == expected


class TestPlaylistPathsUseRelative:
    def test_correctly_toggles_absolute_format_to_relative(self, playlist):
        playlist.write('/home/$USER/Music/album/track\n')
        playlist.seek(0)
        expected_playlist = 'album/track'
        library_path = '/home/$USER/Music/\n'

        commands.playlist_paths_use_relative(playlist, library_path)

        assert playlist.read() == expected_playlist

class TestPlaylistPathsUseAbsolute:
    def test_correctly_toggles_relative_format_to_absolute(self, playlist):
        expected_playlist = '/home/$USER/Music/album/track'
        library_path = '/home/$USER/Music/\n'
        playlist.write('album/track\n')
        playlist.seek(0)

        commands.playlist_paths_use_absolute(playlist, library_path)

        assert playlist.read() == expected_playlist

Please toast me 🔥

Update I want to add, can somebody tell me why my question now has 10 upvotes? In my eyes it's as crappy as all the other questions, if not crappier. Because, I would want to repeat this. Or maybe you guys are just nicer than the SO folks, and I'm flattering myself.

\$\endgroup\$
  • \$\begingroup\$ I'll try answer this, at a later date. However if I or anyone-one else don't, then ping me for another bounty, :) \$\endgroup\$ – Peilonrayz Sep 27 '17 at 11:59
5
+50
\$\begingroup\$

High Coupling

I don't think there's any problem with the high coupling between mutil.py and commands.py. In general you're going to have to somehow connect your argparse actions with the functions in your utility - at a certain point one has to call the other. The only problem here, I think, is that you've pretty tightly coupled your commands to argparse Actions, which may not be desirable. I think you're fine with the current separation of your Action classes and your playlist operation functions, but maybe put the actual Action classes into mutil.py as they are grouped with argparse, not your mutil. Then you only have the actual functionality in mutil.py, leaving you free to add as many front-ends to your utility.

If you don't like that you're creating a separate class for each function, you could create some type of factory class that creates an appropriate class, but that might be a little tricky as their __call__ dunder-methods are going to be a little different. You might be able to come up with something clever that isn't too complicated, but I think until you actually start running into bloat from too many classes you should leave it alone - YAGNI.

If you do run into that bloat, I'd see if you can separate the classes into ones with similar patterns - for example, RemovePlaylistDuplicatesAction would be in one group, while PlaylistPathsUseRelative and PlaylistPathsUseAbsolute would be in another one (the general structure of their __call__ dunders is the same). Then just make one factory for each pattern.


Commands

Use sets to handle duplicates

I think there are a handful of ways to simplify your code. For example, removing duplicates is really quite easy:

def remove_playlist_duplicates(playlist):
    old_playlist = [entry.strip('\n') for entry in playlist]
    unique_entries = set(old_playlist)
    new_playlist = '\n'.join(unique_entries)
    util.overwrite_and_reset(playlist, new_playlist)
    return len(old_playlist) - len(unique_entries)

Take arbitrary iterables

I also think it might be worthwhile to have a utility function where you hand any arbitrary iterable to it and it does any processing (such as making a new-line delimited string) necessary before writing the contents to a file. I'm also not positive that you'll always want '\n' to be your delimiter - do you care about what platform you're running on? You may also decide that you want some other format to be supported, in which case having the above utility function (and maybe another one to treat the file-like object as an iterable and do any stripping required) might be worthwhile.

Use more comprehensions

In general I think that using more list/generator comprehensions will help; they're often more efficient, and they're much nicer to read. Additionally, if you end up making the utility functions to have an arbitrary iterable write to file you can avoid buffering a lot of stuff in memory.

def playlist_paths_use_relative(playlist, library_path):
    library_path = library_path.strip()
    playlist_entries = [entry.strip('\n').strip(library_path) for entry in playlist]
    new_playlist = '\n'.join(playlist_entries)
    util.overwrite_and_reset(playlist, new_playlist)

Avoid useless comments

Also, you had a handful of comments, e.g. # Overwrite the old playlist that literally just tell us what the function name tells us. If a comment tells us how the code works, rewrite your code to make sense. If a comment tells us what the code does, consider replacing with a descriptive function name (which you already had). If a comment tells us why the code does what it does (or worse why it works) then it might be worthwhile keeping it in; still consider rewriting to make it simpler, or moving into a function (i.e. def remove_extra_element_from_3rdpartylib_bug or something). Comments slow down developers from reading or understanding the code, and can often be replaced by cleaner code.

Remove some duplication

All three of your functions have some of the same code. You could fix that in a few ways, perhaps with a decorator

from functools import wraps

def update_playlist(f):
    @wraps(f)
    def _(playlist, *args, **kwargs):
        new_playlist = '\n'.join(f(playlist, *args, **kwargs)
        util.overwrite_and_reset(playlist, new_playlist)
    return _

You could also do it with some higher order functions

def update_playlist(f, playlist, *args, **kwargs)
    new_playlist = '\n'.join(f(entry, *args, **kwargs) for entry in playlist))
    util.overwrite_and_reset(playlist, new_playlist)

remove_playlist_duplicates is a little bit different because it returns a value (which is a very tiny code-smell, because you're doing two things, but I think that's okay), but again you could partition your functions into different groups and apply the appropriate decorator/higher order function to each.

Pass an already stripped library_path

There's no reason that your utility functions should know that library_path may need its whitespace stripped. Similarly, especially if you use something like update_playlist above where your function processes a single entry, you could pass an already pre-processed playlist to the functions. This nicely separates out the concerns of formatting the playlist, and of processing the playlist.

Be platform aware

As far as I can tell it won't be an issue so far, but be aware that different platforms have different file separators (and line endings).


Tests

I'd add in some additional tests, e.g.

  1. Empty playlists
  2. Invalid playlists
  3. Subset of (2), but if you pass an already relative playlist to the use_relative function (or similar with the absolute) what happens?

Also, you're using some of the same values for paths and playlists and whatnot, so make those constants somewhere.

\$\endgroup\$
  • \$\begingroup\$ I thought a little about remove_playlist_duplicates doing two things. It modifies a file (the side-effect) and then it returns a value, but being connected in a way (the value returned is the amount of duplicates) I don't see how this qualifies as two separate things, and why that's bad. Can I keep the return value somehow, and why exactly is it bad when they are in essence part of the same thing? \$\endgroup\$ – Martin Kleiven Oct 10 '17 at 21:07
  • \$\begingroup\$ @MartinKleiven there are some people who claim that no function should ever do two things, even when those two things make sense to be done together, or maybe can't be done separately. I'm not one of those people. I pointed it out because whenever you find yourself writing a function that does more than 1 thing you should consider why it does more than 1 thing, and if theres a better way to do it. In this case I think you're fine to let it do both things. \$\endgroup\$ – Dannnno Oct 10 '17 at 21:10
  • \$\begingroup\$ Sure, because I saw no way to separate those without making a horrible mess! Thanks! \$\endgroup\$ – Martin Kleiven Oct 10 '17 at 21:12

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.