1

Closed

Feedback in Forum

description

Hi James,
 
Of course, thousands of lines of code will have bugs, but I do hope you look past the trivial (and hastily written) first few hundred lines. As for your comments, they are great -- here are my responses:
 
 
Well... Having just downloaded the code... er... um... it's not very good.
 
Ow ;-)
 
Starting right at the beginning.
PublicDomainVersion & PublicDomainStrongName: The values defined here are
"fake". The real values are defined elsewhere (in AssemblyInfo.cs). The fact
that they may, for the moment, be the same, is coincidental. It is a virtual
guarantee that we wil eventually update them in one spot and not the other.
Hence, we need to make these refer to the real values defined in
AssemblyInfo.cs.
 
If you look at AssemblyInfo.cs in the version control, it uses these constants in the version declaration, so whether they are in the file itself (for external reference) or AssemblyInfo.cs makes no difference. In this case, I think this method is better since I don't need to depend on having an AssemblyInfo.cs (in the common case that I will simply be adding the file to an existing project). The StrongName becomes pretty useless in this case, but a version might still be useful in knowing what version of the code that I am using.
 
public readonly string PublicDomainVersion =
Assembly.GetCallingAssembly().GetName().Version.ToString();
 
public readonly string PublicDomainStrongName
=Assembly.GetCallingAssembly().GetName().FullName;
(note that they had to be changed from "const" to "realonly".)
 
Again, the issue is that a design goal of this project is that I should be able to add PublicDomain.cs to an existing project. In that case, Assembly.GetCallingAssembly().GetName() would give me something completely different than PublicDomain metadata.
 
Then we have:
public const int BytesInAKilobyte = BitsInAByte * 1000;
 
er..um.. no, that's not "Bytes in a Kilobyte" to "BITS in a Kilobyte", or it
would be, except when in the computer realm "kilo" means 1024 (2^10) and not
1000 (10^2). BOth these problem affect every other BytesInA----- constant.
 
Sorry, dumb mistake, I added these constants for no specific purpose a late night a few nights ago when I was writing the Logging namespace, and I didn't really look at them. I'll fix these in the next release. Thanks.
 
Then we've got EarthRadiusStatuteMiles et al. I've no problem with it per se,
except that they seem to be remarkable obscure data points to be including in
here.
 
Agreed - very obscure, but I was using it somewhere in my code, and I couldn't see why I shouldn't include them. I should reference some kind of data source for getting this information though.
 
DirectorySeparator - Already defined in the CLR as Path.DirectorySeparatorChar
 
I didn't know that, thanks.
 
Move to the code:

I'm unclear why writing
byte[] data = EncodingUtilities.GetBytesFromString(str);
is better than that standard CLR method :
byte[] data = Encoding.Unicode.GetBytes(str);
 
Not all .NET programmers are aware that UTF-16 is the default internal storage of strings, which is represented by the "Unicode" encoding. I don't know about performance data, but I think a logical encapsulation for getting bytes, instead of worrying about the encoding (some developers don't even understand the concept of encodings) seems like a good abstraction.
 
RandomString appears to be the first method defined with an actual algorithm,
so we'll give it a thorough study:
 
This was code adapted from some other public code, so I hope you don't take this as an example of my programming capabilities. Alas, your analysis is right on...
 
public static string RandomString(int size, bool lowerCase)
{
// We know the size. Why not use it?
StringBuilder builder = new StringBuilder();
// We go to trouble to do the same as the default?
Random random = new Random(unchecked((int)DateTime.UtcNow.Ticks));
// if ch is only used inside the loop, why declare it outside?
char ch;
for (int i = 0; i < size; i++)
{
// if we already know that we may want lowercase only, why are we building a
string that may have upper case?
ch = Convert.ToChar(Convert.ToInt32(Math.Floor(26 * random.NextDouble() + 65)));
builder.Append(ch);
}
// Why fix what's wrong, when we could have built it right to start?
if (lowerCase)
{
return builder.ToString().ToLower();
}
return builder.ToString();
}
 
Answering those questions, we'd get the simplier:
public static string RandomString(int size, bool lowerCase)
{
StringBuilder builder = new StringBuilder(size);
Random random = new Random();
int low = 65; // 'A'
int high = 91; // 'Z' +1
if (lowerCase)
{
low = 97; // 'a';
high = 123; // 'z' +1
}
for (int i = 0; i < size; i++)
{
char ch = Convert.ToChar(random.Next(low, high));
builder.Append(ch);
}
return builder.ToString();
}
 
I will update the code, thanks.
 
However, both versions have the problem that they re-seed the RNG to the clock > at the start. Call either function twice quickly in succession will return the > same random string.
It would be better for the containing class to have a single static reused
Random object.
 
I will update the code, thanks.
 
I could go on, but I think that's enough for now.
 
Please do go on, this is great feedback.
 
Thanks! I'll add you to the developer list in case you want to be a part of the project.
 
Kevin
Closed Jan 18, 2007 at 1:51 AM by schizoidboy

comments

schizoidboy wrote Jan 18, 2007 at 2:17 AM

This work item has been fixed in version 0.0.2.23



Hi James,

Answers to a few of your questions:

The equatorial radius of the Earth is used in LatitudeLongitudePoint.DistanceBetween to find the distance between two points.

I've moved GetBytesFromString into StringUtilities so that it is more obvious that is a logical way to get bytes from a String.

The whole bit/byte code mess will be changed to:

/// <summary>/// The number of bits in a byte./// </summary>public const int BitsInAByte = 2 ^ 3;

/// <summary>/// The number of bytes in 1KB/// </summary>public const int BytesInAKilobyte = 2 ^ 10;

/// <summary>/// The number of bits in 1KB/// </summary>public const int BitsInAKilobyte = BitsInAByte * BytesInAKilobyte;

/// <summary>/// The number of bytes in 1MB/// </summary>public const int BytesInAMegabyte = 2 ^ 20;

/// <summary>/// The number of bits in 1MB/// </summary>public const int BitsInAMegabyte = BitsInAByte * BytesInAMegabyte;

/// <summary>/// The number of bytes in 1GB/// </summary>public const long BytesInAGigabyte = 2 ^ 30;

/// <summary>/// The number of bits in 1GB/// </summary>public const long BitsInAGigabyte = BitsInAByte * BytesInAGigabyte;

/// <summary>/// The number of bytes in 1TB/// </summary>public const long BytesInATerabyte = 2 ^ 40;

/// <summary>/// The number of bits in 1TB/// </summary>public const long BitsInATerabyte = BitsInAByte * BytesInATerabyte;

/// <summary>/// The number of bytes in 1PB/// </summary>public const long BytesInAPetabyte = 2 ^ 50;

/// <summary>/// The number of bits in 1PB/// </summary>public const long BitsInAPetabyte = BitsInAByte * BytesInAPetabyte;

Thanks again for your comments, I hope you look more at the code, and I hope you find at least some of the code useful!

Kevin