Turn on thread page Beta

Higher computing 2014/15 coursework penny's decking watch

    Offline

    19
    ReputationRep:
    (Original post by INTit)
    Thanks for the input some of it I disagree with though.
    • XML is easily read and edited using a text editor JSON is not and theirs no real performance benefit to be gained by using JSON in this case.
    • I don't see where I have getters and setters. I have some methods prefixed with get but they perform logic so would not be suitable as properties.
    • The select case statement was intended to be added to by the OP as they implemented the rest of the functionality.
    • Lots of methods doing one thing was intentional - it doesn't call 4 routines to display an error message. Sure get rid of invalidInput that one was redundant but I'd keep the rest.
    First one, fair enough.

    But uhm you do use getters and setters sub routines. you did setDeck

    You could use a property for that.

    Also, you have functions that do not return a value. There's so much I could point out, but I'm new to this forums so the layout is just so long to work with. I could re-code what you have coded from scratch and show you a better way to do some of the things you did if you want. Maybe you could learn a thing of two.
    Offline

    17
    ReputationRep:
    (Original post by Async)
    First one, fair enough.

    But uhm you do use getters and setters sub routines. you did setDeck

    You could use a property for that.

    Also, you have functions that do not return a value. There's so much I could point out, but I'm new to this forums so the layout is just so long to work with. I could re-code what you have coded from scratch and show you a better way to do some of the things you did if you want. Maybe you could learn a thing of two.
    Your right setDeck is 1) a setter and 2) returns nothing despite being a function .3) shouldnt be a function 4) probably shouldnt exist.

    You can rewrite the whole thing and it would interest me but probably not the OP. Be forwarned the temptation to pick holes would be strong
    Offline

    19
    ReputationRep:
    (Original post by INTit)
    Your right setDeck is 1) a setter and 2) returns nothing despite being a function .3) probably shouldnt exist.

    You can rewrite the whole thing and it would interest me but probably not the OP. Be forwarned the temptation to pick holes would be strong
    It's fine, I welcome criticisms. That's how I've learnt programming to be honest with you.
    Right, before I start. Should I do this using a consoles application or windows form? Also, can I use a database? It would be easier to do this using a database, the storing of data is something that should always be done using a database. Depending on what you intend to store. If I can't use databases then I'll be most likely be using JSON to store in a txt file.
    Offline

    17
    ReputationRep:
    (Original post by Async)
    It's fine, I welcome criticisms. That's how I've learnt programming to be honest with you.
    Right, before I start. Should I do this using a consoles application or windows form? Also, can I use a database? It would be easier to do this using a database, the storing of data is something that should always be done using a database. Depending on what you intend to store. If I can't use databases then I'll be most likely be using JSON to store in a txt file.
    I think OP might of expected Winforms so go with that. Up to you on the database.
    Offline

    19
    ReputationRep:
    (Original post by INTit)
    I think OP might of expected Winforms so go with that. Up to you on the database.
    Just got back now. I'm gonna start it. I should be finished soon.
    Offline

    19
    ReputationRep:
    (Original post by INTit)
    I think OP might of expected Winforms so go with that. Up to you on the database.
    DeckHelper.vb
    Code:
    Imports Newtonsoft.Json
    
    
    Public Class DeckHelper
    
    
        Private Shared ReadOnly R As New Random
    
    
        Public Shared Function GetDeckByArea(deckList As List(Of GardenDeck), area As Integer) As IEnumerable(Of GardenDeck)
            Return From d In deckList Where d.Area >= area
        End Function
    
    
        Public Shared Function GetDeckByLength(deckList As List(Of GardenDeck), length As Integer) As IEnumerable(Of GardenDeck)
            Return From d In deckList Where d.Length >= length
        End Function
    
    
        Public Shared Function GetCheapestDeck(deckList As List(Of GardenDeck)) As GardenDeck
            Return deckList.OrderBy(Function(d) d.Price).FirstOrDefault()
        End Function
    
    
        Public Shared Function LoadDeck(location As String) As GardenDeck()
            Dim txt As String = IO.File.ReadAllText(location)
            Return JsonConvert.DeserializeObject(Of GardenDeck())(txt)
        End Function
    
    
        Public Shared Sub SaveDeck(deckList As List(Of GardenDeck), location As String)
            Dim txt As String = JsonConvert.SerializeObject(deckList)
            IO.File.WriteAllText(location, txt)
        End Sub
    
    
        Public Shared Function GenerateDeck(amount As Integer) As GardenDeck()
            Dim decks(amount) As GardenDeck
    
    
            For i = 0 To amount
                decks(i) = New GardenDeck With {.Name = "Deck: " & i, .Length = R.Next(1, 500), .Width = R.Next(1, 500), .Price = R.Next(1, 500)}
            Next
    
    
            Return decks
        End Function
    End Class
    
    
    Public Class GardenDeck
        Public Property Name As String
        Public Property Length As Integer
        Public Property Width As Integer
        Public Property Price As Double
    
    
        Public ReadOnly Property Area As Double
            Get
                Return Width * Length
            End Get
        End Property
    
    
    End Class


    Main program
    Code:
    Module Module1
    
    
        Private MyDeckList As New List(Of GardenDeck)
    
    
        Sub Main()
            Start()
        End Sub
    
    
        Private Sub Start()
            Dim choice As Integer
    
    
            Do
                Console.Clear()
    
    
                DisplayMenu()
    
    
                choice = GetUserChoice()
    
    
                Select Case choice
                    Case 1
                        Dim deck As GardenDeck = DeckHelper.GetCheapestDeck(MyDeckList)
                        Console.WriteLine("The cheapest desk is {0} which costs ${1}", deck.Name, deck.Price)
                    Case 2
                        Dim length As Integer = 50
                        For Each deck In DeckHelper.GetDeckByLength(MyDeckList, 10)
                            Console.WriteLine("{0} is greater than {1}cm", deck.Name, length)
                        Next
                    Case 3
                        Dim area As Integer = 27
                        For Each deck In DeckHelper.GetDeckByArea(MyDeckList, area)
                            Console.WriteLine("{0} is greater than {1}cm", deck.Name, area)
                        Next
                    Case 4
                        MyDeckList.AddRange(DeckHelper.GenerateDeck(20))
                    Case 5
                        DeckHelper.SaveDeck(MyDeckList, "deck.txt")
                    Case 6
                        MyDeckList.AddRange(DeckHelper.LoadDeck("deck.txt"))
                    Case 7
                        MyDeckList = New List(Of GardenDeck)
                    Case 8
                        Environment.Exit(0)
                End Select
    
    
                Console.ReadLine()
    
    
            Loop Until choice = 8
        End Sub
    
    
    
    
        Private Function GetUserChoice() As Integer
            Dim choice As Integer
    
    
            Do
                Console.WriteLine()
                Console.WriteLine("Enter your choice")
            Loop Until Integer.TryParse(Console.ReadLine(), choice)
    
    
            Return choice
        End Function
    
    
        Private Sub DisplayMenu()
            Console.WriteLine("Enter 1 to find the cheapest garden deck")
            Console.WriteLine("Enter 2 to display the names of garden decks over a certain length")
            Console.WriteLine("Enter 3 to display garden decks that are available over a certain area")
            Console.WriteLine("Enter 4 to generate a deck")
            Console.WriteLine("Enter 5 to save the current deck")
            Console.WriteLine("Enter 6 to load the current deck")
            Console.WriteLine("Enter 7 to wipe current deck")
            Console.WriteLine("Enter 8 to quit")
            Console.WriteLine()
        End Sub
    
    
    End Module
    Send me your criticisms. Please excuse the fact that I didn't make the application all fancy and stuff. Couldn't be bothered to be honest.

    You might need to download the JSON.NET package for your VB Solution through the Visual Studio NuGET package if you don't already know how to, to be able to use that class.
    http://www.newtonsoft.com/json
    Offline

    19
    ReputationRep:
    I see you don't want to reply
    Offline

    17
    ReputationRep:
    (Original post by Async)
    I see you don't want to reply
    Sorry I didn't reply last night reply was getting late.

    My thoughts:

    I prefer the do while loop you used. When I was coding the example I think I saw the opportunity to create a loop by linking parseInput back to readInput and although convenient its more difficult to follow. I'd keep readInput and parseinput but I'd add a loop to main instead of linking them.

    VBs LINQ syntax VS dot notation. The VB syntax is easier to follow for a VB dev but the dot notation is common to c# and vb. Theres trade-offs for both my choice was based on familiarity.

    Helper object vs DeckService class. My motivation was to isolate the business logic from the rest of the logic.
    Offline

    19
    ReputationRep:
    (Original post by INTit)
    Sorry I didn't reply last night reply was getting late.

    My thoughts:

    I prefer the do while loop you used. When I was coding the example I think I saw the opportunity to create a loop by linking parseInput back to readInput and although convenient its more difficult to follow. I'd keep readInput and parseinput but I'd add a loop to main instead of linking them.

    VBs LINQ syntax VS dot notation. The VB syntax is easier to follow for a VB dev but the dot notation is common to c# and vb. Theres trade-offs for both my choice was based on familiarity.

    Helper object vs DeckService class. My motivation was to isolate the business logic from the rest of the logic.
    I don't know what the dot notation is hahaha. And uhhmm fair enough. And I call every static class a helper class. and yes your parseInput was like going all over the place just to perform one thing. It felt like a game of pass the parcel. A simple while loop would suffice. Anyway, nice to know there's members here that can read & write code.
    Offline

    0
    ReputationRep:
    (Original post by INTit)
    Ok lets extend the code to complete option two "Get the names of decks over a certain length"

    Lets create the getDecksAvailableOver function in the DeckService class. I'll use LINQ again if you don't know what it is google it as its awesome.
    Code:
    Public Function getDecksAvailableOver(length as long ) As List(Of Deck)
          Dim decksOver As List<Of Deck> =m_Decks.Select( _
                Function(x) x.length>length).toList()
         Return decksOver 
        End Function
    Ok so that will give us the decks over a certain length. But we still need to request the length from the user when they select option 2 and then display the decks.
    So lets add the findDecksOfLength method to Module1 this method will read the input in and display the result.
    It will get the user input by calling readLength() and display the decks over the desired length by calling displayDecksOver()
    Code:
    private sub findDecksOfLength()
       dim length as long = readLength() 
       dim decksOverLength As List<Of Deck>=  m_deckService.getDecksAvailableOver(length)
        displayDecksOver(decksOverLength)
    end sub
    
    private function readLength() as long
        Dim userInput As String = Console.ReadLine() //todo input validation
        return userInput;
    end sub
    
    private function displayDecksOver(decks as List<Of Deck>,length as long)
    Console.WriteLine(String.Format("Decks over:{0}", length))
      for each deck in decks
      Console.WriteLine(decks.Name)
     next deck
    end function
    Finally we need to add a new case to the select statement so that when they select option 2 the new code gets called

    Code:
    Select Case Integer.Parse(userInput)
                    Case 1
                        showCheapest()
                    Case 2 
                        findDecksOfLength()
                End Select
    Be warned the above is from the top of my head into the reply box so don't be surprised if there's typos.

    Please I am really stuck with this. Is it okay if you can convert this code into a Visual Basic Windows application instead of a Console application because I don't understand most of it. Plus I really need help with designing the user interface. Like for example I don't know how to do it the way they are asking you to in the coursework????
    Offline

    16
    ReputationRep:
    (Original post by Async)
    I don't know what the dot notation is hahaha. And uhhmm fair enough. And I call every static class a helper class. and yes your parseInput was like going all over the place just to perform one thing. It felt like a game of pass the parcel. A simple while loop would suffice. Anyway, nice to know there's members here that can read & write code.
    Why would those methods be static in the first place? That is a recipe for unmaintainable code. I see a case for a Deck class and a DeckStore class. I don't see any case for a bunch of static methods (except the argument that the application is trivial, in which case there is no point asking for a critique).

    A rule of thumb - if "Helper" is the best suffix you can come up with for a static class, you're probably making a smell.
    Offline

    19
    ReputationRep:
    (Original post by Planto)
    Why would those methods be static in the first place? That is a recipe for unmaintainable code. I see a case for a Deck class and a DeckStore class. I don't see any case for a bunch of static methods (except the argument that the application is trivial, in which case there is no point asking for a critique).

    A rule of thumb - if "Helper" is the best suffix you can come up with for a static class, you're probably making a smell.
    How is the code not maintainable? It's pretty simple to read if you ask me. I made the class static because I saw it more of a utility class. A class to help accomplish the functions of the given task. But you do have a point though. Now that I think of it, I could of made the LoadDeck, SaveDeck and GenerateDeck a static methods and the others should not be.

    To be honest, it's debateable

    If you think you could do better then I'd like to see your attempt
    Offline

    19
    ReputationRep:
    (Original post by JordanSmith1998)
    Please I am really stuck with this. Is it okay if you can convert this code into a Visual Basic Windows application instead of a Console application because I don't understand most of it. Plus I really need help with designing the user interface. Like for example I don't know how to do it the way they are asking you to in the coursework????
    There's nothing to convert. Everything is pretty much done. Just use the class provided to do what you aim to do.
    If you don't understand how the class works, don't use it. Do something you understand.
    Offline

    16
    ReputationRep:
    (Original post by Async)
    How is the code not maintainable? It's pretty simple to read if you ask me. I made the class static because I saw it more of a utility class. A class to help accomplish the functions of the given task. But you do have a point though. Now that I think of it, I could of made the LoadDeck, SaveDeck and GenerateDeck a static methods. But the others should not be, I should of used a constructor for the DeckHelper class whereby the other non static methods used the data initialized through the constructors.

    To be honest, it's debateable
    I think most experienced software engineers would disagree with you on how debatable this is.

    Maintainability and readability are two very different things. As I already hinted at, in its current size and complexity, there are no issues with this code but - in its current size and complexity - bunging everything in a big Main method would probably be fine, too. The point is, you asked for a critique, so it's fair to assume that you're writing this code as though it's a real application. If someone on my team gave me this, I would send it back.

    The static methods you have break encapsulation, they are not testable (how do you mock out your DAL?), they make extending the app to support concurrency impossible without a rewrite (loading your decks into a static member?) and they lead to absolute spaghetti as your applicaton's complexity increases and you have to pass all kinds of parameters to all kinds of static methods because you have no discernible structure or object state.

    There are cases for static "utility" methods - namely extensions or methods that expose static configuration. Data access is certainly not a utility. Why not use an instance-based storage mechanism (forgive my C#)?

    Code:
    public class DeckStore
    {
        private readonly string _location;
    
        public DeckStore(string location)
        {
            if (location == null) throw new ArgumentNullException("location");
            _location = location;
        }
    
        public ICollection<Deck> Load()
        {
            ...
        }
    
        public void Save(ICollection<Deck> decks)
        {
            ...
        }
    }
    Isolated, testable, scalable and, better still, you can abstract the implementation behind an interface so when you decide a JSON file is no good you don't have to throw everything away again.
    Offline

    19
    ReputationRep:
    (Original post by Planto)
    I think most experienced software engineers would disagree with you on how debatable this is.

    Maintainability and readability are two very different things. As I already hinted at, in its current size and complexity, there are no issues with this code but - in its current size and complexity - bunging everything in a big Main method would probably be fine, too. The point is, you asked for a critique, so it's fair to assume that you're writing this code as though it's a real application. If someone on my team gave me this, I would send it back.

    The static methods you have break encapsulation, they are not testable (how do you mock out your DAL?), they make extending the app to support concurrency impossible without a rewrite (loading your decks into a static member?) and they lead to absolute spaghetti as your applicaton's complexity increases and you have to pass all kinds of parameters to all kinds of static methods because you have no discernible structure or object state.

    There are cases for static "utility" methods - namely extensions or methods that expose static configuration. Data access is certainly not a utility. Why not use an instance-based storage mechanism (forgive my C#)?

    Code:
    public class DeckStore
    {
        private readonly string _location;
    
        public DeckStore(string location)
        {
            if (location == null) throw new ArgumentNullException("location");
            _location = location;
        }
    
        public ICollection<Deck> Load()
        {
            ...
        }
    
        public void Save(ICollection<Deck> decks)
        {
            ...
        }
    }
    Isolated, testable, scalable and, better still, you can abstract the implementation behind an interface so when you decide a JSON file is no good you don't have to throw everything away again.

    Again, this is heavyweight for such a simple application but there is no point in asking for a critique unless you're going to treat it like a real project.

    Edit: I made a comment on the code, I just realized it was a constructor. So disregard that comment if you seen it.

    Other than that, uhmm, you already know I'm not an experienced developer in terms of years from my previous help thread. But that is my goal though and I appreciate your comment I agree with your input. I should of used constructors to initialize the deck and the location too.

    Quick question, how long have you been coding for?
    Offline

    17
    ReputationRep:
    (Original post by Async)
    Edit: I made a comment on the code, I just realized it was a constructor. So disregard that comment if you seen it.

    Other than that, uhmm, you already know I'm not an experienced developer in terms of years from my previous help thread. But that is my goal though and I appreciate your comment I agree with your input. I should of used constructors to initialize the deck and the location too.

    Quick question, how long have you been coding for?
    Keep in mind the code posted was simplified not real world quality . An example of making it more flexible: get rid of the Config class. Write a repository interface with concrete implementations saving stuff in JSON, XML, SQL etc. Change DeckService to take a repository instance via constructor injection and use that instead of the Config class.

    Swappable, mockable data access like Planto described.
    If it had a graphical UI it could use a presentation pattern etc.

    An example showing code like that would be far too complicated for the OP and potentially the teacher .
    Offline

    19
    ReputationRep:
    I just re-wrote that DeckHelper class of mine.

    Code:
    Imports Newtonsoft.Json
    
    
    Public Class DeckProvider
    
    
        Private Shared ReadOnly R As New Random
        Private _DeckList As List(Of GardenDeck)
    
    
        Sub New()
            _DeckList = New List(Of GardenDeck)
        End Sub
    
    
        Public ReadOnly Property Count As Integer
            Get
                Return _DeckList.Count
            End Get
        End Property
    
    
        Public Function GetDeckByArea(area As Integer) As IEnumerable(Of GardenDeck)
            Return From d In _DeckList Where d.Area >= area
        End Function
    
    
        Public Function GetDeckByLength(length As Integer) As IEnumerable(Of GardenDeck)
            Return From d In _DeckList Where d.Length >= length
        End Function
    
    
        Public Function GetCheapestDeck() As GardenDeck
            If _DeckList.Count <= 0 Then
                Throw New Exception("Deck is empty")
            Else
                Return _DeckList.OrderBy(Function(d) d.Price).FirstOrDefault()
            End If
        End Function
    
    
        Public Sub ImportDeck(location As String)
            Dim txt As String = IO.File.ReadAllText(location)
            _DeckList.AddRange(JsonConvert.DeserializeObject(Of GardenDeck())(txt))
        End Sub
    
    
        Public Sub SaveDeck(location As String)
            Dim txt As String = JsonConvert.SerializeObject(_DeckList)
            IO.File.WriteAllText(location, txt)
        End Sub
    
    
        Public Sub GenerateDecks(amount As Integer)
            For i = 0 To amount
                _DeckList.Add(New GardenDeck With {.Name = "Deck: " & i, .Length = R.Next(1, 50), .Width = R.Next(1, 50), .Price = R.Next(1, 100)})
            Next
        End Sub
    
    
        Public Sub Add(deck As GardenDeck)
            _DeckList.Add(deck)
        End Sub
    
    
        Public Sub RemoveAt(i As Integer)
            _DeckList.RemoveAt(i)
        End Sub
    
    
        Public Sub ClearDeck()
            _DeckList = New List(Of GardenDeck)
        End Sub
    
    
        Public Function GetAllDecks() As List(Of GardenDeck)
            If _DeckList.Count = 0 Then
                Throw New Exception("Deck list is empty")
            Else
                Return _DeckList
            End If
        End Function
    
    
    End Class
    
    
    Public Class GardenDeck
        Public Property Name As String
        Public Property Length As Integer
        Public Property Width As Integer
        Public Property Price As Double
    
    
        Public ReadOnly Property Area As Double
            Get
                Return Width * Length
            End Get
        End Property
    End Class

    Is this better than before? Is this the overall concept Planto was looking for or am I still missing something?
    Offline

    17
    ReputationRep:
    I think you missed the concept of separating out the data access code - notice that Plantos DeckStore is only concerned with loading and saving decks.


    I'll make a more detailed post tommorow it's 3am
    Offline

    19
    ReputationRep:
    Hmmnm I see. You can single it out but who cares 😂
    Offline

    16
    ReputationRep:
    (Original post by Async)
    Hmmnm I see. You can single it out but who cares 😂
    You will, when you want to test your Deck logic independently of physical stored files, or when you want to use a more robust storage mechanism but your current one is tightly coupled to the rest of your app :yes:
 
 
 
Reply
Submit reply
Turn on thread page Beta
TSR Support Team

We have a brilliant team of more than 60 Support Team members looking after discussions on The Student Room, helping to make it a fun, safe and useful place to hang out.

This forum is supported by:
Updated: May 23, 2016
Poll
Black Friday: Yay or Nay?
Useful resources

The Student Room, Get Revising and Marked by Teachers are trading names of The Student Room Group Ltd.

Register Number: 04666380 (England and Wales), VAT No. 806 8067 22 Registered Office: International House, Queens Road, Brighton, BN1 3XE

Write a reply...
Reply
Hide
Reputation gems: You get these gems as you gain rep from other members for making good contributions and giving helpful advice.