Skip to content

added helpCommand() and HELP_TEXT.#23

Open
RoeeWork wants to merge 3 commits into
feature/organize-program-structurefrom
refactor/helpCommand
Open

added helpCommand() and HELP_TEXT.#23
RoeeWork wants to merge 3 commits into
feature/organize-program-structurefrom
refactor/helpCommand

Conversation

@RoeeWork

@RoeeWork RoeeWork commented Jul 9, 2026

Copy link
Copy Markdown
Owner

added the help text command

@NetanelPa
NetanelPa self-requested a review July 10, 2026 10:58

@NetanelPa NetanelPa left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As an overall this pretty goood and way more organized. You have some wrong indents (probably) and magic numbers but overall pretty good. The most important thing is the operator overloading if you don't understand something fell free to ask

Comment thread IO/HandleOutput.cpp Outdated
Comment thread IO/HandleOutput.cpp Outdated
Comment thread IO/HandleOutput.cpp Outdated
Comment thread IO/HandleOutput.cpp Outdated
Comment thread IO/HandleOutput.cpp Outdated
Comment thread IO/HandleOutput.cpp Outdated

// prints the host vector `hosts` in a numbered list.
// only prints IPv4 and MAC addresses.
void HandleOutput::printHostVectorNumberedList(std::vector<Host> hosts) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This should get the host list as it is (I explained why in line 44 - 52 suggestion)

Suggested change
void HandleOutput::printHostVectorNumberedList(std::vector<Host> hosts) {
void HandleOutput::printHostVectorNumberedList(const HostList& hosts) {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Why get a HostList as an arguement? you would need to create a HostList object for that to work, and we agreed upon using HostLists methods and fields statically

Comment thread IO/HandleOutput.cpp Outdated
Comment on lines +44 to +52
for (const auto &h : hosts) {
std::cout << std::left
<< std::setw(4) << i << RESET
<< GREEN
<< std::setw(15) << h.getIPv4()
<< std::setw(20) << h.getMAC()<< RESET
<< '\n';
i++;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

All of the printing should be with operator overloading as eli started doing that's way more sorted and readable you should also add the index varlible to there.

Suggested change
for (const auto &h : hosts) {
std::cout << std::left
<< std::setw(4) << i << RESET
<< GREEN
<< std::setw(15) << h.getIPv4()
<< std::setw(20) << h.getMAC()<< RESET
<< '\n';
i++;
}
std::cout << hosts;
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

how should this be done in your opinion? overload << in the Host class or in the HostList class?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Both in host you should print the class members and in host list you should print each host with the already overloaded operator

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

this method is used in startChooseHostsLoop(). it prints a std::vector<Host> which represents new hosts found on the users LAN. the HostList class represents the hosts saved to serverinfo.json by the user, so overloading << doesnt fit the usecase here.

nevertheless i did add the oprator<< overloading to the Host and HostList classes, just because it seems like a better way to print the hosts saved by the user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants