3
\$\begingroup\$

I have a map. It can be zoomed-in and out. At first, I used one large image. Scrolling the mouse wheel defined the desired zoom level. Every scrolling created a new instance of the image in the desired dimensions. This approach was horribly slow.

So I gave up the possibility of zooming into any level, and created a list of predefined available zoom levels, ranging from 35% to 100% with 5% intervals, and then up to 300% with 50% intervals. For each zoom level, I have created a copy of the original image with the appropriate dimensions. The image name corresponds to its dimensions, so a 35% image is called "35.jpg". I added the images to the project's resources in a dedicated folder. Visual Studio automatically inserts an underline before the object's name because it starts with a number, so the object that represents the file is called "_35". So now, after the desired zoom level has been determined by the mouse wheel, the corresponding image is loaded from the resources. Performance has improved significantly.

Some of the application's components need a list of all available zoom levels. For example, for a ComboBox that allows the user to select a specific zoom level, rather than reach it through the mouse wheel. Therefore, when loading the form, a List<float> is initialized with all available zoom dimensions, so a 35% zoom is saved as 0.35 (a floating point representation is required by most components that uses the list) . The initialization is done by importing the names of the objects from the resources.

Here is the method that extracts file names and parse them into float. I feel it is not well written but I cannot point to a specific problem. I would love to hear your opinion. Thanks!

private static void InitializeZoomFactorsList()
{
    ResourceSet images = Properties.Resources.ResourceManager.GetResourceSet(CultureInfo.CurrentUICulture, true, true);
    List<string> names = images.Cast<DictionaryEntry>().Select(item => item.Key.ToString()).ToList();
    List<float> factors = names.ConvertAll<float>(delegate(string i) { return float.Parse(i.Substring(1, i.Length - 1)) / 100; });
    factors.Sort(); // because the list was strings, 300 came before 90, so after the parsing, I sort it by the value.
    zoomFactorsList = factors;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Are you sure this is working code? There are not enough }. A copy/paste error or is this just pseudocode? \$\endgroup\$ – t3chb0t Jun 27 '17 at 9:37
1
\$\begingroup\$

First of all let's try to make it more concise:

var zoomLevels = Properties.Resources.ResourceManager
    .GetResourceSet(CultureInfo.CurrentUICulture, true, true);
    .Cast<DictionaryEntry>()
    .Select(x => ParseZoomLevelAndNormalize(x.Key))
    .OrderBy(x => x);

Note that you do not need to create multiple temporary lists (not that you will ever note this waste) using ToList(). Also note that parsing code has been moved to a separate function. Let's talk about that...

static float ParseZoomLevelAndNormalize(object resourceName)
{
    string zoomLevel = Convert.ToString(resourceName).Substring(1);

    return Single.Parse(zoomLevel,
        NumberStyles.Number, CultureInfo.InvariantCulture) / 100;
}

If you do not thin it's clear enough then you should introduce separate functions to explain what it is doing:

var precalculatedMaps = new PrecalculatedMaps();
var zoomLevels = precalculatedMaps.ZoomLevels.Select(x => x / 100);

Where PrecalculatedMaps hides the implementation detail (now it's a resource file, in future might be a folder in file system). ZoomLevels property simply return a standard IEnumerable<float> of available zoom levels (in range [0...100]). Proof of concept:

sealed class PrecalculatedMaps
{
    public IEnumerable<float> ZoomLevels
    {
        get
        {
            if (_zoomLevels == null)
                _zoomLevels = ReadAvailableZoomLevels().OrderBy(x => x);


            return _zoomLevels;
        }
    }

    private IEnumerable<float> _zoomLevels;

    private static IEnumerable<float> ReadAvailableZoomLevels()
    {
        return Properties.Resources.ResourceManager
            .GetResourceSet(CultureInfo.CurrentUICulture, true, true);
            .Cast<DictionaryEntry>()
            .Select(x => ParseZoomLevel(x.Key));
    }

    private static float ParseZoomLevel(object resourceName)
    {
        string zoomLevel = Convert.ToString(resourceName).Substring(1);

        return Single.Parse(zoomLevel,
            NumberStyles.Number, CultureInfo.InvariantCulture);
    }
}

Note that here I made few assumptions which you can't ignore:

  • Resource names are valid numbers (otherwise Single.Parse() will throw an exception).
  • Numbers are formatted using en-US locale. In your code you're parsing using CultureInfo.CurrentCulture (the default if you do not specify any IFormatProvider). For integer numbers you will probably never have trouble but it's better to be sure, right?
  • There is always a leading underscore (that might be VS default but you can change resource name by hand). Do you want to handle the case? Change .Substring(1) with .TrimStart('_').
  • If you have only natural numbers then you can use NumberStyles.Integer instead of NumberStyles.Number (which allows decimals).

OK, that was about your actual code but let me say few words about the algorithm. Zooming an image is not an incredibly slow operation, I don't know how you did it and which technology you use for UI then I can't give you better ideas but:

  • You limit zooming to integer factors.
  • You can cache those generated images.
  • You can generate images only at special thresholds (for example 200%, 50%, 25%) leaving the exact scale in/out duty to UI controls (which are pretty good at this). This might not be applicable if your zoom algorithm is REALLY slow and undoubtedly better than canonical decimation/interpolation algorithms already in use.
  • You can debounce zoom requests to do not generate hundreds images when user is zooming from 100% to 10% and he's not interested in the intermediate images (leaving the duty to scale things to UI controls).
\$\endgroup\$
  • \$\begingroup\$ This answer should get one thousand upvotes. \$\endgroup\$ – Sipo Jun 28 '17 at 6:51
  • 1
    \$\begingroup\$ @Sipo thank you but it's not that special! Especially second part is more "Stack Overflow style" than "Code Review style". If you post a new question with code you're using for zooming the image then we may TRY to polish it little bit, maybe it will work in real-time without any trick with pre-computed imaged. \$\endgroup\$ – Adriano Repetti Jun 28 '17 at 7:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.