2
\$\begingroup\$

I have been working on my first ever C# project and I'd love some feedback of the any and all aspects variety. I am building a game called Fleet Command. In the MainWindow I use a ContentControl to use various UserControls based on user interaction. The screen navigation is pretty simple so for the most part I am leaving it out. But for context I would like to give you this:

MainWindow.xaml

<Window x:Class="FleetCommand.MainWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
        xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
        mc:Ignorable="d"
        Title="Fleet Command" Height="700" Width="1200" Background="DarkSlateGray">
    <Grid>
        <ContentControl x:Name="MainScreenContent"/>
    </Grid>
</Window>

and

MainWindow.xaml.cs

using System.Windows;
using System.Windows.Controls;

namespace FleetCommand
{
    /// <summary>
    /// Struct to encapsulate initialization state from Custom game settings.
    /// Used to simplify passing many parameters to initialization function and minimize errors.
    /// </summary>
    public struct InitialState
    {
        public int NumberPlayerCharacters;
        public int NumberNonPlayerCharacters;
        public int StartingOil;
        public int StartingCash;
        public int StartingResearch;
        public Difficulty Difficulty;
    }

    public enum Difficulty
    {
        Easy,
        Normal,
        Hard
    }

    /// <summary>
    /// Interaction logic for MainWindow.xaml
    /// </summary>
    public partial class MainWindow : Window
    {
        public MainWindow()
        {
            InitializeComponent();
            SetScreen(SplashScreen);
        }

        public void ReturnToSplash() => SetScreen(SplashScreen);
        public void OpenNewGameScreen() => SetScreen(NewGameScreen);
        public void OpenLoadGameScreen()
        {
            LoadGameScreen.Set();
            SetScreen(LoadGameScreen);
        }
        public void OpenCustomScreen() => SetScreen(CustomGameScreen);
        public void OpenWorldMap() => GameScreen.OpenWorldMap();
        public void OpenCity(string CityName) => GameScreen.OpenCity(CityName);

        public void StartCampaign()
        {
            GameData.StartGame();
            SetScreen(GameScreen);
        }
        public void StartCustomGame(InitialState state)
        {
            GameData.StartGame(state);
            SetScreen(GameScreen);
        }
        public void StartLoadedGame(LoadFile FileSlot)
        {
            GameData.StartGame(FileSlot);
            SetScreen(GameScreen);
        }

        public void Reset()
        {
            CustomGameScreen.Reset();
            GameScreen.Reset();
            GameData.Reset();
        }

        public void Save(LoadFile SaveSlot) => GameData.Save(SaveSlot);

        private void SetScreen(UserControl screen) => MainScreenContent.Content = screen;

        private static SplashScreen SplashScreen { get; } = new SplashScreen();
        private static NewGameScreen NewGameScreen { get; } = new NewGameScreen();
        private static LoadGameScreen LoadGameScreen { get; } = new LoadGameScreen();
        private static CustomGameScreen CustomGameScreen { get; } = new CustomGameScreen();
        private static GameScreen GameScreen { get; } = new GameScreen();

        private static GameData GameData { get; set; } = new GameData();
    }
}

The various UserControls call the public methods the MainWindow provides and then it acts accordingly.

However, then I come to the GameScreen. This is where I need a review. GameScreen also provides a ContentControl that can be swapped out with various UserControls in response to users. I'm not unhappy with that and don't think I need to change it, but I don't know, do I? I give the GameScreen all of the buttons it will need in it's various states. Here is. . .

GameScreen.xaml

<UserControl x:Class="FleetCommand.GameScreen"
             xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
             xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
             xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" 
             xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
             mc:Ignorable="d">
    <Grid>
        <Grid.ColumnDefinitions>
            <ColumnDefinition Width="20"/>
            <ColumnDefinition Width="*"/>
            <ColumnDefinition Width="*"/>
            <ColumnDefinition Width="*"/>
            <ColumnDefinition Width="*"/>
            <ColumnDefinition Width="*"/>
            <ColumnDefinition Width="*"/>
            <ColumnDefinition Width="*"/>
            <ColumnDefinition Width="*"/>
            <ColumnDefinition Width="20"/>
        </Grid.ColumnDefinitions>
        <Grid.RowDefinitions>
            <RowDefinition Height="*"/>
            <RowDefinition Height="*"/>
            <RowDefinition Height="*"/>
            <RowDefinition Height="*"/>
            <RowDefinition Height="*"/>
            <RowDefinition Height="*"/>
            <RowDefinition Height="*"/>
            <RowDefinition Height="*"/>
            <RowDefinition Height="*"/>
            <RowDefinition Height="*"/>
            <RowDefinition Height="*"/>
            <RowDefinition Height="*"/>
            <RowDefinition Height="*"/>
            <RowDefinition Height="*"/>
            <RowDefinition Height="*"/>
            <RowDefinition Height="*"/>
        </Grid.RowDefinitions>
        <ContentControl Grid.Column="1" Grid.ColumnSpan="8" Grid.Row="1" Grid.RowSpan="14" x:Name="GameField"/>

        <Rectangle Grid.Column="0" Grid.Row="0" Grid.RowSpan="16" Width="auto" Height="auto" Fill="SteelBlue"/>
        <Rectangle Grid.Column="9" Grid.Row="0" Grid.RowSpan="16" Width="auto" Height="auto" Fill="SteelBlue"/>

        <Button Grid.Column="1" Grid.Row="0" Content="Menu" Click="Click_Button" x:Name="menu" VerticalContentAlignment="Top" BorderThickness="0"/>
        <Button Grid.Column="1" Grid.Row="1" Content="Save" Click="Click_Button" x:Name="save" Visibility="Collapsed"/>
        <Button Grid.Column="1" Grid.Row="2" Content="Exit" Click="Click_Button" x:Name="exit" Visibility="Collapsed"/>
        <Button Grid.Column="1" Grid.ColumnSpan="2" Grid.Row="15" Content="Dashboard" Click="Click_Button" x:Name="dashboard"/>
        <Button Grid.Column="3" Grid.ColumnSpan="2" Grid.Row="15" Content="Research" Click="Click_Button" x:Name="research"/>
        <Button Grid.Column="5" Grid.ColumnSpan="2" Grid.Row="15" Content="Fleet" Click="Click_Button" x:Name="fleet"/>
        <Button Grid.Column="7" Grid.ColumnSpan="2" Grid.Row="15" Content="End Turn" Click="Click_Button" x:Name="endTurn"/>

        <Button Grid.ColumnSpan="2" Grid.Row="15" Content="Unit Design" Click="Click_Button" x:Name="unit" Visibility="Collapsed"/>
        <Button Grid.Column="7" Grid.ColumnSpan="2" Grid.Row="15" Content="Map" Click="Click_Button" x:Name="world" Visibility="Collapsed"/>
        <Button Grid.Column="3" Grid.ColumnSpan="2" Grid.Row="15" Content="City Map" Click="Click_Button" x:Name="cityMap" Visibility="Collapsed"/>
        <Button Grid.Column="5" Grid.ColumnSpan="2" Grid.Row="15" Content="Upgrade City" Click="Click_Button" x:Name="cityUpgrade" Visibility="Collapsed"/>
        <Button Grid.Column="1" Grid.ColumnSpan="2" Grid.Row="15" Click="Click_Button" x:Name="cityDashboard" Visibility="Collapsed"/>

        <TextBlock Grid.Column="2" Grid.ColumnSpan="2" Grid.Row="0" x:Name="playerName" Text="Placeholder Name" Background="SteelBlue" FontSize="36"/>
        <TextBlock Grid.Column="4" Grid.ColumnSpan="2" Grid.Row="0" x:Name="playerOil" Text="10000" Background="SteelBlue" FontSize="36"/>
        <TextBlock Grid.Column="6" Grid.ColumnSpan="2" Grid.Row="0" x:Name="playerCash" Text="1000" Background="SteelBlue" FontSize="36"/>
        <TextBlock Grid.Column="8" Grid.Row="0" x:Name="playerResearch" Text="10" Background="SteelBlue" FontSize="36"/>
    </Grid>
</UserControl>

I collapse visibility on buttons not in use. I'm still not sure there is anything wrong with this approach but now I am starting to get into a bit of code duplication and I'm not sure of a way around it. As seen in. . .

GameScreen.xaml.cs

using System.Windows;
using System.Windows.Controls;

namespace FleetCommand
{
    /// <summary>
    /// Interaction logic for GameScreen.xaml
    /// </summary>
    public partial class GameScreen : UserControl
    {
        public GameScreen()
        {
            InitializeComponent();
            SetGameField(OuterMap);
        }

        public void Reset()
        {
            SetGameField(OuterMap);
            OuterMap.Reset();
            CloseMenu();
        }

        public void OpenWorldMap() => OpenWorld();
        public void OpenCity(string CityName) => OpenCityManager(CityName);

        private void SetGameField(UserControl field) => GameField.Content = field;

        private void Click_Button(object sender, RoutedEventArgs e)
        {
            Button button = sender as Button;
            switch (button.Name)
            {
                case "menu":
                    Menu();
                    break;
                case "save":
                    Save();
                    break;
                case "exit":
                    Exit();
                    break;
                case "dashboard":
                    OpenDashboard();
                    break;
                case "research":
                    OpenResearch();
                    break;
                case "fleet":
                    OpenFleet();
                    break;
                case "unit":
                    OpenUnitDesign();
                    break;
                case "cityMap":
                    OpenCityMap();
                    break;
                case "cityUpgrade":
                    OpenCityUpgrade();
                    break;
                case "cityDashboard":
                    OpenCity(NameOfCity);
                    break;
                case "world":
                    OpenWorld();
                    break;
                case "endTurn":
                    EndTurn();
                    break;
                default:
                    break;
            }
        }

        private void Menu()
        {
            if (IsMenuOpen)
            {
                CloseMenu();
            }
            else
            {
                OpenMenu();
            }
        }

        private void Save()
        {
            SaveWindow saveWindow = new SaveWindow();
            saveWindow.ShowDialog();
        }

        private void Exit()
        {
            bool? proceed = true;
            Confirm confirm = new Confirm();
            proceed = confirm.ShowDialog();
            if (proceed != null && (bool)proceed)
            {
                MainWindow window = Application.Current.MainWindow as MainWindow;
                window.ReturnToSplash();
                window.Reset();
            }
        }

        private void OpenWorld()
        {
            dashboard.Visibility = Visibility.Visible;
            research.Visibility = Visibility.Visible;
            fleet.Visibility = Visibility.Visible;
            endTurn.Visibility = Visibility.Visible;

            unit.Visibility = Visibility.Collapsed;
            world.Visibility = Visibility.Collapsed;
            cityMap.Visibility = Visibility.Collapsed;
            cityUpgrade.Visibility = Visibility.Collapsed;
            cityDashboard.Visibility = Visibility.Collapsed;

            SetGameField(OuterMap);
            OuterMap.OpenWorldMap();
        }

        private void OpenDashboard()
        {
            Grid.SetColumn(unit, 1);
            unit.Visibility = Visibility.Visible;
            research.Visibility = Visibility.Visible;
            fleet.Visibility = Visibility.Visible;
            world.Visibility = Visibility.Visible;

            dashboard.Visibility = Visibility.Collapsed;
            endTurn.Visibility = Visibility.Collapsed;
            cityMap.Visibility = Visibility.Collapsed;
            cityUpgrade.Visibility = Visibility.Collapsed;
            cityDashboard.Visibility = Visibility.Collapsed;

            SetGameField(Dashboard);
        }

        private void OpenResearch()
        {
            Grid.SetColumn(unit, 3);
            dashboard.Visibility = Visibility.Visible;
            unit.Visibility = Visibility.Visible;
            fleet.Visibility = Visibility.Visible;
            world.Visibility = Visibility.Visible;

            research.Visibility = Visibility.Collapsed;
            endTurn.Visibility = Visibility.Collapsed;
            cityMap.Visibility = Visibility.Collapsed;
            cityUpgrade.Visibility = Visibility.Collapsed;
            cityDashboard.Visibility = Visibility.Collapsed;

            SetGameField(Research);
        }

        private void OpenFleet()
        {
            Grid.SetColumn(unit, 5);
            dashboard.Visibility = Visibility.Visible;
            research.Visibility = Visibility.Visible;
            unit.Visibility = Visibility.Visible;
            world.Visibility = Visibility.Visible;

            fleet.Visibility = Visibility.Visible;
            endTurn.Visibility = Visibility.Visible;
            cityMap.Visibility = Visibility.Collapsed;
            cityUpgrade.Visibility = Visibility.Collapsed;
            cityDashboard.Visibility = Visibility.Collapsed;

            SetGameField(Fleet);
        }

        private void OpenCityManager(string CityName)
        {
            dashboard.Visibility = Visibility.Visible;
            cityMap.Visibility = Visibility.Visible;
            cityUpgrade.Visibility = Visibility.Visible;
            world.Visibility = Visibility.Visible;

            research.Visibility = Visibility.Collapsed;
            unit.Visibility = Visibility.Collapsed;
            fleet.Visibility = Visibility.Collapsed;
            endTurn.Visibility = Visibility.Collapsed;
            cityDashboard.Visibility = Visibility.Collapsed;

            cityDashboard.Content = NameOfCity = CityName;
            SetGameField(CityDashboard);
        }

        private void OpenCityMap()
        {
            Grid.SetColumn(unit, 3);
            cityDashboard.Visibility = Visibility.Visible;
            unit.Visibility = Visibility.Visible;
            cityUpgrade.Visibility = Visibility.Visible;
            world.Visibility = Visibility.Visible;

            dashboard.Visibility = Visibility.Collapsed;
            cityMap.Visibility = Visibility.Collapsed;
            research.Visibility = Visibility.Collapsed;
            fleet.Visibility = Visibility.Collapsed;
            endTurn.Visibility = Visibility.Collapsed;

            SetGameField(OuterMap);
            OuterMap.OpenCityMap(NameOfCity);
        }

        private void OpenUnitDesign()
        {
            dashboard.Visibility = Visibility.Visible;
            research.Visibility = Visibility.Visible;
            fleet.Visibility = Visibility.Visible;
            world.Visibility = Visibility.Visible;

            cityDashboard.Visibility = Visibility.Collapsed;
            unit.Visibility = Visibility.Collapsed;
            cityUpgrade.Visibility = Visibility.Collapsed;
            cityMap.Visibility = Visibility.Collapsed;
            endTurn.Visibility = Visibility.Collapsed;

            SetGameField(UnitDesign);
        }

        private void OpenCityUpgrade()
        {
            Grid.SetColumn(unit, 5);
            cityDashboard.Visibility = Visibility.Visible;
            cityMap.Visibility = Visibility.Visible;
            unit.Visibility = Visibility.Visible;
            world.Visibility = Visibility.Visible;

            dashboard.Visibility = Visibility.Collapsed;
            research.Visibility = Visibility.Collapsed;
            fleet.Visibility = Visibility.Collapsed;
            cityUpgrade.Visibility = Visibility.Collapsed;
            endTurn.Visibility = Visibility.Collapsed;

            SetGameField(CityUpgrade);
        }

        private void EndTurn()
        {
            // TODO: End Turn via Data
        }

        private void OpenMenu()
        {
            save.Visibility = Visibility.Visible;
            exit.Visibility = Visibility.Visible;
            IsMenuOpen = true;
        }

        private void CloseMenu()
        {
            save.Visibility = Visibility.Hidden;
            exit.Visibility = Visibility.Hidden;
            IsMenuOpen = false;
        }

        private string NameOfCity { get; set; }
        private string PlayerName { get; set; }
        private string PlayerOil { get; set; }
        private string PlayerCash { get; set; }
        private string PlayerResearch { get; set; }
        private bool IsMenuOpen { get; set; } = false;

        private static OuterMap OuterMap { get; } = new OuterMap();
        private static Dashboard Dashboard { get; } = new Dashboard();
        private static CityDashboard CityDashboard { get; } = new CityDashboard();
        private static Research Research { get; } = new Research();
        private static Fleet Fleet { get; } = new Fleet();
        private static UnitDesign UnitDesign { get; } = new UnitDesign();
        private static CityUpgrade CityUpgrade { get; } = new CityUpgrade();
    }
}

My biggest issue with this approach is manually handling the visibility of each button as I navigate through each subcontent of the GameScreen. I had considered not needing the GameScreen at all (after all each UserControl could simply be the content of the MainWindow, but they all share so many elements that I didn't like that idea. I could move just the <Button>s but that wouldn't actually remove the repetitiveness it would just spread it out across multiple files. I think I would prefer to keep it together in one place. It will make making changes easier I believe.

I can add more of the files if needed but the GameScreen is the big and ugly one I'm worried about. I am also interested in anything else you might see.

\$\endgroup\$
2
\$\begingroup\$

There is one thing that struck me most namely that you use event handlers for the buttons:

Click="Click_Button"

The WPF or rather MVVM way would be to use commands to handle these actions. This way you could get rid of all the ugly switch with case "menu": etc.

public partial class GameScreen : UserControl

The UserControl would then become a GameScreenModel that you would data-bind to the window.

Also all your visibility assignments like dashboard.Visibility = Visibility.Visible; could be driven much easier and automatically by binding them to dependency properties on the GameScreenModel. It might also be necessary to create a custom converter from a GameScreenModel property like an enum to Visibility. You could then setup nearly everything declaratively in XAML only.

\$\endgroup\$
  • \$\begingroup\$ IS MVVM relatively inseparable from WPF? I'm brand new to C# and this project was just me plunging right in. The more I spent reading the more I found MVVM popping up but is it unconventional to do WPF without it? \$\endgroup\$ – brug Jan 4 '19 at 15:01
  • \$\begingroup\$ @bruglesco MVVM is just a design pattern to help you make certain things easier to implement, maintain and test, primarily via data-binding. Such design virtually forces you to separate the model from the view so theoretically you could much easier exchange them but practically it makes testing easier because you don't require the view to test its model. Sure, you can do WPF without MVVM, there is nothing wrong with that but it's not the most popular way and that it has its disadvantages, so yes, it's pretty unconventional. Once you get how MVVM works, you won't be doing anything else ;-) \$\endgroup\$ – t3chb0t Jan 4 '19 at 15:59

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.