2
\$\begingroup\$

I am working project where I need to build ASP.net control base on JSON data. I am using this method below to get all the data into class. I as using FirstOrDefault and the Where(x => x.Type == ??? to drill down into the data. I think the logic could be simplified, Any suggestions are welcomed. working sample below

JSON data

[
    {
        "MeetingPollingQuestionId": 2,
        "MeetingPollingQuestionType": "LongAnswerText",
        "MeetingPollingId": 3,
        "SequenceOrder": 1,
        "MeetingPollingParts": [
            {
                "MeetingPollingPartsId": 2,
                "Type": "Question",
                "MeetingPollingQuestionId": 2,
                "MeetingPollingPartsValues": [
                    {
                        "Type": "label",
                        "QuestionValue": "This is a long question",
                        "FileManagerId": 0,
                        "FileName": null,
                        "FileData": null,
                        "FileType": null
                    }
                ]
            }
        ]
    },
    {
        "MeetingPollingQuestionId": 3,
        "MeetingPollingQuestionType": "MultipleChoice",
        "MeetingPollingId": 3,
        "SequenceOrder": 2,
        "MeetingPollingParts": [
            {
                "MeetingPollingPartsId": 3,
                "Type": "Question",
                "MeetingPollingQuestionId": 3,
                "MeetingPollingPartsValues": [
                    {
                        "Type": "label",
                        "QuestionValue": "this is a multiple choice question",
                        "FileManagerId": 0,
                        "FileName": null,
                        "FileData": null,
                        "FileType": null
                    }
                ]
            },
            {
                "MeetingPollingPartsId": 4,
                "Type": "Image",
                "MeetingPollingQuestionId": 3,
                "MeetingPollingPartsValues": [
                    {
                        "Type": "Image",
                        "QuestionValue": null,
                        "FileManagerId": 14552,
                        "FileName": null,
                        "FileData": null,
                        "FileType": null
                    }
                ]
            },
            {
                "MeetingPollingPartsId": 5,
                "Type": "Answers",
                "MeetingPollingQuestionId": 3,
                "MeetingPollingPartsValues": [
                    {
                        "Type": "radio",
                        "QuestionValue": "Yes",
                        "FileManagerId": 0,
                        "FileName": null,
                        "FileData": null,
                        "FileType": null
                    },
                    {
                        "Type": "radio",
                        "QuestionValue": "No",
                        "FileManagerId": 0,
                        "FileName": null,
                        "FileData": null,
                        "FileType": null
                    },
                    {
                        "Type": "radio",
                        "QuestionValue": "Abstain",
                        "FileManagerId": 0,
                        "FileName": null,
                        "FileData": null,
                        "FileType": null
                    }
                ]
            }
        ]
    }
]

method creating controls

public class LongAnswerText : IMeetingPollingQuestion
    {
        private List<MeetingPollingParts> meetingPollingParts;
        private string label = null;
        private string textbox = null;
        private string type = null;

       
        public LongAnswerText(List<MeetingPollingParts> meetingPollingParts)
        {
           
            this.SequenceOrder = SequenceOrder;
            this.MeetingPollingId = MeetingPollingId;
            this.MeetingPollingQuestionId = MeetingPollingQuestionId;

            this.meetingPollingParts = meetingPollingParts;
            var MeetingPollingPartsValuesLabel = new List<MeetingPollingPartsValues>();
            MeetingPollingPartsValuesLabel = meetingPollingParts.SelectMany(s => s.MeetingPollingPartsValues).ToList();
            var labelControl = MeetingPollingPartsValuesLabel.Where(x=>x.Type=="label").FirstOrDefault();
            var textboxControl = MeetingPollingPartsValuesLabel.Where(x => x.Type == "textbox").FirstOrDefault();
            this.label = LabelControl(string.Format("label_{0}", labelControl.MeetingPollingPartsValuesId), labelControl.QuestionValue);
            this.textbox = TextboxControl(string.Format("label_{0}", labelControl.MeetingPollingPartsValuesId));
        }

        public  string LabelControl(string target, string text){
            return string.Format("@Html.LabelFor('{0}'>{1}</input>",target,text);
        }

        public string TextboxControl(string target) { 
            return string.Format("@Html.TextBoxFor('{0}')",target);
            }
    }

Output View

<div class="form-group row">
   div class="col-md-12">
        @Html.LabelFor(model => model.label_1)
        @Html.TextBoxFor(x => x.label_1, new { id = "label_1", @class = "form-control" })
   </div>
</div>

fiddle

https://dotnetfiddle.net/j6YIPN
\$\endgroup\$
7
  • \$\begingroup\$ Could you please share with us a sample JSON data as well? \$\endgroup\$ Oct 20, 2022 at 8:45
  • 1
    \$\begingroup\$ I updated with the same json data \$\endgroup\$
    – Jefferson
    Oct 20, 2022 at 11:31
  • 2
    \$\begingroup\$ .Where(x=>x.Type=="label").FirstOrDefault(); => .FirstOrDefault(x=>x.Type=="label"); \$\endgroup\$ Oct 20, 2022 at 13:52
  • 1
    \$\begingroup\$ Am I understand correctly that you do self-assignment here: this.SequenceOrder = SequenceOrder;? \$\endgroup\$ Oct 21, 2022 at 14:39
  • 1
    \$\begingroup\$ Oh your correct no need to do that \$\endgroup\$
    – Jefferson
    Oct 22, 2022 at 15:06

1 Answer 1

3
+50
\$\begingroup\$

As was mentioned in the comments, there is no need in using Where LINQ since FirstOrDefault accepts a predicate that can be used to get the value you need:

var labelControl = MeetingPollingPartsValuesLabel.FirstOrDefault(part => part.Type == "label");

As for the other issues...

this.SequenceOrder = SequenceOrder;
this.MeetingPollingId = MeetingPollingId;
this.MeetingPollingQuestionId = MeetingPollingQuestionId;

There is no point is assigning a property to itself (unless there is some important logic in the setter but that is even worse and not your case).
You can just delete those lines with no effect.

Similarly there is no reason to assign null to all those fields:

private string label = null;
private string textbox = null;
private string type = null;

They are already null by default.

List<MeetingPollingParts> meetingPollingParts
This parameter name implies that it is a collection of some parts. However type parameter of the List is MeetingPollingParts in plural, which means that it is a collection of collections of parts. MeetingPollingParts should be named MeetingPollingPart since it represents a singular part. Such small naming mistakes make the code much harder to read. Please pay attention to them.

Methods LabelControl and TextboxControl don't use the instance of LongAnswerText whose member methods they are, therefore there is no reason in them being member methods.

LongAnswerText : IMeetingPollingQuestion
This implementation looks out of place: why does Text implement Question? Judging by your full code, there is no reason in having this interface in the first place since it only has one implementation and you never call instances of that implementation by the interface.


In conclusion, most of the suggestions are aiming at making your solution less verbose: it will help a lot with readability and overall code clarity. There is a lot of extra logic which is not needed for the task you're trying to accomplish, get read of it and it will become much easier to implement stuff that you actually need.

Good luck with refactoring!

\$\endgroup\$
10
  • \$\begingroup\$ Not sure what you mean by member methods can you explain \$\endgroup\$
    – Jefferson
    Oct 22, 2022 at 2:17
  • \$\begingroup\$ @Jefferson member or instance method is a method that works on a particular instance of a class. E.g. method bark() in dog.bark() would not make sense if you call it without a dog instance. \$\endgroup\$ Oct 22, 2022 at 9:14
  • \$\begingroup\$ Thanks for the question about the interface if I wanted to use that what would I need to do? \$\endgroup\$
    – Jefferson
    Oct 22, 2022 at 18:52
  • \$\begingroup\$ @Jefferson sorry, what question? \$\endgroup\$ Oct 22, 2022 at 18:58
  • \$\begingroup\$ The one about no reason in having this interface? \$\endgroup\$
    – Jefferson
    Oct 22, 2022 at 21:14

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.