Higher computing 2014/15 coursework penny's decking

Thanks for the input some of it I disagree with though.

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) 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.

I think OP might of expected Winforms so go with that. Up to you on the database.

I think OP might of expected Winforms so go with that. Up to you on the database.

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.

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.

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()


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


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.

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 [pre]Deck[/pre] class and a [pre]DeckStore[/pre] 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.

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????

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 [pre]Main[/pre] 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#)?

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?

Hmmnm I see. You can single it out but who cares 😂