[Mono-dev] [Patch] AssemblyName ctor

[Kornél Pál]

Some more things in addition to Andreas' comments:

>// Remove white spaces, to mimic .Net behavior
>assemblyName = assemblyName.Replace (" ", "");

This should not be done. First of all white spaces are a set of characters
not just space. All of them (String.WhiteChars, but I don't recommend to use
it directly) has to be removed. But not so agressive you are doing it
currently. White spaces cannot be embedded inside identifiers and values. So
white spaces are stripped only from the beginning and end of each segment
and around the = sign. (This is based on my tests.) I advise to use Trim()
on each segment. Then look form name without = sign. Then Remove() name, do
TrimStart(), look for "=" remove "=", do TrimStart(). You will have a
trimmed value that can be passed to parse functions. It may be easier to do
a common zero lenght check before parsing.

I think there should be no
>} catch {
>  throw new FileLoadException ("The assembly name is invalid.");
>}

I think you should throw exceptions at parsing insted of plain Exceptions
that are converted later to FileLoadException. This makes code look better.

Catching all Exceptions is a bad practicle because it can hide bugs in your
code. Catch only exceptions that you know that may be thrown and can be
ignored in that particular code or has to be converted to a different
exception. I think there is no such case in your code.

Note that ArgumentException is thrown when Culture is invalid but
FileLoadException is thrown when Culture is empty string. This can be
achieved by filtering empty string and using CultureInfo constructor without
catch.

If you use Remove insted of Substring no exception will be thrown for empty
strings and you will be able to throw your own in parse functions. Note that
currently these functions never get empty string.

As a conclusion your patch is good but there are things that should be
modified.

Kornél

----- Original Message -----
From: "Andreas Nahr" <ClassDevelopment at A-SoftTech.com>
To: "Carlos Alberto Cortez" <calberto.cortez at gmail.com>; "Mono Devel"
<mono-devel-list at lists.ximian.com>
Sent: Friday, August 19, 2005 11:07 AM
Subject: Re: [Mono-dev] [Patch] AssemblyName ctor


> Just looked over it briefly:
>
> if (assemblyName == "")
> is better to be value.Length == 0 (used more than one time)
>
> +   assemblyName = assemblyName.Replace (" ", "");
> Is this correct? Shouldn't it be .Trim?
>
> +     if (String.Compare (parts [i], 0, "Version=", 0, 8, true,
> CultureInfo.InvariantCulture) == 0)
> This is most likely incorrect and needs to use an ordinal comparison, no
> InvariantCulture (used more than one time)
>
> Greets
> Andreas
>
> ----- Original Message -----
> From: "Carlos Alberto Cortez" <calberto.cortez at gmail.com>
> To: "Mono Devel" <mono-devel-list at lists.ximian.com>
> Sent: Friday, August 19, 2005 10:49 AM
> Subject: [Mono-dev] [Patch] AssemblyName ctor
>
>
>> Hey,
>>
>> The patch attached implements the new AssemblyName ctor without using
>> internal calls. Could anybody review it?
>>
>> Carlos.
>>
>
>
> --------------------------------------------------------------------------------
>
>
>> _______________________________________________
>> Mono-devel-list mailing list
>> Mono-devel-list at lists.ximian.com
>> http://lists.ximian.com/mailman/listinfo/mono-devel-list
>>
>
> _______________________________________________
> Mono-devel-list mailing list
> Mono-devel-list at lists.ximian.com
> http://lists.ximian.com/mailman/listinfo/mono-devel-list