Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added assignment 1, 2, 3, 4, 5 (WIP) #3

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

khushbu014
Copy link

Khushbu Goyal (SGSITS Indore) : Added Assignment no. 1 and Assignment no. 2

@abhinav-adtechs abhinav-adtechs added the review pending review pending label Nov 19, 2019
@khushbu014 khushbu014 changed the title added assignment1 and assignment2 added assignment 1, 2 and 3 Nov 19, 2019
NucleiAsgn1/src/asgn/CalculateTax.java Outdated Show resolved Hide resolved
NucleiAsgn1/src/asgn/Inventory.java Outdated Show resolved Hide resolved
package asgn.model;

//Class to describe the properties of item
public class Item {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try lombok plugin.
Lombok removes the extra boiling plate code like getter and setters, Constructors etc.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I am adding it.

public static ArrayList<Item> getInputItem() {
Scanner sc = new Scanner(System.in);

System.out.println("Enter the following product details:");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In question it was stated to use command line inputs. Not the scanner.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can take command line input for first input.
But for multiple inputs I will have to use scanner?

NucleiAsgn1/src/asgn/util/InputUtil.java Outdated Show resolved Hide resolved
NucleiAsgn2/src/asgn/StudentRecord.java Outdated Show resolved Hide resolved
NucleiAsgn2/src/asgn/StudentRecord.java Outdated Show resolved Hide resolved
NucleiAsgn2/src/asgn/StudentRecord.java Outdated Show resolved Hide resolved

private static void performAction(int choice) {
switch (choice) {
case 1:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not use hardcoding


import asgn.model.Node;

public class GraphOperation {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every operation is in same class. Try to segregate the common logic

@gaurav4ever gaurav4ever added review done review done scope for improvement scope for improvement and removed review pending review pending labels Nov 23, 2019
@gaurav4ever
Copy link
Owner

  1. Should have used Code Design patterns.
  2. Constant class missing.
  3. Business logic was not segregated.
  4. Business logic used was correct.
  5. Command line inputs were not used.

@khushbu014 khushbu014 changed the title added assignment 1, 2 and 3 added assignment 1, 2, 3 and 4 (WIP) Dec 1, 2019
@khushbu014 khushbu014 changed the title added assignment 1, 2, 3 and 4 (WIP) added assignment 1, 2, 3, 4, 5 (WIP) Dec 19, 2019
@10yogi 10yogi mentioned this pull request Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review done review done scope for improvement scope for improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants