Ricther Scale Calculator: Code Review

[muted bear]

Hello I made a small C++ project pertaining to earthquake calculations and the ricther scale today! This is another project to help build my portfolio as a software dev I would really appreciate any feedback and also if you would consider giving me some tips on where to improve, thank you!

https://github.com/lsclarke/Ricther-Scale-Calculator

[gilded garden]

Two super surface-level things: 1) It's Richter, not Ricther (pretty sure you can change github repo names and it'll setup a redirect from the old name to the new so it doesn't break links) 2) set up a .gitignore, so you're not adding build artifacts to the repo, you should only be putting the actual code into git, DLLs or EXE-files can be published via the "Releases" feature of Github, if you want

[muted bear]

@gilded garden do you have any other takes on my project? I also applied the change from your comment earlier!

[gilded garden]

somehow the gitignore doesn't include all the build file stuff generated by the VS compiler. *.exe, *.obj, *.ilk or what they are

you also need to manually remove those files from the repo afterwards, the gitignore doesn't "kick" them retroactively

[gilded garden]

Quick glance at the main method: Remember that C++ is not java. You should only put things on the heap (in your case by doing new Earthquake) if you have a good reason to. Otherwise, you can just leave stuff on the stack like Earthquake a{9.3} or something.
If you need things on the heap, make sure to use smart pointers (or collections) so you don't have to worry about cleaning up

also, you don't have to put all your code in classes. If some function doesn't naturally "attach" to some piece of data, that's okay, just leave it as a free function and put it into a namespace instead, no need to construct an empty object to be able to access it

(in particular your RichterCalculator class doesn't contain any members, but its methods are non-static, so you need to construct an otherwise useless object to be able to use its methods)

[muted bear]

Oh thank you! I've never made a namespace before haha so this will definitely be good practice!!

[muted bear]

@gilded garden I just made the update here!

I changed the class to a name space instead

//Richter Scale Calculator
namespace rsc {
    /// <summary>
    /// Responsible for calculating the magnitude of earthquakes!
    /// </summary>
    double CalculateEnergy(Earthquake* quake) {
        quake->energy = (2.5 * pow(10, 4)) * pow(10, (1.5 * quake->magnitude));
        return quake->energy;
    }

    /// <summary>
    /// Responsible for calculating the difference in intensity between two earthquakes!
    /// </summary>
    double CalculateIntensityDifference(Earthquake* quake1, Earthquake* quake2) {

        double magnitude_Difference = (quake1->magnitude - quake2->magnitude);
        double diffIntensity = round(pow(10, magnitude_Difference));

        quake1->intensity = quake2->intensity = diffIntensity / 2;
        if (quake1->magnitude > quake2->magnitude)
            std::cout << "Earthquake #1 Intensity:: " << quake1->magnitude << " was approximately " << diffIntensity << " times stronger than Earthquake #2 Intensity:: " << quake2->magnitude << std::endl;

        if (quake1->magnitude < quake2->magnitude)
            std::cout << "Earthquake #2 Intensity:: " << quake2->magnitude << " was approximately " << diffIntensity << " times stronger than Earthquake #1 Intensity:: " << quake1->magnitude << std::endl;

        return diffIntensity;
    }
};

[gilded garden]

another thing that you should aim for is to separate calculation of "stuff" from the presentation. In your case, the CalculateIntensityDifference doesn't only calculate the difference, but it also outputs the difference to the console in your case. When you're writing a more real-world application, the output will often be a GUI or a a web API route or something like that, and by "baking" the output into the computation function, you're pretty much preventing that computation from being re-used somewhere else

[muted bear]

Thank you for that! So I should take out the print outputs from all my namespace functions?

I plan on using this repo for an unreal project as well for a earthquake simulator 🌍

or in a gui

qt project

[gilded garden]

In a bigger-ish project, you'd have the output in a separate namespace or something. In your case, a good start would be to have the output in a separate function at least

[muted bear]

I'm starting to see how custom namespaces are very useful thank you!

Also what is best practice when creating a namespace? Should I have it in a seperate header from everything else?

I'd like to have it so that others can download it as a package/plugin like curl or crow and other cool well know c++ libraries?

but this is new territory for me haha

[gilded garden]

one useful aspect with namespaces, compared to a class that has only static methods, is that you can easily distribute it over several files. Also nesting namespaces is an option

often times, you'll have one big namespace for an entire library or even a group of libraries that are meant to be used with each other (but can be downloaded separately) and then inside that big namespace, you'll have nested namespaces for more specific features

a common pattern is to have a mylib::_detail namespace where you put stuff that needs to be public for the library to work, but which shouldn't be of interest to the "average user" of the library, so it stays out of the way during auto-completion etc

[muted bear]

Why not just make it in one idk maybe its my inexperience C++ brain but it just sounds like it would make more sense and easier on users to have it in one name space like the **std **library ?

[gilded garden]

basically the same reason you put stuff in folders on your hard drive

you can group stuff that belongs together, and prevent conflicts between stuff that happens to be named the same way

this is a funny trap to fall into btw... in a project I worked on, I had a class Tree for a tree data structure in one part and a class Tree for the wooden plant in another part in such a way that no single .cpp file could see both definitions, so there was no error. But the linker would throw away one of the definitions because it would consider both to be the same class Tree, due to them being in the same namespace, and that lead to the weirdest crashes ever at runtime.

Just having two different sub-namespaces for these two different types of tree would've prevented that issue entirely

[muted bear]

That makes a lot of sense, and wow yeah then I really need to be careful with designing/developing functions to make sure they dont have this issue. I would like to add more to this name space honestly !

I'm not really sure how to actually even convert a namespace to a library itself for people to download and use haha. I'll probably go more into the shipping/creating library for distribution after i complete it haha

I did find this tutorial but it went over my head

https://www.youtube.com/watch?v=Slfwk28vhws

[gilded garden]

you probably don't want a shared library for this in particular. "Shared" in this case doesn't refer to you sharing it with other people, but the same compiled code being potentially shared between several compiled executables without directly being a part of any of them. But in reality, shared libraries have a ton of issues, especially with C++.... I'd suggest you look into something like CMake (which is the closest thing to a "standard" build system for C++) you can define your library as a (static) library there, then if you put it up on github, somebody else can include your library as a subproject, use add_subdirectory in their own CMake file to attach it to their build process and instruct CMake to link it into their executable

Both shared and static libraries have in theory the option that you distribute just the headers and the library files (*.so/*.dll or *.a/*.lib). But for C++, these files are not compatible between different compilers, sometimes not even between different versions of the same compiler (unless you put a lot of effort into making them) and of course not between different OSes. This way of distribution also brings issues with optimization, as the compiler translating the "main" program can't "look into" the library code to identify potential for optimization, like functions that could be inlined, or pure functions

[muted bear]

I took the time to update my project after some review and realized it's best to use smart pointers instead of regular pointers!

///BEFORE
int main
{
    Earthquake* quake1 = new Earthquake(9.3);
    Earthquake* quake2 = new Earthquake(8.7);

    Earthquake* quake3 = new Earthquake(9);
    Earthquake* quake4 = new Earthquake(6.7);

    Earthquake* quakeEnergy = new Earthquake(9.3);

    rsc::CalculateIntensityDifference(quake1, quake2);
    rsc::CalculateIntensityDifference(quake3, quake4);

    std::cout << "Energy Output:: " << rsc::CalculateEnergy(quake3) << std::endl;

    std::cin.get();
    return 0;
}

[stable valley]

@muted bear

It looks like you may have code formatting errors in your message

Note: Make sure to use back-ticks (`) and not quotes (')
Note: Make sure to specify a highlighting language, e.g. `cpp`, after the back-ticks

Markup

```cpp
int main() {}
```

Result

int main() {}

[muted bear]

///NOW
int main
{
    //Smart Pointers 
    std::unique_ptr<Earthquake> quake1 { new Earthquake(9.3) };
    std::unique_ptr<Earthquake> quake2{ new Earthquake(8.7) };

    std::unique_ptr<Earthquake> quake3{ new Earthquake(9) };
    std::unique_ptr<Earthquake> quake4{ new Earthquake(6.7) };

    std::unique_ptr<Earthquake> quakeEnergy{ new Earthquake(9.3) };


    rsc::CalculateIntensityDifference(quake1, quake2);
    rsc::CalculateIntensityDifference(quake3, quake4);

    std::cout << "Energy Output:: " << rsc::CalculateEnergy(quake3) << std::endl;

    std::cin.get();
    return 0;
}