1
\$\begingroup\$

Let's get right into it, I have a function downloadFile(url, path); Downloads file and saves it.

donwloadFile.js

module.exports = function donwloadFile(url, path) {
    return new Promise(res => {
        return axios({ url, method: 'GET', responseType: 'stream', })
            .then(({ data }) => {
                const file = createWriteStream(path);
                data.pipe(file);
                file.on('finish', () => {
                    file.close();
                    res({ ok: true });
                });
            })
            .catch(err => res({ ok: false, err }))
    });
}

Just there it already seems to violate SRP which i don't know what i can do about since the two functionality are part of each other, downloading and saving.

Ignoring that however, it's perfectly fine, it gets the job done and tells you what's going on.

to download a file you provide the url and the path where to save, simple.


However my main issue is that i want to add a progressBar (using progress library) to give some feedback and in order to do that i need to have access to data parameter of axios and the code becomes as follows.

donwloadFile.js

module.exports = function donwloadFile(url, path) {
    return new Promise(res => {
        return axios({ url, method: 'GET', responseType: 'stream', })
            .then(({ data }) => {
                const file = createWriteStream(path);
                data.pipe(file);

                const progressBar = new ProgressBar('[:bar] :percent | Time left: :etas.', {
                    complete: '=',
                    incomplete: ' ',
                    renderThrottle: 1,
                    total: parseInt(data.headers['content-length'])
                })
                data.on('data', (chunk) => {
                    progressBar.tick(chunk.length);
                })

                file.on('finish', () => {
                    file.close();
                    res({ ok: true });
                });
            })
            .catch(err => res({ ok: false, err }))
    });
}

and suddenly the download function isn't just downloading it's also spitting out a progressBar which is not ideal.


I had the idea of passing a callback which would be called inside data.on('data', callback)

But then we're doing the same thing again, the download function should only take two arguments, having a third one as callback makes it look like it's doing more than it says.


So my question is:

How can i keep the download function only downloads and also have the ability to show a progress bar ?

Also I'd welcome any healthy criticism, writing pretty and maintainable code has always been the bane of my existence, and i just got fed up and decided to learn some concepts.

\$\endgroup\$
8
  • \$\begingroup\$ Welcome to the Code Review Community. While your concerns about the code are valid, our goal here is to help you improve your coding skills by providing insightful observations about working code from your projects. We don't help write the code. Please read Where can I get help? and How do I ask a good question?. \$\endgroup\$
    – pacmaninbw
    Mar 6, 2022 at 13:55
  • \$\begingroup\$ @pacmaninbw How is this asking to write code ? I've already provided everything and only asking how i can refactor it to maintain a rule of clean code while having the same functionality. If this isn't what Code Review Community is about, Could you point me to which of the stackexchange communities that are appropriate for such question. \$\endgroup\$
    – user197941
    Mar 6, 2022 at 14:03
  • \$\begingroup\$ You have already made most of the observations anyone could make about the code. Please note I did provide an answer as well. I did not down vote the question or vote to close it. Please read the How do I ask a good question? link so your questions are a better fit in the future. How to questions are off-topic on Code Review. \$\endgroup\$
    – pacmaninbw
    Mar 6, 2022 at 14:05
  • 1
    \$\begingroup\$ @hfontanez Yes i just wasn't sure about it so i decided to ask while i'm at it, thanks for the feedback. \$\endgroup\$
    – user197941
    Mar 6, 2022 at 14:53
  • 1
    \$\begingroup\$ So if i'm to show progress a function won't cut it, i might as well write a download class with a progress event that i can subscribe to which is basically equivalent to passing a callback to a function might be more cleaner that way. \$\endgroup\$
    – user197941
    Mar 6, 2022 at 22:00

1 Answer 1

2
\$\begingroup\$

Is SRP being violated?

I think the first version of donwloadFile.js is fine and obeys SRP. That said, I believe a cleaner solution is to refactor this method to return a file and then create a function that saves the file. I believe this is what @pacmaninbw suggested in his or her answer.

When to refactor?

If you find that you are copying the same lines to save a file in other parts of the code, you found yourself a refactoring opportunity. Now the question is how would you refactor. The way I have handled this in the past, and to stay true to Open/Close Principle (OCP), is to include a new version of the same method with this refactoring in place. In other words, I expand the functionality, instead of modifying it. That way, legacy code can continue to call the old downloadFile.js, while new code calls the new version. In fact, sometimes I would modify the legacy function where the legacy function will call the new function. To me, it is OK for legacy functions to depend on new functions (obviously, not the other way around).

What advantage does this approach have?

I can think of two:

  1. You won't have to worry about failing test cases due to unforeseen side effects of this modification
  2. If you ever retire the legacy code, you don't have to worry about dependencies (new functions don't depend on old functions).

So, in my humble opinion, even if downloadFile.js violates SRP, the best way to handle it might be leaving it alone and simply expand your functionality by adding a new version (or variant). That said, if the code is not in production yet, feel free to make all the modifications needed to have the cleanest implementation you could possibly get before your targeted release date.

Disclaimer: If you have core functions that you are OK with being part of the application forever, it is then OK for new functions to depend on these (core) old functions. SOLID Principles are recommendations, not gospel. It's like we striving for perfection. We will never get there no matter how hard we try. BUT, that should not stop up in trying our best to achieve it. In the end, the important thing is for us to be the best version of ourselves than we can be. Likewise, our code will NEVER be perfect. But, that should not stop us in developing the best applications we can. There are going to be situations when we would have to modify old code. We should be OK with that as long as it is not a common occurrence. The important thing is that, when opportunity present itself, we do a better job than the previous time to ensure these "violations" occur less often.

\$\endgroup\$
6
  • 1
    \$\begingroup\$ I don't think it's a good idea to save downloaded payload because it will be sitting in memory just to pass it to a save function, I had that to begin with then I switched to streams because i was hitting memory limits with large files. Now streams do provide chunks that i can be saving gradually with a separate saving function, However this brings us back to the actual question I'm asking, Which is how to access those chunks within the donwloadFile.js and at the time keep the function intact with a single purpose so to speak \$\endgroup\$
    – user197941
    Mar 6, 2022 at 17:40
  • \$\begingroup\$ @ZohirSalak "I don't think it's a good idea to save downloaded payload because it will be sitting in memory just to pass it to a save function..." - it will be in memory regardless you save it or not. Eventually, memory will be reclaimed by the garbage collector. So, I won't worry about that too much. \$\endgroup\$
    – hfontanez
    Mar 6, 2022 at 17:54
  • \$\begingroup\$ That is not what i meant, before i was downloading files as arraybuffer which broke because it couldn't hadnle files larger than 512MB streams don't work like that, they download files in chunks which do sit in memory but will never hit memory limits \$\endgroup\$
    – user197941
    Mar 6, 2022 at 18:31
  • \$\begingroup\$ @ZohirSalak if you are having problems with large files, perhaps you should try this: stackoverflow.com/q/42614880/2851311 \$\endgroup\$
    – hfontanez
    Mar 6, 2022 at 19:08
  • 1
    \$\begingroup\$ I understand that, Im mainly asking this question because i want to starting learning how to clean my code, and that is why i tried to implement the solution you suggested, that is where i ended up back in the same issue that lead me to ask the question. The question about the original function was merely a side and small question and didn't want to post a second question just for that. The main question was at the bottom. \$\endgroup\$
    – user197941
    Mar 6, 2022 at 21:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.