You ain't doing any argument validation in the constructor of the CachedFormatter
which can lead to some strange behaviour and ArgumentNullException
's.
If the passed in BigIntegerFormatter
is null
any call to Format()
will
result in an ArgumentNullException
exposing implementation details of your class via the stacktrace.
A passed in maxCacheSize <= 0
will lead to a significant loss in performance because each time the AddToCache()
method is called the added number
and formattedNumber
are removed immediately hence no caching at all.
In addition you should initielize the Dictionary
inside the constructor to make use of the maxCacheSize
argument to set the initial capacity of the Dictionary
which removes the need for the Dictionary
to dynamically increase its size after reaching the initial capazity
which is by default 0
and increased after reaching that value to the next prime number.
So better do
numberCache = new Dictionary<BigInteger, string>(maxCacheSize);
after you have checked the validness of maxCacheSize
.
You ain't doing any argument validation in the constructor of the CachedFormatter
which can lead to some strange behaviour and ArgumentNullException
's.
If the passed in BigIntegerFormatter
is null
any call to Format()
will
result in an ArgumentNullException
exposing implementation details of your class via the stacktrace.
A passed in maxCacheSize <= 0
will lead to a significant loss in performance because each time the AddToCache()
method is called the added number
and formattedNumber
are removed immediately hence no caching at all.
In addition you should initielize the Dictionary
inside the constructor to make use of the maxCacheSize
argument to set the initial capacity of the Dictionary
which removes the need for the Dictionary
to dynamically increase its size after reaching the initial capazity
which is by default 0
and increased after reaching that value to the next prime number.
So better do
numberCache = new Dictionary<BigInteger, string>(maxCacheSize);
after you have checked the validness of maxCacheSize
.
Edit based on your comment
However, I disagree with your opinion on exceptions. I think that functions should do one thing and if they fail they should throw an exception. This also improves readability in my opinion as I think that the CachedFormatter.Format() is much more readable when using the exception than when introducing if statements.
Sure a method should do only one thing, but having a method just for the sake of having one is just overengeniering. What advantage do you gain by having
private string FromCache(BigInteger number) { return numberCache[number]; }
absolutely none IMO. You are adding a method to read a property which is harder to read because you need to jump around in your code.
If that would be some different, more complex method I would agree, but replacing a simple if
statement with a try..catch
to simply wrap a method call which only reads a property is just too much.
See also: Are exceptions as control-flow considered a serious antipattern if so why
The Dictionary<TKey, TValue>
provides the handy TryGetValue()
method which avoids the usage of exceptions to control the flow.
The Dictionary<TKey, TValue>
provides the handy TryGetValue()
method which avoids the usage of exceptions to control the flow.
Edit based on your comment
However, I disagree with your opinion on exceptions. I think that functions should do one thing and if they fail they should throw an exception. This also improves readability in my opinion as I think that the CachedFormatter.Format() is much more readable when using the exception than when introducing if statements.
Sure a method should do only one thing, but having a method just for the sake of having one is just overengeniering. What advantage do you gain by having
private string FromCache(BigInteger number) { return numberCache[number]; }
absolutely none IMO. You are adding a method to read a property which is harder to read because you need to jump around in your code.
If that would be some different, more complex method I would agree, but replacing a simple if
statement with a try..catch
to simply wrap a method call which only reads a property is just too much.
See also: Are exceptions as control-flow considered a serious antipattern if so why
The Dictionary<TKey, TValue>
provides the handy TryGetValue()
method which avoids the usage of exceptions to control the flow.