fredholmP
commented
2 years ago Thank you for your feedback!
I agree that using external libraries do handle the dates would have been much better, thank you for the suggested library! If I had more time to look more carefully at the csv-parser libraries using those instead of my own implementation would probably also have been much better.
As for some of the points, I agree with that the constructor was way too long. Due to some turn of events, I was not able to spend as much time as I wanted on all the parts of the project and “polishing up” the constructor by implementing member functions for i.e splitting strings and setting up vector structures was one of those things.
As for the confusion about the “rootDataReader.h”, that was not intended to be included in the main.cpp-file, the “dataReader.h” was. When writing the code, I first experimented with using Root in a normal C++ file by including things and using some flags I found on some of the Root forums to pass to the g++ compiler, but after that didn’t work, I decided to try to use a Root macro instead.
Since Root did not play nicely with the translation units, I put everything into the rootDataReader.h-file to be able to include it into a root macro. When trying to debug some issues with the root macro, I added an intermediate step with processing the data in a normal C++ file (the main.cpp file) to see what was wrong. I eventually gave up on using one entire Root macro, but I must have forgotten to change the include statement to dataReader.h. I also probably did not notice since the files are very similar (although not exactly, dataReader.h has a different implementation of getting the first and last year of measurements). This might also explain why I ran into a lot of issues with try{}catch(…) statements not behaving as expected when testing changes to the dataReader.cpp-file when trying to locate what caused “ segmentation errors”.
The issues with doing everything in a root macro turned out to be related to some really strange behaviour where i.e C++ code on Aurora would give errors for very simple code (such as “Hello World” scripts giving errors about missing libraries) that worked just fine when copy-pasted and ran on my local machine. (I had the Gcc module on Aurora loaded)
I had a similar situation with ROOT on Aurora where I located the errors to be caused from i.e trying to access elements of vectors. This even appeared in very simple test macros that worked just fine on my local computer (I tested both with the .L
I am definitely going to keep in mind to delete code I am not using henceforth to avoid these kinds of issues.
Hi everyone,
here is some feedback on the C++ part of your project that I hope might be helpful for you. Note: These are not corrections that you need to do, its just for the future!
On the ChrisCode part
Save your code in an actual format with a useful name... Unsaved Document 1 isn't particularly helpful for anyone reading your code
Format your code. Again, you want to optimize for people reading your code, not to save yourself time when writing it. Formatting code helps people understand what your code is doing.
Give your variables descriptive names. What's the difference between the "hist" and the "histo" variables? What does "tmpe" mean? etc On the PhilipCode part
main.cpp
When including your rootDataReader header, don't add the include part! Use -I with your compiler to tell it where your headers are located.
You don't need to take readData as a pointer to avoid copies, the default choice to get a view to something is to take it by reference to const (const readData&). If you tried this and it didn't work, it may be because you haven't marked the member functions you use that don't change the object as const.
I'm not a huge fan of the maxmin parameter, using an integer to represent one of a finite set of elements isn't a great interface. We haven't covered this in the course, but I would recommend looking up enum classes.
What happens if someone enters an invalid chosenYear? In general, I think this is a sign that you are missing a layer of abstraction. What the person writing the code wants is to get the data corresponding to a particular year, so provide a member function that does this job instead. You really want to avoid member functions like getData that expose the internals of your objects, it breaks encapsulation.
You reference that you have some issues with handling the number of days in a month. Instead of trying to solve this on your own, you could consider using a date library and represent years, days, and months as proper dates instead of just raw integers. This would have made your life overall much easier! See e.g. https://github.com/HowardHinnant/date
I would strongly advise against creating uninitialized objects like your compareValue and extremalDay variables, it is much too easy to end up missing initialization on one of the branches. In general, try to declare and initialize your objects when you actually need them (inside the two branches). You don't need the extremalDay variable to be accessible outside the if/else condition to return it. You can have more than one return statement in a function!
Instead of writing the code to produce a flat year in your functions, write a suitable member function that returns this representation!
You have quite a lot of raw loops, consider if you can replace some of them with an algorithm instead. If you can't consider writing these as separate functions.
I'm not sure what you need the dayCounter variable for? If it is just the number of days that you processed, since you are looping over all values it should just be the same as .size()!
checkSummer Nice work on using accumulate for calculating the mean!
main
You aren't changing the filename, so you could mark it as const. This probably applies to the readData variable as well, but you can't do it at the moment since its member functions aren't marked as const.
rootDataReader/dataReader
It is a bit unclear which one of these you actually intended to keep. Don't keep code you aren't using in your sources, if you make use of git you can always restore things if you later learn that you needed them. Your makefile happens to work but contains a bunch of errors. In particular, try running the make dataReader.o target.
At the moment, what happens is that you are including the rootDataReader.h header, which happens to define the same things that are in the src/dataReader.o object file (ps. don't add object files into git!). So the linker happens to accept this file by accident even though src/dataReader.o was created from a different file. This is a nightmare waiting to happen!
Since you edited the dataReader translation unit most recently, I'll give feedback on that one!
As mentioned above, provide more interfaces to things that your other code needs in your readData class, and mark things that don't change the object as const. Note, you want to mark member functions as const, not member variables.
You don't need the this-> to reference member variables, when you are in the body of a member function you can access these directly.
Instead of manipulating filepaths manually, consider using a library (e.g. the c++17 std::filesystem library!).
Instead of parsing the file manually, maybe you can guess what I'd suggest based on the previous comment :)
If you want to create a vector with a bunch of default values (like you do in the loop over number of columns), see constructor 4 over at https://en.cppreference.com/w/cpp/container/vector/vector
Consider not abbreviating temperature as temp, it is a really common thing for people to mix up whether temp/tmp is an abbreviation for temporary or temperature. In general, be careful with using abbreviations and only use them when they actually make the code easier to read!
Your constructor is WAY too long, it does a whole bunch of things. Separate it into different functions.
For getting min and max elements, you can use https://en.cppreference.com/w/cpp/algorithm/minmax_element.