From 72e98bdd2e1a6bb3b7f593ead45080f03b3081c5 Mon Sep 17 00:00:00 2001 From: "Chris.Watts90@outlook.com" Date: Wed, 6 Jun 2018 10:50:06 +0100 Subject: [PATCH] optimise code to reduce code complexity on UpdateUser method in particular. code has been split into smaller methods, and queries on db have been optimised to make fewer, larger queries than lots of small queries. (using IN sql queries). restructured UpdateUser to be more logical which has reduced some of the complexity of the method. When deleting unassociated cards, the system will ensure all timelogs associated to those Ids are deleted also. CreateGroup will now not allow duplicate groups, but will return the ID of the existing Group found in the DB. pulled out GetUserCount method to simplify the GetUsers method. removed empty quotes in favour of string.empty #95 --- .../SQLiteRepository/SQLiteRepository.cs | 240 +++++++++--------- 1 file changed, 124 insertions(+), 116 deletions(-) diff --git a/DataCenter_Windows/WindowsDataCenter/SQLiteRepository/SQLiteRepository.cs b/DataCenter_Windows/WindowsDataCenter/SQLiteRepository/SQLiteRepository.cs index 9fedbd3..94e09ed 100644 --- a/DataCenter_Windows/WindowsDataCenter/SQLiteRepository/SQLiteRepository.cs +++ b/DataCenter_Windows/WindowsDataCenter/SQLiteRepository/SQLiteRepository.cs @@ -19,6 +19,7 @@ namespace SQLiteRepository private readonly ILogger _logger; private readonly string _path; private const string DATABASE_NAME = "flexitimedb.db"; + private const string UPGRADE_SCRIPT_PREFIX = "SQLiteRepository.UpgradeScripts."; public SQLiteRepository(ILogger logger) { @@ -40,43 +41,27 @@ namespace SQLiteRepository _logger.Trace("Checking For Upgrades"); CheckForDbUpgrade(); } + public UserList GetUsers(int pageNumber = -1, int pageSize = -1, int groupId = -1) { var ret = new UserList(); - List users; - int userCount; - if (pageNumber != -1 && pageSize != -1) + List users = GetUserList(groupId, pageSize, pageNumber); + + var userCount = GetUserCount(); + + if (pageNumber == -1 && pageSize == -1) { - users = _connection.Query(SQLiteProcedures.GET_ALL_USERS_PAGINATE, - pageSize, (pageNumber - 1) * pageSize); - userCount = _connection.ExecuteScalar(SQLiteProcedures.GET_TOTAL_USER_COUNT); - } - else if (groupId != -1) - { - users = - _connection.Query( - SQLiteProcedures.GET_ALL_USERS_BY_GROUP, - groupId); - userCount = users.Count; + ret.PageNumber = 1; + ret.PageSize = 20; } else { - users = _connection.Query(SQLiteProcedures.GET_ALL_USERS); - userCount = users.Count; + ret.PageNumber = pageNumber; + ret.PageSize = pageSize; } if (!users.Any()) { - if (pageNumber == -1 && pageSize == -1) - { - ret.PageNumber = 1; - ret.PageSize = 20; - } - else - { - ret.PageNumber = pageNumber; - ret.PageSize = pageSize; - } return ret; } @@ -90,20 +75,11 @@ namespace SQLiteRepository userObj.Groups = GetGroups(user.Id); ret.Users.Add(userObj); } - if (pageNumber == -1 && pageSize == -1) - { - ret.PageSize = 10; - ret.PageNumber = 1; - } - else - { - ret.PageSize = pageSize; - ret.PageNumber = pageNumber; - } + ret.TotalUserCount = userCount; return ret; } - + public UserList Search(string searchParam) { _logger.Trace("Searching SQLite database for the term: {0}", searchParam); @@ -131,19 +107,12 @@ namespace SQLiteRepository foreach (var card in cards) { userObj.AssociatedIdentifiers.Add(IdentifierConverter.ConvertToIdentifierDto(card)); - //userObj.AssociatedIdentifiers.Add(new Identifier() - //{ - // UniqueId = card.CardUId, - // IsAssociatedToUser = true, - // Id = card.Id - //}); } userObj.State = GetUserState(GetLogDirection(user.Id)); ret.Users.Add(userObj); } - //TODO: figure out paging here. - should there be any? ret.PageSize = 20; - ret.PageNumber = 1; + ret.PageNumber = (int)Math.Ceiling((double)ret.Users.Count/(double)ret.PageSize); return ret; } @@ -175,12 +144,6 @@ namespace SQLiteRepository foreach (var card in cards) { ret.AssociatedIdentifiers.Add(IdentifierConverter.ConvertToIdentifierDto(card)); - //new Identifier - //{ - // UniqueId = card.CardUId, - // IsAssociatedToUser = true, - // Id = card.Id - //}); } } catch (Exception ex) @@ -262,60 +225,35 @@ namespace SQLiteRepository public void ClearUnassignedIdentifiers() { + var unassignedIdentifiers = _connection.Query(SQLiteProcedures.GET_UNASSIGNED_CARD_LIST); + + //remove the logs from the timelog db + _connection.Query( + SQLiteProcedures.FormatInQuery(SQLiteProcedures.DELETE_TIMELOG_ENTRIES, + unassignedIdentifiers.Select(x => x.Id.ToString()).ToList())); + + //now remove the unassigned identifiers/cards _connection.Execute( SQLiteProcedures.CLEAR_UNASSIGNED_CARDS, Constants.UNASSIGNED_CARD_USER_ID); } - //TODO: Check time logs table on update to ensure associated cards/unique identifiers are removed/added as appropriate. public OperationResponse UpdateUser(User user, out int userIdResult) { var ret = OperationResponse.NONE; - var cardIds = new List(); //Get a list of current associated identifiers, convert into a list of Identifier Objects.. var currentCards = _connection.Query(SQLiteProcedures.GET_CARDS_BY_USER_ID, user.UserId) .Select(IdentifierConverter.ConvertToIdentifierDto); var cardsToRemove = currentCards.Except(user.AssociatedIdentifiers).Select(x => x.Id).ToList(); - - #region GetUnique Identifiers - foreach (var card in user.AssociatedIdentifiers) - { - var existingCard = _connection.Query( - SQLiteProcedures.GET_CARDS_BY_UNIQUE_ID, card.UniqueId); - - if (!existingCard.Any()) - { - var cardInsert = IdentifierConverter.ConvertFromIdentifierDto(card, Constants.UNASSIGNED_CARD_USER_ID); - _connection.Insert(cardInsert); - cardIds.Add(cardInsert.Id); - if (ret < OperationResponse.CREATED) - ret = OperationResponse.CREATED; //only change it if my status supercedes. - } - else - { - cardIds.Add(existingCard.First().Id); - if (ret < OperationResponse.UPDATED) - ret = OperationResponse.UPDATED; - } - } - #endregion - + #region Update/Create User int userId; if (user.UserId != -1) { - //edit.. - _connection.Query( - SQLiteProcedures.UPDATE_USER_DETAILS, - user.FirstName, - user.LastName, - user.HoursPerWeek, - user.IsContractor, - user.UserId - ); + UpdateUserDetails(user.FirstName, user.LastName, user.HoursPerWeek, user.IsContractor, user.UserId); userId = user.UserId; } else @@ -328,21 +266,34 @@ namespace SQLiteRepository } #endregion - #region Update Card/Unique Id entries. - foreach (var cardId in cardIds) + #region GetUnique Identifiers + + var existingCards = _connection.Query(SQLiteProcedures.FormatInQuery( + SQLiteProcedures.GET_CARDS_BY_UNIQUE_ID_LIST, + user.AssociatedIdentifiers.Select(x => x.UniqueId.ToString()).ToList())); + + foreach (var card in user.AssociatedIdentifiers) { - _connection.Query( - SQLiteProcedures.UPDATE_CARD_USER_ID, - userId, cardId); - } - foreach (var card in cardsToRemove) - { - _connection.Query( - SQLiteProcedures.UPDATE_CARD_USER_ID, - Constants.UNASSIGNED_CARD_USER_ID, card); + if (existingCards.All(x => x.CardUId != card.UniqueId)) + { + //this card is not currently in the system.. + var cardInsert = IdentifierConverter.ConvertFromIdentifierDto(card, user.UserId); + _connection.Insert(cardInsert); + existingCards.Add(cardInsert); + if (ret < OperationResponse.CREATED) + ret = OperationResponse.CREATED; //only change it if my status supercedes. + } } #endregion + #region Update Card/Unique Id entries/associations + //make sure all identifiers are associated to the right user + UpdateIdentifierUserId(existingCards.Select(x=>x.Id).ToList(), userId); + + //make sure to remove all identifiers that have been deselected in edit are removed from user + UpdateIdentifierUserId(cardsToRemove, Constants.UNASSIGNED_CARD_USER_ID); + #endregion + #region Update Group Associations SetUserGroups(userId, user.Groups.Where(x => x.IsAssociatedToUser).ToList()); @@ -375,11 +326,11 @@ namespace SQLiteRepository ret.Direction = LogDirection.UNKNOWN; return ret; } - else - { - //TODO: log when more than one comes back. should NEVER happen but.... - ident = cardIdQuery.First(); - } + + ident = cardIdQuery.First(); + _logger.Warn("More than 1 Identifier returned with ID {0}, card ids returned: {2}", + ident.CardUId, + string.Join(",", cardIdQuery.Select(x => x.CardUId).ToList())); #endregion // Get The User Direction (are they going in or out)? @@ -432,11 +383,15 @@ namespace SQLiteRepository ret.ProcessResponse = OperationResponse.SUCCESS; return ret; } - - /*Groups*/ - //TODO: check group name can only be entered once. + public OperationResponse CreateGroup(Group group, out int groupId) { + var query = _connection.Query(SQLiteProcedures.GET_GROUP_BY_NAME, group.Name); + if (query.Any()) + { + groupId = query[0].GroupId; + return OperationResponse.NONE; + } var groupDb = new GroupDb { GroupName = group.Name }; var resp = _connection.Insert(groupDb); groupId = groupDb.GroupId; @@ -487,7 +442,7 @@ namespace SQLiteRepository public OperationResponse UpdateGroup(Group group) { - var resp = _connection.Query(SQLiteProcedures.UPDATE_GROUP, group.Name, group.Id); + _connection.Query(SQLiteProcedures.UPDATE_GROUP, @group.Name, @group.Id); return OperationResponse.UPDATED; } @@ -553,6 +508,34 @@ namespace SQLiteRepository return OperationResponse.UPDATED; } + private int GetUserCount() + { + return _connection.ExecuteScalar(SQLiteProcedures.GET_TOTAL_USER_COUNT); + } + + private List GetUserList(int groupId = -1, int pageSize=-1, int pageNumber=-1) + { + List users; + if (pageNumber != -1 && pageSize != -1) + { + users = _connection.Query(SQLiteProcedures.GET_ALL_USERS_PAGINATE, + pageSize, (pageNumber - 1) * pageSize); + } + else if (groupId != -1) + { + users = + _connection.Query( + SQLiteProcedures.GET_ALL_USERS_BY_GROUP, + groupId); + } + else + { + users = _connection.Query(SQLiteProcedures.GET_ALL_USERS); + } + + return users; + } + private void CheckForDbUpgrade() { var data = _connection.Query(SQLiteProcedures.GET_DB_VERSION); @@ -576,20 +559,21 @@ namespace SQLiteRepository private void ExecuteUpgradeFromVersion(string installedVersion) { - const string PREFIX = "SQLiteRepository.UpgradeScripts."; var instVers = new Version(installedVersion); //so we have established that each script for upgrade will be .sql, so now start at lowest version and work up! var assembly = Assembly.GetExecutingAssembly(); var upgradeScripts = assembly.GetManifestResourceNames() - .Where(x=>x.StartsWith(PREFIX)) - .OrderBy(x=>new Version(Path.GetFileNameWithoutExtension(x.Replace(PREFIX, "")))) - .Where(x => - { - var scriptVersion = new Version(Path.GetFileNameWithoutExtension(x.Replace(PREFIX, ""))).CompareTo(instVers); - return scriptVersion > 0; - } - ) - .ToList(); + .Where(x => x.StartsWith(UPGRADE_SCRIPT_PREFIX)) + .OrderBy(x => + new Version(Path.GetFileNameWithoutExtension(x.Replace(UPGRADE_SCRIPT_PREFIX, string.Empty)))) + .Where(x => + { + var scriptVersion = + new Version(Path.GetFileNameWithoutExtension(x.Replace(UPGRADE_SCRIPT_PREFIX, string.Empty))) + .CompareTo(instVers); + return scriptVersion > 0; + }) + .ToList(); //now have an ordered list of upgrade script files, so execute in order! foreach (var upgradeScript in upgradeScripts) { @@ -878,5 +862,29 @@ namespace SQLiteRepository } } } + + private void UpdateIdentifierUserId(List cardsToUpdate, int userId) + { + foreach (var card in cardsToUpdate) + { + _connection.Query( + SQLiteProcedures.UPDATE_CARD_USER_ID, + userId, card); + } + } + + private void UpdateUserDetails(string firstName, string lastName, float hoursPerWeek, bool isContractor, + int userId) + { + _connection.Query( + SQLiteProcedures.UPDATE_USER_DETAILS, + firstName, + lastName, + hoursPerWeek, + isContractor, + userId + ); + } + } }