[refactoring] network library #9600

AndrewShkrob · 2024-11-03T09:56:07Z

AndrewShkrob commented

2024-11-03 09:56:07 +00:00

Here comes a huge refactoring of networking :)

Created a separate network library.
Moved all network-related sources from platform to network folder.

New library structure:

├── internal   - Internal lib sources that are not visible outside of the library
│   ├── internal sources
│   └── native  - Internal lib sources unique for each platform
│       ├── apple  - sources for macos/ios
│       │   ├── http
│       │   │   ├── client.mm
│       │   │   ├── thread.h
│       │   │   ├── thread.mm
│       │   │   └── ...
│       └── desktop  - sources for desktop (linux/windows) - qt network implementation
│           └── http
│               ├── client.cpp
│               ├── thread.cpp
│               └── thread.hpp
├── network_tests
│   └── tests sources
├── network_tests_support
│   └── tests sources
└── library sources

Updated and rewritten with (gtest) unit tests.

Here comes a huge refactoring of networking :) Created a separate network library. Moved all network-related sources from platform to network folder. New library structure: ``` ├── internal - Internal lib sources that are not visible outside of the library │ ├── internal sources │ └── native - Internal lib sources unique for each platform │ ├── apple - sources for macos/ios │ │ ├── http │ │ │ ├── client.mm │ │ │ ├── thread.h │ │ │ ├── thread.mm │ │ │ └── ... │ └── desktop - sources for desktop (linux/windows) - qt network implementation │ └── http │ ├── client.cpp │ ├── thread.cpp │ └── thread.hpp ├── network_tests │ └── tests sources ├── network_tests_support │ └── tests sources └── library sources ``` Updated and rewritten with (gtest) unit tests.

🚀 2

vng (Migrated from github.com) reviewed 2024-11-03 09:56:07 +00:00

biodranik (Migrated from github.com) reviewed 2024-11-03 18:46:51 +00:00

biodranik (Migrated from github.com) left a comment

Thanks for cleaning it up.

What's the motivation/benefits?
Are there any changes or is it only a pure refactoring?

Thanks for cleaning it up. 1. What's the motivation/benefits? 2. Are there any changes or is it only a pure refactoring?

network/CMakeLists.txt

					
				@ -0,0 +25,4 @@

				omim_add_library(${PROJECT_NAME} ${PUBLIC_SOURCES})

				target_sources(${PROJECT_NAME} PRIVATE

				  internal/http/file_request.hpp

biodranik (Migrated from github.com) commented

2024-11-03 18:38:41 +00:00

Dropping the internal folder may simplify the structure.

network/chunks_download_strategy.cpp

biodranik (Migrated from github.com) commented

2024-11-03 18:39:13 +00:00

nit

          LOG(LINFO, ("Thread for url", m_servers[s].m_url, "failed to download chunk number", m_servers[s].m_chunkIndex));

nit ```suggestion LOG(LINFO, ("Thread for url", m_servers[s].m_url, "failed to download chunk number", m_servers[s].m_chunkIndex)); ```

network/chunks_download_strategy.hpp

					
				@ -0,0 +78,4 @@

				  /// @returns url of the chunk

				  std::string ChunkFinished(bool success, RangeT const & range);

				  size_t ActiveServersCount() const { return m_servers.size(); }

biodranik (Migrated from github.com) commented

2024-11-03 18:40:42 +00:00

No &&?

    ServerT(std::string url, int ind) : m_url(std::move(url)), m_chunkIndex(ind) {}

No &&? ```suggestion ServerT(std::string url, int ind) : m_url(std::move(url)), m_chunkIndex(ind) {} ```

network/http/client.cpp

					
				@ -0,0 +2,4 @@

				#include "coding/base64.hpp"

				namespace om::network::http

biodranik (Migrated from github.com) commented

2024-11-03 18:41:40 +00:00

Is om:: namespace necessary/helpful?

AndrewShkrob reviewed 2024-11-05 17:27:22 +00:00

network/CMakeLists.txt

					
				@ -0,0 +25,4 @@

				omim_add_library(${PROJECT_NAME} ${PUBLIC_SOURCES})

				target_sources(${PROJECT_NAME} PRIVATE

				  internal/http/file_request.hpp

AndrewShkrob commented

2024-11-05 17:27:22 +00:00

An internal folder was added on purpose.
As I wrote in the description, Internal folder will hide all sources (and headers) that shouldn't be visible outside of the library.
This will help to organize our sources and keep the public interface clear.
In theory (it can be tested) it may also reduce the binary size or at least linking stage time

An internal folder was added on purpose. As I wrote in the description, Internal folder will hide all sources (and headers) that shouldn't be visible outside of the library. This will help to organize our sources and keep the public interface clear. In theory (it can be tested) it may also reduce the binary size or at least linking stage time

AndrewShkrob reviewed 2024-11-05 17:29:30 +00:00

network/chunks_download_strategy.hpp

					
				@ -0,0 +78,4 @@

				  /// @returns url of the chunk

				  std::string ChunkFinished(bool success, RangeT const & range);

				  size_t ActiveServersCount() const { return m_servers.size(); }

AndrewShkrob commented

2024-11-05 17:29:30 +00:00

This is a clang-tidy's recommendation
https://clang.llvm.org/extra/clang-tidy/checks/modernize/pass-by-value.html

This is a clang-tidy's recommendation https://clang.llvm.org/extra/clang-tidy/checks/modernize/pass-by-value.html

AndrewShkrob reviewed 2024-11-05 17:32:57 +00:00

network/http/client.cpp

					
				@ -0,0 +2,4 @@

				#include "coding/base64.hpp"

				namespace om::network::http

AndrewShkrob commented

2024-11-05 17:32:57 +00:00

I'd say so.
In the future (when all our libs have it) it should help to differentiate our libs from the others
If we decide to create some kind of SDK, it would also be good to have a namespace.
Just following best practices, you know :)

I'd say so. In the future (when all our libs have it) it should help to differentiate our libs from the others If we decide to create some kind of SDK, it would also be good to have a namespace. Just following best practices, you know :)

AndrewShkrob reviewed 2024-11-05 17:34:10 +00:00

biodranik (Migrated from github.com) reviewed 2024-11-09 09:15:36 +00:00

network/chunks_download_strategy.hpp

					
				@ -0,0 +78,4 @@

				  /// @returns url of the chunk

				  std::string ChunkFinished(bool success, RangeT const & range);

				  size_t ActiveServersCount() const { return m_servers.size(); }

biodranik (Migrated from github.com) commented

2024-11-09 09:15:36 +00:00

The link explicitly says that this approach (pass an argument by value) is beneficial when the function is used in two different ways: accepts a copy of the argument, or accepts a moveable object (e.g. RVO). Is that the case here? E.g. if the argument is always copied, or is always moved, then passing it explicitly as a const ref or && avoids unnecessary pessimisation.

biodranik (Migrated from github.com) reviewed 2024-11-09 09:16:49 +00:00

network/http/client.cpp

					
				@ -0,0 +2,4 @@

				#include "coding/base64.hpp"

				namespace om::network::http

biodranik (Migrated from github.com) commented

2024-11-09 09:16:49 +00:00

Does it make sense to start following best practices in the whole code, and only when it is necessary? E.g. when we start working on the SDK? Otherwise, we'll have different mixed approaches in the code that will confuse developers.

biodranik (Migrated from github.com) reviewed 2024-11-09 09:22:23 +00:00

network/CMakeLists.txt

					
				@ -0,0 +25,4 @@

				omim_add_library(${PROJECT_NAME} ${PUBLIC_SOURCES})

				target_sources(${PROJECT_NAME} PRIVATE

				  internal/http/file_request.hpp

biodranik (Migrated from github.com) commented

2024-11-09 09:22:23 +00:00

With your proposed change, developers immediately see header files that are part of the public interface. However, it may be harder to start using something necessary that was hidden before in the internal folder.

In this specific case, IMO it is easier to keep "internal" files in their subfolders ("http"), the "internal" folder adds an unnecessary level to the sources tree.

With your proposed change, developers immediately see header files that are part of the public interface. However, it may be harder to start using something necessary that was hidden before in the internal folder. In this specific case, IMO it is easier to keep "internal" files in their subfolders ("http"), the "internal" folder adds an unnecessary level to the sources tree.

AndrewShkrob reviewed 2024-11-24 14:30:01 +00:00

network/CMakeLists.txt

					
				@ -0,0 +25,4 @@

				omim_add_library(${PROJECT_NAME} ${PUBLIC_SOURCES})

				target_sources(${PROJECT_NAME} PRIVATE

				  internal/http/file_request.hpp

AndrewShkrob commented

2024-11-24 14:30:01 +00:00

I can propose another file structure:

├── include
│   ├── public_header_1.hpp
│   └── public_header_2.hpp
├── src
│   ├── private_header.hpp
│   └── source.cpp
├── tests
│   └── tests sources
└── CMakeLists.txt

But anyway, for a library there should be

public interface
private implementation

However, it may be harder to start using something necessary that was hidden before in the internal folder.

The only reason it may be harder (regarding our project) is that the feature is not yet implemented for every platform. We have this problem right now. Some files/classes are used in the iOS project, but they are not implemented for Android. So, there is no single public interface for it.
This should be refactored/fixed but not in this PR.
There are also some files that are not used at all (and I doubt they will be used in the future) but I didn't want to remove them. At least in this PR.

All in all, I think the problem you described is not a real problem :)

It's not hard to create a public header for a new feature. Because it's just a header, right?
You won't use something "necessary" that was implemented before (and wasn't already used) and lacks public headers because:
If it lacks a public header then it's not fully implemented
You cannot use something that is not implemented, you need to implement it first
When you implement it, there is no problem with adding a public header for it.

P.S. I just reviewed these two file structures and must admit I like the second one more (with separate include and src folders).

@biodranik @vng WDYT?

I can propose another file structure: ``` ├── include │ ├── public_header_1.hpp │ └── public_header_2.hpp ├── src │ ├── private_header.hpp │ └── source.cpp ├── tests │ └── tests sources └── CMakeLists.txt ``` But anyway, for a library there should be 1. public interface 2. private implementation > However, it may be harder to start using something necessary that was hidden before in the internal folder. The only reason it may be harder (regarding our project) is that the feature is not yet implemented for every platform. We have this problem right now. Some files/classes are used in the iOS project, but they are not implemented for Android. So, there is no single public interface for it. This should be refactored/fixed but not in this PR. There are also some files that are not used at all (and I doubt they will be used in the future) but I didn't want to remove them. At least in this PR. All in all, I think the problem you described is not a real problem :) 1. It's not hard to create a public header for a new feature. Because it's just a header, right? 2. You won't use something "necessary" that was implemented before (and wasn't already used) and lacks public headers because: 1. If it lacks a public header then it's not fully implemented 2. You cannot use something that is not implemented, you need to implement it first 3. When you implement it, there is no problem with adding a public header for it. P.S. I just reviewed these two file structures and must admit I like the second one more (with separate `include` and `src` folders). @biodranik @vng WDYT?

rtsisyk reviewed 2025-01-11 09:18:41 +00:00

network/CMakeLists.txt

					
				@ -0,0 +25,4 @@

				omim_add_library(${PROJECT_NAME} ${PUBLIC_SOURCES})

				target_sources(${PROJECT_NAME} PRIVATE

				  internal/http/file_request.hpp

rtsisyk commented

2025-01-11 09:18:41 +00:00

Storing public headers in include/ and the implementation in src/ is a well-established practice and likely a good approach. Personally, I prefer a flatter, one-level structure, without the need for separate "native" and "http" folders. A more complex structure encourages people to explicitly increase complexity, whereas this entire library could likely be simplified and fit into just a few reasonably sized files.

This repo is archived. You cannot comment on pull requests.

No reviewers

vng