0
\$\begingroup\$

I'm building a game. It is going well at the moment, but I'm not quite happy with my code because is not working the way I want to, due my low knowledge of C#. I was wondering if you guys could help me out to improve it by giving me some ideas and tips.

This is how I'm checking if the user has completed the levels.

public partial class MainMenu : PhoneApplicationPage
{
public  MainMenu()
{
    InitializeComponent();
    ReadStar1();
    ReadStar2();


}

private void btnbatland_Click(object sender, RoutedEventArgs e)
{
    NavigationService.Navigate(new Uri("/1star/MainPage.xaml", UriKind.Relative));
}


private async  Task ReadStar1()
{
    StorageFolder local = Windows.Storage.ApplicationData.Current.LocalFolder;

    if (local != null)
    {
        var dataFolder = await local.GetFolderAsync("level");
        var file = await dataFolder.OpenStreamForReadAsync("star1.txt");
        string readtxt;
        using (StreamReader streamReader = new StreamReader(file))
        {
            readtxt = streamReader.ReadToEnd();
        }

        if (readtxt == "star1")
            star2_lock.Visibility = Visibility.Collapsed;
           star1_1.Visibility = Visibility.Collapsed;
    }
}

private async Task ReadStar2()
{
    StorageFolder local = Windows.Storage.ApplicationData.Current.LocalFolder;

    if (local != null)
    {
        var dataFolder = await local.GetFolderAsync("level");
        var file = await dataFolder.OpenStreamForReadAsync("star2.txt");
        string readtxt;
        using (StreamReader streamReader = new StreamReader(file))
        {
            readtxt = streamReader.ReadToEnd();
        }

        if (readtxt == "star2")
            star3_lock.Visibility = Visibility.Collapsed;

    }
}

Sometimes it takes 1-4 seconds to load the MainMenu, then after 1-3 to load the function ReadStar1() and ReadStar2(). What could I do to improve its speed, so that the users don't even notice it has changed?

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$
  1. In your ReadStar1 method I assume your forgot the brackets around the if (readtxt == "star1") block? Right now you always collapse visibility of star1_1.

  2. Your ReadStar1 and ReadStar2 methods share lot of code and should be refactored into one method. The only real difference is which file you read and what you do if you read a specific string. Pass in what you want to read and remove the concern of what should happen if you do read it by returning a bool indicating whether the string was found (the purpose of the method should be to determine if a certain file contains a certain string):

    private async Task<bool> StarExists(string star)
    {
        StorageFolder local = Windows.Storage.ApplicationData.Current.LocalFolder;
    
        if (local != null)
        {
            var dataFolder = await local.GetFolderAsync("level");
            var file = await dataFolder.OpenStreamForReadAsync(star + ".txt");
            string readtxt;
            using (StreamReader streamReader = new StreamReader(file))
            {
                readtxt = streamReader.ReadToEnd();
            }
    
            return readtxt == star;
        }
        return false;
    }
    
    private async void CheckLevels()
    {
        var hasStar1 = await StarExists("star1");
        var hasStar2 = await StarExists("star2");
    
        if (hasStar1)
        {
            star2_lock.Visibility = Visibility.Collapsed;
            star1_1.Visibility = Visibility.Collapsed;
        }
    
        if (hasStar2)
        {
            star3_lock.Visibility = Visibility.Collapsed;
        }         
    }
    

    Then your constructor becomes:

    public MainMenu()
    {
        InitializeComponent();
        CheckLevels();
    }
    
  3. If you use strings like "star1" and "star2" in multiple places then consider making them public const fields of a class rather than sprinkling string literals around everywhere. If you ever decide to change them you will have to change a lot less code.

\$\endgroup\$
5
  • \$\begingroup\$ @JeffersonFilho: I use bool to indicate that the text matches the required star. \$\endgroup\$
    – ChrisWue
    Oct 13, 2013 at 10:40
  • \$\begingroup\$ thanks! but now when I run the app it crashed and say that the error is on the star1.Wait();. Btw sorry to delete the other comment I wanted to edit it to say that the app was crashing but I couldn't! \$\endgroup\$ Oct 13, 2013 at 11:04
  • \$\begingroup\$ @JeffersonFilho: Can't see any reason what the problem might be with the code given. You can try asking on StackOverflow (remember to provide the details of the error). \$\endgroup\$
    – ChrisWue
    Oct 13, 2013 at 18:16
  • \$\begingroup\$ @JeffersonFilho: That being said it's probably nicer to not wait synchronously for the tasks, I updated the code. Makes it a bit cleaner. \$\endgroup\$
    – ChrisWue
    Oct 14, 2013 at 0:38
  • \$\begingroup\$ still not working unfortunately. \$\endgroup\$ Oct 14, 2013 at 17:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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