nekiro / Nekiro-Rcc-Editor

Electron qt .rcc file editor written using typescript and react
10 stars 7 forks source link

Code enhancements #1

Closed bpawel10 closed 2 years ago

bpawel10 commented 2 years ago

reader.js:7 Why bluebird? fs-extra docs say „All fs methods return promises if the callback isn't passed.”.

reader.js:12 Just use ternary, also I’m not sure if we need a function here? Can this app.isPackaged value change during runtime?

reader.js:28 Use Promise.all to make it faster. You could also consider using one .reduce instead of two .map and this .reduce would return an object like { files: […], folders: […] }.

reader.js:34,99,102,108,132 Unnecessary template literal.

reader.js:41 In all these places where you want to join multiple parts of path you can use path.join that will handle some things like slashes for you.

reader.js:42 Empty catch, that’s interesting. Why?

reader.js:57 To make it a little bit shorter you can also write isImage: [".png", ".jpg"].includes(ext),. And it would be a good practice to extract this array to a constant, maybe even in a separate file with constants. And the same applies to other const strings, like errors or messages.

reader.js:75 To make it a little bit shorter you can also write if (!images.length).

reader.js:82,114 Promise.all again.

reader.js:97 Instead of this line you can probably use default parameter (async (filePath = loadedFilePath) => {).

reader.js:153 Short syntax for returning?

mainRenderer.js:17 This line looks ugly, maybe there’s a better way to handle passing and retrieving this id?

mainRenderer.js:44 Why not for … of here again? If you need indexes you can write for (const [index, image] of images.entries()) or use images.forEach.

mainRenderer.js:51,68 I’m almost sure you can use const here. It’s because js will only check references, so if you assign different value to a const variable it won’t allow you to do that because it would be different reference, but if you only change some properties of the object without changing it, it will work. It’s also possible to create a const array and modify its elements. So images variable in reader.js:10 could also be const.

mainRendererer.js:52 Unnecessary template literal.

nekiro commented 2 years ago

About fs-extra I didnt really know that xD

Yes, isPackaged changes at runtime, apps that are not in development mode are having this as true.

getFiles was copied from stackoverflow tbh.

I don't get though, why some of these are unnecessary template literals, what do you want me to use instead?

bpawel10 commented 2 years ago

But if you run the app in development mode then you can't change it without restarting, right? In this case you don't need a function, a variable will be enough.

About template literals, if you don't add anything to the variable, it doesn't do anything. So `${a}/` makes sense because it'll append / at the end, but `${a}` doesn't, because instead you can just write a and it'll be the same (assuming a itself is also a string).

nekiro commented 2 years ago

But if you run the app in development mode then you can't change it without restarting, right? In this case you don't need a function, a variable will be enough.

About template literals, if you don't add anything to the variable, it doesn't do anything. So `${a}/` makes sense because it'll append / at the end, but `${a}` doesn't, because instead you can just write a and it'll be the same (assuming a itself is also a string).

I created that function to avoid writting ternary everywhere, it's used to get proper path to resource directory.

bpawel10 commented 2 years ago

Sure, but why not

const getResourcePath = () => app.isPackaged ? process.resourcesPath : ".";

or, if it can't change without restart:

const resourcePath = app.isPackaged ? process.resourcesPath : ".";
nekiro commented 2 years ago

Sure, but why not

const getResourcePath = () => app.isPackaged ? process.resourcesPath : ".";

or, if it can't change without restart:

const resourcePath = app.isPackaged ? process.resourcesPath : ".";

I agree, could be a variable. I pushed some changes, you can look at the code again.

bpawel10 commented 2 years ago

Yup, looks better, you didn't change all the things though.

bpawel10 commented 2 years ago

Also if you want to catch this kind of issues yourself, I recommend using eslint. And prettier for auto code formatting on save.

nekiro commented 2 years ago

Also if you want to catch this kind of issues yourself, I recommend using eslint. And prettier for auto code formatting on save.

I do use prettier for formatting, thought I didnt setup eslint in that project and I dont think I have it in vscode too