Skip to main content
Code Review

Return to Answer

replaced http://programmers.stackexchange.com/ with https://softwareengineering.stackexchange.com/
Source Link
added 1115 characters in body
Source Link
Heslacher
  • 50.9k
  • 5
  • 83
  • 177

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.

added 1134 characters in body
Source Link
Heslacher
  • 50.9k
  • 5
  • 83
  • 177

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.

Source Link
Heslacher
  • 50.9k
  • 5
  • 83
  • 177
Loading
lang-cs

AltStyle によって変換されたページ (->オリジナル) /