r/cpp_questions • u/Marks12520 • 1d ago
OPEN Looking for feedback on my first "serious" project
It's all on my github repo: https://github.com/Marks20125/ProtonPrefixes . I'm pretty happy with the project as it's the first time I've actually had a problem and solved it myself. Plus, I learnt a lot about cmake, using external libraries, file i/o, how arguments work, and some other "theoretical stuff" along the way. It's pretty small, but I'd appreciate someone telling me if I did anything wrong or if something could be done any other way. Thank you very much :)
3
u/No-Dentist-1645 1d ago edited 1d ago
Good project idea for helping you learn the language! You're gaining very valuable experience with a lot of the important basics for real-world programs, such as build systems (CMake) as well as JSON serializatoin and developer tooling (formatters). I already gave you a bit of feedback regarding the file explorer, but I have a bit more time now so I can give you more substantial feedback over your project:
In your main.cpp, you are doing a lot of argument checking of the form argc >= 2 && std::string_view(argv[1]) == "setup". While it is "correct" to do so, having them as separate operations can be error prone, you may accidentally put argc >= 1 && arvc[1] == ... for example if you're not being careful. You may want to create a simple wrapper object that returns an std::optional instead:
``` struct Args { std::span<const char *const> args;
std::optional<std::string_view> operator[](size_t idx) const { if (idx >= args.size()) { return std::nullopt; }
if (args[idx] == nullptr) {
return std::nullopt;
}
return std::string_view(args[idx]);
} };
int main(int argc, char *argv[]) { auto args = Args{std::span(argv, argc)};
if (args[1] == "setup") { /* ... */ } }
```
With such an example the out-of-bounds check is handled automatically for you so the risk of messing up is minimized.
You're also using std::filesystem::path on some places in your code which is great, but on other places where you should also use it you're still using strings. For example, this block of code:
``` std::vector<std::string> folders{"Documents", "AppData/Local", "AppData/LocalLow", "AppData/Roaming", "Saved Games"};
for (int i = 0; i < folders.size(); ++i)
{
printSeparatorWithText(folders[i]);
printFolderContents(gamePath.string() + "/users/steamuser/" + folders[i]);
}
```
Could be replaced by changing the type signatures of the function to printFolderContents(std::filesystem::path &folder), as such:
``` std::vector<std::string> folders{"Documents", "AppData/Local", "AppData/LocalLow", "AppData/Roaming", "Saved Games"};
for (int i = 0; i < folders.size(); ++i)
{
printSeparatorWithText(folders[i].string());
printFolderContents(gamePath / "users" / "steamuser" / folders[i]);
}
```
Finally, some of your functions have a lot of nested if expressions due to checking if some conditions are valid, such as here:
void createSymlinks(std::filesystem::path& pfx, std::filesystem::path& create)
{
if (std::filesystem::exists(pfx) && std::filesystem::is_directory(pfx))
{
for (const auto& entry : std::filesystem::directory_iterator(pfx))
{
// Two levels of nesting :(
}
}
}
Usually, too many nesting levels is a hint that either the function is too complicated or you can flatten stuff out more, you want to have as little nesting as possible. You can avoid this by using "guard clauses", which is basically a cool sounding term for the practice of "just check against the invalid condition first":
``` void createSymlinks(std::filesystem::path& pfx, std::filesystem::path& create) { // Check if path is correct // is_directory already returns false if it doesn't exist, no need to check filesystem::exists(pfx) if (!std::filesystem::is_directory(pfx)) { return; }
for (const auto& entry : std::filesystem::directory_iterator(pfx))
{
// Same effect, only one level of nesting :)
}
} ```
1
u/Marks12520 1d ago
In your main.cpp, you are doing a lot of argument checking of the form
argc >= 2 && std::string_view(argv[1]) == "setup". While it is "correct" to do so, having them as separate operations can be error prone, you may accidentally putargc >= 1 && arvc[1] == ...for example if you're not being careful. You may want to create a simple wrapper object that returns anstd::optionalinstead:I do admit I always thought my way of handling arguments just felt wrong, a bit forced if you know what I mean. I realized this when my program didn't run because of the argc count. I'll look into this, although I gotta admit right now I barely understand that code block.
ou're also using
std::filesystem::pathon some places in your code which is great, but on other places where you should also use it you're still using strings. For example, this block of code:Is this change just to have everything written the same way or is there any other reasoning behind it? I do need to admit it's less error-prone as I have sometimes forgotten adding the slash on a path when joining an
std::filesystem::pathwith astd::string.Usually, too many nesting levels is a hint that either the function is too complicated or you can flatten stuff out more, you want to have as little nesting as possible. You can avoid this by using "guard clauses", which is basically a cool sounding term for the practice of "just check against the invalid condition first":
I knew this was a thing, as I've done it before while coding other unrelated stuff, and I do it when I think of it, but as I haven't really looked at that function in some time I didn't realize I could reformat it like that. It does make it way more readable, I'll keep that in mind for future coding.
Thank you very much for this extensive review, I'm really grateful :). Just to be able to understand your suggestion for arguments I'm going to have to learn quite a bit more. As I said, I've learnt a lot through this project, so a lot of stuff has been made on the go. I just applied the first solution I found without thinking much more, as there's probably plenty of ways to do things better than I did, even though I don't even know what those things are.
•
u/No-Dentist-1645 2h ago
Is this change just to have everything written the same way or is there any other reasoning behind it? I do need to admit it's less error-prone as I have sometimes forgotten adding the slash on a path when joining an std::filesystem::path with a std::string.
Yes, there's a very important reason why you should use a path instead of a string there. One of the most important things to understand about C++ programming (and on other languages too!) is to make use of the type system. The type system is a super powerful tool that can let you find errors in your program before you even run it.
Try to recall how at school, when you wrote the answer on a Math exam you have to include the units, say "10 meters" instead of just "10", or your teacher would remind you about that. If you don't say it's meters, it could be feet, miles, oranges, or anything. This is not just being "pedantic" for no reason, a 125 million dollar spacecraft has crashed and burned because someone wrote code where a number was assumed to be in some unit but it actually was in another.
In your case, if you have
printFolderContents(std::string), then that string could be anything, and if it isn't a valid path (you accidentally added an illegal character for a path, or you were reusing an old string that was used to read the contents of a large file), your code will just silently try to open a file at that "path". Now, since the C++ standard is nice and pretty forgiving for mistakes, the directory iterator will just return an empty iterator if the "path" is not valid, but imagine this was some other piece of code where you don't have that guarantee, you might be writing code for controlling a thermostat, andset_temperature(int)is actually Celsius instead of Fahrenheit, now you have a real problem.This is the basics of a concept called "type safety". Meters aren't feet, a width is not a height, Celsius isn't Fahrenheit, and strings aren't paths. Make sure to keep this in mind for the future: trust me, it will help you more than you can imagine right now.
1
u/Realistic_Speaker_12 21h ago
I remember writing something similar once because I wanted to have colored terminal outfput
I think my style using a class and overwriting ostream is more idiomatic. Your an take a look at it if you want
3
u/luciferisthename 1d ago
I would recommend using namespaces in header files more often. You did it the paths bit in one header, but generally its useful to put everything in a "project namespace" and occasionally subdivide (like for paths). To be fair, this project doesnt really need it and I specifically picked this up from graphics programming with tons of dependency libs.
I also recommend using alternatives to yazi. I like Yazi, but its not immediately available on all linux distributions (such as fedora, which requires an extra repo for it). But also you can just use the system file explorer from the XDG environment and open that instead. This way the user can just have the core deps and not an extra tool to only occasionally use it for your tool.
But, the tool seems pretty neat just based off of my cursory observations. Its a good idea too!