My fourth week ever learning to program in my computer science class and I was just wondering if there was anything I could improve on from
coding conventions to improving how efficient/long my code is. Thank you :3
EDIT: Updated all code to latest code as of 3:07PM 10/23/13
-snipped code so i dont get bitched at by prof-
You should use switch statements instead of the if statements for months.
[QUOTE=alien_guy;42585075]You should use switch statements instead of the if statements for months.[/QUOTE]
>.< I just realized that as I was going over it... I'ma change that
Its also convention(Microsofts) to write private variable names with an underscore at the front.
[QUOTE=alien_guy;42585105]Its also convention to write private variable names with an underscore at the front.[/QUOTE]
Changed to a switch and the private variable naming convention :)
nevermind i'm dumb sorry
[QUOTE=alien_guy;42585105]Its also convention to write private variable names with an underscore at the front.[/QUOTE]
This depends on the coding style you are using. Some do that, yes, but not all. It doesn't hurt to do it though.
[QUOTE=NixNax123;42585139]instead if #ifndef and #def headers you can just use #pragma once[/QUOTE]
#pragma isn't standardized, so while this should work most of the time I'd avoid it.
Alright, don't use #pragma. Also, no idea what the headers actually do it the header file. Assuming it has to do with what and where to read inside the file? CodeBlocks kind of just threw them in there.
[QUOTE=hexpunK;42585154]This depends on the coding style you are using. Some do that, yes, but not all. It doesn't hurt to do it though.[/QUOTE]
Also depends on the language right? Never done it in Java.
[QUOTE=mobrockers;42585301]Also depends on the language right? Never done it in Java.[/QUOTE]
The coding styles we tend to use in Java never really seem to use _varName, largely because Java really, really pushes the idea of encapsulation, so there should never really be any confusion as to public/ private. But there are some people who use those styles in Java, styles tend to be pretty language agnostic.
[editline]20th October 2013[/editline]
[QUOTE=Tamschi;42585166]#pragma isn't standardized, so while this should work most of the time I'd avoid it.[/QUOTE]
Most (if not all) modern compilers support "#pragma once" at least, maybe not other #pragma statements though, so be careful. Header guards are safer, just a bit more labour intensive.
[QUOTE=hexpunK;42585391]The coding styles we tend to use in Java never really seem to use _varName, largely because Java really, really pushes the idea of encapsulation, so there should never really be any confusion as to public/ private. But there are some people who use those styles in Java, styles tend to be pretty language agnostic.
[editline]20th October 2013[/editline]
Most (if not all) modern compilers support "#pragma once" at least, maybe not other #pragma statements though, so be careful. Header guards are safer, just a bit more labour intensive.[/QUOTE]
Have a guy in my Java class who started with C# and learned Hungarian notation, looks so out of place :v:
[QUOTE=hexpunK;42585391]The coding styles we tend to use in Java never really seem to use _varName, largely because Java really, really pushes the idea of encapsulation, so there should never really be any confusion as to public/ private. But there are some people who use those styles in Java, styles tend to be pretty language agnostic.[/QUOTE]
I use it to distinguish private fields and variables, public members in C# are usually UpperCamelCase though.
I think it would be used for the same reason in Java, as there usually aren't any public fields in the first place anyway.
I'm unsure about your current level and what comments you expect, so I just list everything that comes to my mind.
[B]Date.h/.cpp[/B]
(C++11 upwards only) Mark your class as "final" when it is not supposed to be derived from (your private members and non-virtual destructor indicate this here).
Types should be chosen as big as necessary and as small as possible. For example "day" will only cover a range from 1 to 31, so an unsigned short (or uint16_t) will suffice (actually an 8-bit type will, but they cause issues with IO streams)
Omitting the method argument names in your header may cause confusion for users of your class, e.g. when the code completion does not tell you what to pass, especially in this "int, int, int" case (is it "day, month, year" or "year, month, day"?)
Use an initialization list in your constructor instead of assigning the values in the body.
Your getters and printing methods should be "const", otherwise you cannot read the contents of a constant Date instance.
The date class should not use direct access to std::cout to output its content, because this massively limits the usage scenarios for your class (what if I want to output it to a file?). Instead, either pass an std::ostream& or just return a std::string. You can also provide an operator<< overload for std::ostream.
Date::makeMonthName() should be a static method, because it does not rely on any non-static data members, the output is only dependent on the argument given.
Said argument should just be a by-value int. A const& does not provide any advantage in neither performance, storage or intention.
The nested enum "monthNameList" also does not provide any advantage. Replace it with a std::string[12] and you get direct name lookup without an if/switch statement.
Error messages should be output to to std::cerr instead of std::cout. But here you shouldn't output errors at all (as it will end up in your constructed date string). Instead, let the function throw an exception or have some error return code.
Date.cpp includes <string> but never uses any.
[B]Main[/B]
Should not include "Date.cpp". This is a separate translation unit and will be linked in.
"using namespace std;" is not advisable, especially not at global scope. Either use full name qualification or just use "using std::cout; using std::cin;" etc.
Your "...Check()" functions should not take references. References indicate, that the value passed will be manipulated by the function, which is not the case.
"dayCheck()" contains the same enum your Date class has. Such redundancies should be avoided, because any adjustments need to be manually synchronized, plus you are introducing two separate types for the same thing.
Those while loops in your "...Check()" functions are very questionable design. The functions should just return a bool indicating if the value is valid or not and your main() should re-query the user for input. The way you are writing your functions and methods is too interwoven, which completely kills reusability.
So in general there is a lot to improve on, but this is alright for your level of experience I guess. Feel free to ask if you want more specific explanations on the things mentioned.
[QUOTE=Dienes;42585893]Your "...Check()" functions should not take references. References indicate, that the value passed will be manipulated by the function, which is not the case.[/QUOTE]
I'm not the greatest at C++ (after taking a break over the summer I totally forgot how to do arrays :v:), but this isn't always true is it? If you have a particularly large data structure and wish to pass it, passing by reference is acceptable to avoid cloning the structure right? Just mark it as a const and that should clear it up if I'm thinking about this correctly?
As for initialiser lists, I understand their use for references and pointers, but using them for initialising everything gives what benefits?
Sorry to hijack the thread a bit, just realised I could do with a learning.
You pass a const reference to provide read-only access to the original data. The caller can be sure his data will not change.
You pass a non-const reference to provide read-write access to the original data. The caller expects his data to be changed. This is the way out-parameters are done.
Passing a const int& does not provide any benefit over just passing a copy of the int, because the reference (essentially a pointer) usually occupies the same space as an int. If not optimized away by the compiler, it also adds a level of indirection.
Initialization lists let you specify which values to use when the object is constructed. If you do not specify anything for a member, it will be default-constructed, regardless of what follows in the body of the constructor. So the data member will effectively be assigned a value twice in this case.
Alright round two of the files after edits.
-snipped code-
Your leap year logic doesn't work. The rule is once every four years you add a leap year. However, if the year is divisible by one hundred, you don't add leap year. However, if the year is divisible by four hundred, you add a leap year.
[QUOTE=Jookia;42588858]Your leap year logic doesn't work. The rule is once every four years you add a leap year. However, if the year is divisible by one hundred, you don't add leap year. However, if the year is divisible by four hundred, you add a leap year.[/QUOTE]
it works for the years 1950 - 2020 :3? 2000 is /100 but also /400
Sidenote: Rated dumb in a thread where I'm asking for help. Haha people >.>
[QUOTE=Proclivitas;42589084]Sidenote: Rated dumb in a thread where I'm asking for help. Haha people >.>[/QUOTE]
Rated dumb for that bit.
If you program something do it properly or document the validity range and put in checks that throw exceptions if an unexpected parameter is encountered.
I you continue to program with this mentality you're going to be more of a liability when you do it for work or others.
[editline]edit[/editline]
I see there is a check for the range, but it definitely should be in the leap year function and documented there.
[QUOTE=Tamschi;42593534]If you program something do it properly or document the validity range and put in checks that throw exceptions if an unexpected parameter is encountered.
I you continue to program with this mentality you're going to be more of a liability when you do it for work or others.[/QUOTE]
Wow, let him get into programming first. This change in attitude will come at some point, but it's really not top priority when you are in
[QUOTE=Proclivitas;42585041]fourth week ever learning to program in my computer science class[/QUOTE]
[QUOTE=Proclivitas;42589084]it works for the years 1950 - 2020 :3? 2000 is /100 but also /400[/QUOTE]
This is on the topic of writing correct programs. Sure, you're only accepting input from 1950 - 2020, but your dayCheck doesn't know that. Changing yearCheck will break your dayCheck, which seem completely unrelated.
The problem with threads like this is, that the OP is only doing his first steps in programming, but asking somewhat experienced people to C&C his code, which never ends good. Everyone would like to see his code completely rewritten, obeying thousands of idioms, habits, guidelines and best practices the OP might have never heard about. It's hard to decide which advice should be given at this stage, otherwise he will just be overwhelmed, which might even have a bad influence on his fun and motivation for programming.
The way you code will always be changing, based on what you learn in school, learn from books and the web, and issues you run into when experimenting. I think admonishing and scaring him after few weeks of programming at school (which we all know is not the best source of information) is not appropriate.
For his small project, the OP wrote the functions to just do what is required. "If the task was to only accept A, B and C, why implement a solution that handles the whole alphabet?" We all did that when we started out, especially in school. When your projects are getting bigger and you realize you are writing stuff over and over because existing code is not reusable, you automatically start to generalize your code. And with generalization comes proper error handling. All that will evolve over time, your mind has to adapt to those abstract ways of thinking.
I understand what you're saying, and in a sense I am hijacking the thread a little because I want to nail a point in. If we're only here to discuss programming, I'd be quite happy to draw up some criticisms of the current revision.
Before you even start typing, you should be thinking about what you're trying to accomplish in the first place.
[code]//dayCheck will vaildate the day according to acceptable ranges
bool dayCheck(uint16_t& day, uint16_t month, unsigned short year,
bool& isLeapYearRef, bool& nonLeapYearRef, bool& shortYearRef, bool& longYearRef)[/code]
dayCheck has absolutely no indication in it, or in the code itself that it's only meant for the 1950 - 2020 range. There's no syntax errors, no compile errors and no runtime errors (yet) but just by contaminating your thoughts with the rest of the program, you've created a bug in your software.
This is absolutely correct and you should always be doing what you are saying for serious work.
But here we have a case of a fire-and-forget school project with a few lines. The task is very limited, he is working on it alone for no more than a few hours and the requirements exactly specify what should be handled. It is forgivable here to not make every function safe in every regard. There are many more quirks in the code which should have priority over this one.
OK, maybe I was a bit harsh, but this is a really important, really fundamental issue.
The inaccurate leap year function itself isn't the problem, it's the opinion that it doesn't need to be changed because another part of the program does validation.
Bugs like this are really dangerous because they can be masked by other components and then suddenly appear if a seemingly unrelated part of the program changes.
A good example would be a rocket controller where part of the software used a small value type due to the assumption of relatively low velocities.
[URL="http://www.around.com/ariane.html"]When the system was used on a faster rocket, this alignment function crashed the guidance system, making a self-destruct necessary[/URL].
Things like this are extremely important, probably more than having legible code.
In my eyes, illegible but well documented code is better than legible code that doesn't do what it advertises through method signature and documentation.
(I have to admit though that I don't always follow this if the input producing the erroneous result is obviously nonsensical, like a negative speed limit. If someone would point out an error with sensible values like this to me I'd change or document it very quickly though.)
General advice: Depending on what you work on, you have to be pretty careful what you do, and [U]always[/U] put an as-is-no-liability clause into licenses.
Very interesting links, thanks. (Edit: Wait, why did you remove the first one?)
I was only defending OP because of the scale and context of the project and his level of experience. You both are right in every aspect, I just don't think that this is such an important topic for him right now (I highly doubt that he will be working on medical or aerial software any time soon :v:).
His code lacks modularity in every corner, and the check being outside of the leap function instead of inside is just one of those corners.
[QUOTE=Dienes;42596137](Edit: Wait, why did you remove the first one?)[/QUOTE]
I thought it may be better not comparing his code to this when he's just beginning, even if the problem is somewhat similar.
(I sometimes write things harsher than intended and then edit it out. Trying to be somewhat prudent about written stuff is always a good idea imo.)
I moved the reference into the edit comment though, I guess whoever is interested can just google it.
What I gathered from Tamschi, you want me to establish a range check on the years within the dayCheck() function, change the way I calculate when a leap occurs(which I already did after that guys first comment about it for the sake of doing it. Looked better.) The reason I'm not checking the year within the dayCheck() function is because I check for input validation on the year variable before it is even sent as an argument to dayCheck(). I just wouldn't see the point >.< Thank you for your time, I really do appreciate the comments on how I can improve and I did post with the intent on to pickup on fixing the quirks I have so I can become a better programmer faster; and I just thought it was silly to be rated dumb when I mearly just responded with a reason why I had programmed it that way when it was valid. Yes this is a HW assignment that I have three weeks to do(I ask why when it only takes less than a day to read and do it)
[QUOTE=Proclivitas;42599697]What I gathered from Tamschi, you want me to establish a range check on the years within the dayCheck() function, change the way I calculate when a leap occurs(which I already did after that guys first comment about it for the sake of doing it. Looked better.) The reason I'm not checking the year within the dayCheck() function is because I check for input validation on the year variable before it is even sent as an argument to dayCheck(). I just wouldn't see the point >.< Thank you for your time, I really do appreciate the comments on how I can improve and I did post with the intent on to pickup on fixing the quirks I have so I can become a better programmer faster; and I just thought it was silly to be rated dumb when I mearly just responded with a reason why I had programmed it that way when it was valid. Yes this is a HW assignment that I have three weeks to do(I ask why when it only takes less than a day to read and do it)[/QUOTE]
Well, only if there's still a range restriction in the updated dayCheck().
It's enough to document it properly though, but then you should repeat that documentation for the methods calling your range-restricted method, if they don't do a check.
It could take a while until you learn about exceptions, but they are what I would usually use in a case like this. Basically, they can abort a subroutine no matter how deep it's nested and also give you useful information about the error that occurred. It's also unnecessary to do anything special on the caller side ([editline]edit[/editline] in order to prevent silent errors, exception halt the program by default) if you throw an exception (but good practice to mention that and why each type can occur).
Sorry, you need to Log In to post a reply to this thread.
