35

Don't do that. If you log your exceptions at every function, you'll print the same exception over and over and over again. Its only useful the first time, after that its a pain. It will also make your code much harder to read and write. You should log the exception when you would otherwise lose it. At some point, your exception has to be caught and not ...


26

Exceptions are for exceptional cases I'm trying to learn exception handling in C++. This is a very bad example to learn exceptions. They are meant for situations where you cannot know that there is something wrong. For example if you want to use std::vector<int> but you're out of memory: std::vector<int> vec(4000000000LL); // will likely ...


20

Return statement in the finally block is almost always a very bad idea..... The sematics of the finally block are complicated, but, if there is a return statement the try block, or in a catch block, then those will be called, and then the finally block will run, and it will change the return value. IDE's, and the compiler, will complain about this: Code ...


17

Is there anything that can be simplified or made even cleaner? Yes, don't invoke the Java compiler at runtime. From your examples in a comment: DynamicException.Create($"ExtractMethodName(memberName)}NotSupported", ...) From an example on your earlier post (in C#) throw ("SettingNotFoundException", $"Setting {fullName.ToString().QuoteWith("'")} not ...


14

You're throwing System.Exception. Don't do that. If you're going to have to throw an exception for a validation exception, throw a custom ValidationException exception. You haven't shown the code where you catch and handle that exception, but it's going to have to look like this: try { // some code } catch (Exception exception) { // handle the [...


13

You have asked a lot of questions and omitted the most important question. Is the security system I have developed secure against attacks? Absolutely it is not. There is an enormous security problem here. Random is pseudo-random, not crypto-strength random, and that means that it is easy for an attacker to make good guesses about the output of this ...


12

You're using ArrayList which is the weakly typed version of List<T>. Now, since apparently this is done because you're using object[], I would suggest to just strongly type this to List<object> instead. This is assuming your additional information is in fact different unrelated types and not always a string, for example. You're explicitly ...


12

Part One - Pink Glasses On This as a qualifier I like how you're using this in the Throw() method. However this is used as a qualifier in Rethrow() and ThrowFatal(), and the usage or non-usage of this as a qualifier seems inconsistent, especially with private fields being prefixed with an underscore. Member method calls don't need to be qualified like this,...


11

There's quite a few questions about this on stackoverflow but I think my general rule tends to be leave exceptions for exceptional circumstances and booleans for where failure occurances is an accepted/known possible behaviour. So I personally probably prefer your option 2 (interested to see what others think), where if anything the exception is thrown a ...


11

I covered the security problem in another answer; in this answer let's look at more subtle problems. Someone already mentioned that the nested namespaces are unnecessarily verbose, but even with the more compact syntax, things are still not great. We have a class PasswordGenerator.Utility.Service.PasswordService. What is the function of each portion of the ...


11

Surprising This is a gross violation of the Principle of Least Surprise. The only possible reaction I could have seeing this in the wild would be, "What the heck?" This is not a normal way of using exceptions. New developers approaching the code are likely to have a similar reaction. One particular thing that would trip me up and force me to spend time ...


10

You can refactor your code the following way: public int DoSomethingComplicated(ComplicatedObject input) { try { return input.Analyse(); } catch(CustomExceptionTwo ex2) { Log(ex2.Message); return -1; } } throw; re-throws the same exception, so there is no point in such catch block. Alternatively, you can ...


10

At the heart of the solution, there should be a function that converts decimal degrees into a d°mm'ss" string. Assuming that the input is in a reasonable domain (between 0 and 360, for example), the conversion algorithm should be simple and regular. Above all, it should be purely mathematical at this point — you shouldn't be trying to do any string parsing....


9

Everything being initialized in your constructor seems to be a constant. public PasswordService() { this.SAlphaCharacters = "abcdefghijklmnopqrstuwvxyABCDEFGHIJKLMNOPQRSTUWVXYYZ"; this.SNumericCharacters = "0123456789"; this.SNonAlphanumericCharacters = "!$%&()[]{}=?#"; this.UiNumberOfAlphaCharacters = 6; ...


8

Not all compilers support #pragma once #pragma once There are C++ equivalent of most C libraries that put the appropriate interface into the standard namespace. #include <time.h> // Prefer #include <ctime> My pet peeve (OK second after using namespace std;); because everybody thinks they know the rules but get it wrong all the time: ...


8

You're throwing System.Exception - that's bad, you'll want to throw a meaningful exception. See this answer for more details. The main problem with this code, is that you're using exceptions to control the execution flow of your program, which is very expensive - I assume you're going to be catching these exceptions somewhere down the call stack. If a is ...


8

+ (instancetype)screenshot; While it is good to use the instancetype as your return type to allow for subclass, there are two problems using it here. First, this is a class category, not a class, and the only way for a category to be included in a subclass by default is kind of hacky. You just import the file the category is in in the subclass's header. ...


8

Throwing exceptions from a constructor is not only a perfectly legitimate pattern to use. It is also necessary to correctly implement Resource Acquisition Is Initialization (RAII) for some classes (this appears to be the case in your code). And trust me, you really do want RAII :) However, please be aware of the caveat that the destructor of the object will ...


8

private void ValidateAttendance() { //DataService method returns true if the attendance is valid. var validity = _DataService.CheckValidityOfAnAttendance(_View.EmployeeID, _View.Date, _View.ShiftType); //Set the validity of the attendance in a View property. //So that View can stop execution if validity is false. _View.AttendanceValidity =...


8

It seems like you're working harder than you need to. Instead of this: public void warn(Exception cause, String message) { String output = message + "\n" + stackTrace(cause); log.warn(output); } I think you could do this, and no need for the stackTrace method: public void warn(Exception cause, String message) { log.error(cause.getMessage() + " ...


8

I've never seen this: namespace PasswordGenerator { namespace Utility { namespace Service { I'm guessing what you intended was this: namespace PasswordGenerator.Utility.Service { However if you look at some of the framework's namespaces... using System.Collections.Generic; using System.Threading.Tasks; I think PasswordGenerator.Utilities.Services would ...


8

private const string Space = " "; There's little point in storing this in a variable: it's never going to change anyway and you're not clarifying any of the intent. I'm fully aware that is a space and { is a left curly bracket. Using dynamic is a very, very expensive action. By defaulting to it everywhere instead of using an intermediate type you're ...


7

Close a ResultSet/Resource If you are using java 7 then I would recommend to use try with resources it will take care of closing ResultSet for you and make your code a little cleaner. public void updateReplicationStatus(BeanReplicationTask taskBean, String strStatus) throws Exception { String strSelect = this.createReplicationSelectStatement(taskBean);...


7

Refactor into a method: var name = this.GetVariable("Name"); var dueDate= this.GetVariable("Due Date"); . .//more variables . var Title= this.GetVariable("Title"); private string GetVariable(string name) { try { return item[name].ToString(); } catch (Exception ex) { ...


7

Especially if you expect these exceptions to occur (relatively) often, I think that using exceptions is a bad idea here. Exceptions should not be used for regular flow of control, this applies to Java as well. Creating and throwing exceptions is among the most costly operations there is in many programming languages. One comment especially that caught my ...


7

You can re-throw an exception: int main() { ... try { ... try { foo(); } catch(std::exception const& e) { std::cout << "Fatal error: " << e.what() << std::endl; throw; // re-throw the exception. // or you could throw a different ...


7

Lets kill that giant nest of code. When an Exception is thrown, it interrupts the flow of the program. The code does not continue to execute beyond that point, it is like an early return in that regard. This means we do not need to nest our next statement in the Else block, because if an exception was thrown the program has returned. You should not be ...


7

Some small stuff: sgl_detail_current_exception is initialize to 0 as it is in global memory. Given the enumerated list starts with -1, then sgl_logic_error == 0: sgl_exception_t sgl_detail_current_exception; // is effectively sgl_exception_t sgl_detail_current_exception = sgl_logic_error; Recommend adding an enumerated value of sgl_none = 0 to ...


7

Yep, go for it. Adding more detail to error messages is helpful. Also, depending on which style you prefer you could avoid explicitly checking the count by catching the exception thrown by Single(). try { return objects.Single(); } catch (InvalidOperationException ex) { throw new InvalidOperationException( "%Collection% should contain one %...


7

Lots of bad practices exhibited here… First of all, you probably shouldn't be using exception handling here at all, as Incomputable already pointed out in a comment. Exceptions should be for truly exceptional situations. If you expect a zero input as a possibility and are going to explicitly handle it, then I would argue that it is not exceptional at all. ...


Only top voted, non community-wiki answers of a minimum length are eligible