Checking Synchronization issue in SQL? I have the following SQL. I want to make that the SP should work under load. Means it should not have any synchronization issues.

ALTER PROCEDURE [dbo].[DeleteOldDeviceID]
(
    @OldDeviceID VARCHAR(500)
    ,@NewDeviceID  VARCHAR(500)
)
AS
BEGIN
SET NOCOUNT ON;
DECLARE @TranCount INT;
SET @TranCount = @@TRANCOUNT;

BEGIN TRY
    IF @TranCount = 0
        BEGIN TRANSACTION
    ELSE
        SAVE TRANSACTION DeleteOldDeviceID;

    IF @NewDeviceID <> '-1' AND NOT EXISTS(SELECT 1 FROM DeviceCatalog WHERE [UniqueID] = @NewDeviceID)
        BEGIN
        INSERT INTO [DeviceCatalog]
                ([os]
                ,[uniqueid]
                ,[address]
                ,[location]
                ,[culture]
                ,[city]
                ,[country]
                ,[other]
                ,[lastmodifieddate]
                ,[createddate]
                ,[IsActive]
                ,[IPAddress]
                ,[NativeDeviceID]
                ,[IsDeleted])
        SELECT  [os]
            ,@NewDeviceID
            ,[address]
            ,[location]
            ,[culture]
            ,[city]
            ,[country]
            ,[other]
            ,GETDATE()
            ,GETDATE()
            ,[IsActive]
            ,[IPAddress]
            ,[NativeDeviceID]
            ,[IsDeleted]
        FROM    [DeviceCatalog]
        WHERE   [UniqueID] = @OldDeviceID;
    END
    DELETE FROM DeviceCatalog WHERE [UniqueID] = @OldDeviceID; -- Always Delete old one
        LBEXIT:
    IF @TranCount = 0
        COMMIT;
END TRY
BEGIN CATCH
    DECLARE @Error INT, @Message VARCHAR(4000), @XState INT;
    SELECT  @Error = ERROR_NUMBER() ,@Message = ERROR_MESSAGE() ,@XState = XACT_STATE();

        IF @XState = -1
            ROLLBACK;
        IF @XState = 1 AND @TranCount = 0
            rollback
        IF @XState = 1 AND @TranCount > 0
            ROLLBACK TRANSACTION DeleteOldDeviceID;

        RAISERROR ('DeleteOldDeviceID: %d: %s', 16, 1, @error, @message) ;
    END CATCH
 END

Edit:, Is this equal to above SQL?

ALTER PROCEDURE [dbo].[DeleteOldDeviceID]
(
    @OldDeviceID VARCHAR(500)
    ,@NewDeviceID  VARCHAR(500)
)
AS
BEGIN
    SET NOCOUNT ON;
    DECLARE @TranCount INT;
    SET @TranCount = @@TRANCOUNT;

    BEGIN TRY
        IF @TranCount = 0
            BEGIN TRANSACTION
        ELSE
            SAVE TRANSACTION DeleteOldDeviceID;

        IF @NewDeviceID <> '-1' AND NOT EXISTS(SELECT 1 FROM DeviceCatalog WHERE [UniqueID] = @NewDeviceID)
            BEGIN
                UPDATE  [DeviceCatalog]
                SET [UniqueID] = @NewDeviceID
                WHERE   [UniqueID] = @OldDeviceID;
            END
        ELSE
        BEGIN
            DELETE FROM DeviceCatalog WHERE [UniqueID] = @OldDeviceID; -- Always Delete old one
        END
        LBEXIT:
            IF @TranCount = 0
                COMMIT;
    END TRY
    BEGIN CATCH
        DECLARE @Error INT, @Message VARCHAR(4000), @XState INT;
        SELECT  @Error = ERROR_NUMBER() ,@Message = ERROR_MESSAGE() ,@XState = XACT_STATE();
            IF @XState = -1
                ROLLBACK;
            IF @XState = 1 AND @TranCount = 0
                ROLLBACK
            IF @XState = 1 AND @TranCount > 0
                ROLLBACK TRANSACTION DeleteOldDeviceID;

            RAISERROR ('DeleteOldDeviceID: %d: %s', 16, 1, @error, @message) ;
    END CATCH
END

No, the Second Query won't Delete anything.

In the statement prior to the delete you change the oldID to the newID so there aren't any records with the oldID in that table.

Difference between the two Queries

The first Query, inserts new records into the table, then deletes the records with the oldID

The Second Query, updates all the records removing the oldID's and replacing it with the newID's, so instead of inserting and then deleting the old records, it updates the old records UniqueID column

These both do very different things.


Iterate through series of timers I wrote a short program for my exercise routine. It takes a JSON array of exercise names and duration (seconds) and puts it on the screen. The JSON looks like this:

var exerciseArray = [
    {
        "desc":       "Pushup/Pullup",
        "countdown":  30
    }, {
        "desc":       "Squat punch",
        "countdown":  45
    }, {
        "desc":       "Jumping jacks",
        "countdown":  60
    }
];

The function works, but it seems to be to be kind of a kludge. My problem was that I couldn't figure out how work with setInterval to iterate through the list of exercises (exerciseArray). What I did in the end is just decrement the countdown value, and when it's zero, move to the next item in exerciseArray (i++;). When I hit the end of the array — as measured by i and exerciseArray.length — I just run clearInterval and I'm done. Code below (I removed some of the extraneous stuff that just updates the HTML with the number of seconds, you can see the full thing on github):

function exerciseTimer(exerciseArray) {
    var i = 0;
    var exerciseObject = exerciseArray[i];
    // do stuff
    var tt = setInterval(function() {
        var exerciseObject = exerciseArray[i];
        // write html to the page
        exerciseObject.countdown = exerciseObject.countdown - 1;
        if (exerciseObject.countdown <= 0) {
            if(i < (exerciseArray.length - 1)) {
                i++;
            } else {
                clearInterval(tt);
            }
        }
    }, 100);
}

My question is, what is the appropriate way to iterate while using setInterval? Is there a cleaner way? This feels very ugly to me.

If you only need the entries once, you could simply keep using shift until there are no more entries:

var exercises = [
    {
        "desc":       "Pushup/Pullup",
        "countdown":  30
    }, {
        "desc":       "Squat punch",
        "countdown":  45
    }, {
        "desc":       "Jumping jacks",
        "countdown":  60
    }
];

function exerciseTimer(exercises) {
    // do stuff
    var exercise = exercises.shift(),
        tt = setInterval(function() {
            // write html to the page
            exercise.countdown = exercise.countdown - 1;
            //Does it work ?
            console.log( exercise );
            if (exercise.countdown <= 0) {
                exercise = exercises.shift();
                if(!exercise) {
                    clearInterval(tt);
                }
            }
        }, 100);
    };

exerciseTimer( exercises );

I would also suggest to not make the type of the variable part of the variable name. exercise and exercises are much easier to read than exerciseArray and exerciseObject.


How to simplify this ES6 Array code I'm just wondering Is there any ways I could simplify the following code without making it too difficult:

Actually I want to extract specific year data from content node based on lastNumberOfYears variable.

Example: Content array has 2019,2018,2017,2016 years data and lastNumberOfYears value is 2 then output should have only 2019, 2018 data. other scenario is if content array is like 2020, 2014, 2013 then output should have 2020, 2014 data (if lastNumberOfYears value is 2)

My solution

Step 1 : Define predefined data

const content = [{
    "document": "/content/path/document1.pdf",
    "date": "2020-02-20"
  }, {
    "document": "/content/path/file.docx",
    "date": "2019-02-20"
  }, {
    "document": "/content/abc/document2.docx",
    "date": "2018-06-20"
  }, {
    "document": "/document/order/order.pdf",
    "date": "2020-06-20"
  }, {
    "document": "/content/order/update.pdf",
    "date": "2018-03-21"
  }, {
    "document": "/content/author/author.pdf",
    "date": "2017-03-21"
  }, {
    "document": "/content/path/dummy.pdf",
    "date": "2016-02-15"
  }, {
    "document": "/content/dummy/dummy.docx",
    "date": "2015-12-15"
}];

let lastNumberOfYears = 2;
let selectedYearArray = [];

Step 2: Convert content data into year base data

const result = content.reduce((current, value) => {
  const dateYear = new Date(value.date).getFullYear();
  current[dateYear] = current[dateYear] || [];
  current[dateYear].push(value);
  return current;
}, {});

/*
output is like
{
   "2015":[{...}],"2016":[{}],..."2020":[{}, {}]
}
*/

Step 3: Put specific year data in same order for result array

Object.values(result).forEach(value => value.reverse());

Step 4: Create Year array from object key

const yearsArray = Object.keys(result).sort((first, second) => (first > second ? -1 : 1));

Step 5: Create last number of year array using lastNumberOfYears variable
Step 6: Extract Step 5 data from result array define in step 3
Step 7: Remove year key and join all array value of Step 6 outcome

if (yearsArray.length !== 0 && lastNumberOfYears <= yearsArray.length) {
  for (let i = 0; i < lastNumberOfYears; i++) {
    selectedYearArray.push(yearsArray[i]);
  }

  /*
    Step 5: console.log(selectedYearArray);
  */

  const filteredResult = Object.keys(result)
    .filter(key => selectedYearArray.includes(key))
    .reduce((obj, key) => {
      obj[key] = result[key];
      return obj;
    }, {});

  /*
    Step 6: console.log(filteredResult);
  */

  contentObj = [].concat.apply([],Object.values(filteredResult));

  /*
    Step 7:
  */
  console.log(contentObj.reverse());
}

Output:

[{
    "document": "/content/path/document1.pdf",
    "date": "2020-02-20"
}, {
    "document": "/document/order/order.pdf",
    "date": "2020-06-20"
}, {
    "document": "/content/path/file.docx",
    "date": "2019-02-20"
}]

Here's a scheme that appears to be about 7 times faster that yours when run in node.js:

function getData2(content, lastNumberOfYears) {
    // organize data by year into a Map object
    const map = new Map()
    for (let item of content) {
        const year = new Date(item.date).getFullYear();
        let yearData = map.get(year);
        if (yearData) {
            yearData.push(item);
        } else {
            map.set(year, [item]);
        }
    }

    /* map object data looks like this:
        Map {
          2020 => [
            { document: '/content/path/document1.pdf', date: '2020-02-20' },
            { document: '/document/order/order.pdf', date: '2020-06-20' }
          ],
          2019 => [ { document: '/content/path/file.docx', date: '2019-02-20' } ],
          2018 => [
            { document: '/content/abc/document2.docx', date: '2018-06-20' },
            { document: '/content/order/update.pdf', date: '2018-03-21' }
          ],
          2017 => [ { document: '/content/author/author.pdf', date: '2017-03-21' } ],
          2016 => [ { document: '/content/path/dummy.pdf', date: '2016-02-15' } ],
          2015 => [ { document: '/content/dummy/dummy.docx', date: '2015-12-15' } ]
        }
    */

    // get lastNumberOfYears by getting all the years, sorting them
    // and getting most recent ones
    let sortedYears = Array.from(map.keys()).sort();
    if (sortedYears.length > lastNumberOfYears) {
        sortedYears = sortedYears.slice(-lastNumberOfYears);
    }
    let result = [];
    for (let year of sortedYears) {
        result.push(map.get(year));
    }
    return result.flat();

}

function time(fn) {
    const start = process.hrtime.bigint();
    let result = fn();
    const end = process.hrtime.bigint();
    console.log(result);
    console.log(`Benchmark took ${end - start} nanoseconds`);
}

It uses the following steps:

Parse years and put each year's into a Map object with the year as the key and the value as an array of values for that year
Get the years from the Map object, sort them and get the desired lastNumberOfYears
Collect the results from the Map object for each of the chosen years and flatten them into a single array

If you want to run both yours and mine in node.js, here's the entire program I used to compare:

const inputData = [{
    "document": "/content/path/document1.pdf",
    "date": "2020-02-20"
  }, {
    "document": "/content/path/file.docx",
    "date": "2019-02-20"
  }, {
    "document": "/content/abc/document2.docx",
    "date": "2018-06-20"
  }, {
    "document": "/document/order/order.pdf",
    "date": "2020-06-20"
  }, {
    "document": "/content/order/update.pdf",
    "date": "2018-03-21"
  }, {
    "document": "/content/author/author.pdf",
    "date": "2017-03-21"
  }, {
    "document": "/content/path/dummy.pdf",
    "date": "2016-02-15"
  }, {
    "document": "/content/dummy/dummy.docx",
    "date": "2015-12-15"
}];

function getData1(content, lastNumberOfYears) {

    let selectedYearArray = [];

    const result = content.reduce((current, value) => {
      const dateYear = new Date(value.date).getFullYear();
      current[dateYear] = current[dateYear] || [];
      current[dateYear].push(value);
      return current;
    }, {});

    /*
    output is like
    {
       "2015":[{...}],"2016":[{}],..."2020":[{}, {}]
    }
    */

    Object.values(result).forEach(value => value.reverse());

    const yearsArray = Object.keys(result).sort((first, second) => (first > second ? -1 : 1));

    if (yearsArray.length !== 0 && lastNumberOfYears <= yearsArray.length) {
      for (let i = 0; i < lastNumberOfYears; i++) {
        selectedYearArray.push(yearsArray[i]);
      }

      /*
        Step 5: console.log(selectedYearArray);
      */

      const filteredResult = Object.keys(result)
        .filter(key => selectedYearArray.includes(key))
        .reduce((obj, key) => {
          obj[key] = result[key];
          return obj;
        }, {});

      /*
        Step 6: console.log(filteredResult);
      */

      contentObj = [].concat.apply([],Object.values(filteredResult));

      /*
        Step 7:
      */
      return contentObj.reverse();
    }
}

function getData2(content, lastNumberOfYears) {
    // organize data by year into a Map object
    const map = new Map()
    for (let item of content) {
        const year = new Date(item.date).getFullYear();
        let yearData = map.get(year);
        if (yearData) {
            yearData.push(item);
        } else {
            map.set(year, [item]);
        }
    }

    // get lastNumberOfYears by getting all the years, sorting them
    // and getting most recent ones
    let sortedYears = Array.from(map.keys()).sort();
    if (sortedYears.length > lastNumberOfYears) {
        sortedYears = sortedYears.slice(-lastNumberOfYears);
    }
    let result = [];
    for (let year of sortedYears) {
        result.push(map.get(year));
    }
    return result.flat();

}

function time(fn) {
    const start = process.hrtime.bigint();
    let result = fn();
    const end = process.hrtime.bigint();
    console.log(result);
    console.log(`Benchmark took ${end - start} nanoseconds`);
}

time(() => getData1(inputData, 2));

time(() => getData2(inputData, 2));

And, the output on my desktop computer was this:

[
  { document: '/content/path/document1.pdf', date: '2020-02-20' },
  { document: '/document/order/order.pdf', date: '2020-06-20' },
  { document: '/content/path/file.docx', date: '2019-02-20' }
]
Benchmark took 602501 nanoseconds
[
  { document: '/content/path/file.docx', date: '2019-02-20' },
  { document: '/content/path/document1.pdf', date: '2020-02-20' },
  { document: '/document/order/order.pdf', date: '2020-06-20' }
]
Benchmark took 89100 nanoseconds

Note a slightly different sort order of the results.  I didn't see in your description if a specific sort order of results was required or not.


API using NodeJS to recive data from client and send it to a server Hello I have a NodeJS which act like an interface it connects the client(mobile app) to the actual server
My node JS API receives data from the client to verify it and send it to the server.

I want to know does this code looks good?
Is that how API should be written in NodeJS?
Does this code violate any of the SOLID principles?
How can I improve this code?

const axios = require("axios")
require("express-async-errors")
const bodyParser = require("body-parser")
const express = require("express")

const LOLTrackingSystem = require("../methods/onlineGamesTracking/LOLTracking")
const getUserData = require("../methods/leagueOfLegends/getUserData")
//======[Gizmo Operator Settings]=======

// userId = token

//========[Gizmo API URL]==========
apiRoute = (api) => {
  api.use(bodyParser.json())
  // api.use(bodyParser.urlencoded({ extended: false }));
  // api.use(express.json());

  api.post("/api/auth", (req, res) => {
    const clientAuth = {
      username: req.body.username,
      password: req.body.password
    }

    const loginValidationURL = `http://${process.env.HOST}/api/users/${clientAuth.username}/${clientAuth.password}/valid`

    axios
      .get(loginValidationURL, { headers: { Authorization: process.env.AUTH } })
      .then((response) => {
        let token = response.data.result.identity.userId
        response.data.result.result == 0
          ? res.json({ status: "Authenticated", result: 1, token: hash })
          : res.json({ status: "Wrong Username or Password", result: 0 })
      })
      .catch((error) => {
        console.log("error " + error)
        res.json({
          status: `Couldn't reach Arena Gaming Server. Try again later`,
          result: 404
        })
      })
  })

  api.post("/api/memberProfile", (req, res) => {
    var token = req.headers["authorization"]
    // bcrypt.compare(token, hash).then(function(res) {
    //     // res == true
    // });
    const memberProfileURL = `http://${process.env.HOST}/api/users/${encryptedId}`

    axios
      .get(memberProfileURL, { headers: { Authorization: process.env.AUTH } })
      .then((response) => {
        response.data
      })
      .catch((error) => {
        console.log("error " + error)
        res.json({
          status: `Couldn't reach Arena Gaming Server. Try again later`,
          result: 404
        })
      })
  })
  //=============================================================================
  // Gizmo Related API
  //=============================================================================
  // MemberState Get The UserId & HostId when member login or logout of Gizmo
  api.post("/api/gizmo/memberState/:userId/:host/:state", async (req, res) => {
    const member = {
      state: req.params.state,
      userId: req.params.userId,
      hostComputer: req.params.host
    }
    if (Number(member.state)) {
      console.log(
        "[Logged In] Member with userId: " +
          member.userId +
          " To HostId: " +
          member.hostComputer +
          "[Sent From GizmoServer]"
      )
      await getUserData(member.userId, async (subscribed) => {
        // Check if the user is subscribed to the tracking system or not
        //*TODO* Add a list of currently tracking members that we can see
        if (subscribed) {
          LOLTrackingSystem(member.userId) // Start tracking that member
          res.json("[Started] Member " + member.userId + " Tracking")
        } else {
          res.json(
            "[Started] Member " +
              member.userId +
              " is not subscribed to the tracking system"
          )
        }
      })
    } else {
      console.log(
        "[Logged Out] Member with userId: " +
          member.userId +
          " From HostId: " +
          member.hostComputer +
          "[Sent From GizmoServer]"
      )
    }
  })
}

module.exports = apiRoute

Thanks

the code has plenty of room for improvement, and yes it violates the solid principals like the  Single responsibility principle "your functions are way too big and the module in itself has too many responsibilities ",  to clarify there are no rules on how API should be written in NodeJS there are code practices and design patterns that help us developers write more maintainable code, so don't try to follow every single rule you will get stuck in optimizing and it really depends on your project needs.
Now for the improvement here are some suggestions you can work on :

separate your endpoints in different files, take a look at my answer here, you can use the express router
move your constant variables like "loginValidationURL " in a different file like config.js then import and use it
don't overuse URL params use post request to get objects to your server
Build the application with a framework independent mindset
Make your functions lighter separate them by  roles like this take this example on your "/api/auth endpoint" it will explain many things

// Your implimation

api.post("/api/auth", (req, res) => {
  const clientAuth = {
    username: req.body.username,
    password: req.body.password
  };

  const loginValidationURL = `http://${process.env.HOST}/api/users/${clientAuth.username}/${clientAuth.password}/valid`;

  axios
    .get(loginValidationURL, { headers: { Authorization: process.env.AUTH } })
    .then(response => {
      let token = response.data.result.identity.userId;
      response.data.result.result == 0
        ? res.json({ status: "Authenticated", result: 1, token: hash })
        : res.json({ status: "Wrong Username or Password", result: 0 });
    })
    .catch(error => {
      console.log("error " + error);
      res.json({
        status: `Couldn't reach Arena Gaming Server. Try again later`,
        result: 404
      });
    });
});

/* Api Handler
 * file path : routes/api/client.js
 * in this file i only write the client api endpoints /client/auth /client/logout /client/:id the basic crud
 * i have seperate the main job in a controller module like that the api is mucj cleanter
 */
const express = require("express");
const router = express.Router();
const { authClient } = require("../controller/clientcontroller");

router.post("/client/auth", (req, res) => {
  authClient(req, res)
});

/* controller
 * file path : routes/controller/clientcontroller.js
 * in this file i only write the client controllers for each api call
 */

const { extractClientAuth } = require("./api/client/reqHandler"); // this module will be responsible for extracting and validation req objects
const { loginValidationUrlBuilder } = require("../../config"); // this module is responsible for all your static and config objects like axios custom headers
const { apiClient } = require("./services/externalJobs"); // this for exrtenal jobs and api calls
const { responseBuilder } = require("./api/tools"); // this module for creating response objects

function authClient(req, res) {
  const clientAuth = extractClientAuth(req);
  const loginValidationURL = loginValidationUrlBuilder(clientAuth);
  apiClient
    .login(loginValidationURL)
    .then(response => res.json(responseBuilder.authSuccess(response)))
    .catch(error => {
      res.json(responseBuilder.authFailed(error));
    });
}

this is by no mean the best way it's just a way to structures your project take from it what you like another this I can recommend this video on youtube channel Dev Mastry he does an excellent job of explaining how to create a scalable node application using express and MongoDB https://www.youtube.com/watch?v=fy6-LSE_zjI


PostgreSQL simple query with repeated function calls I have PostgreSQL 9.1.1 running in a shared hosting environment and I'm running this query:

SELECT a.id,
CASE WHEN a.idpai IS NULL THEN a.nome
ELSE concat(a.nome, ' (', b.nome, ')')
END AS nome, a.idpai
FROM localidade a
LEFT OUTER JOIN localidade b ON a.idpai = b.id
WHERE a.idcidade = :idcidade AND normalizar(
    CASE WHEN a.idpai IS NULL THEN a.nome
    ELSE concat(a.nome, ' (', b.nome, ')')
    END
) LIKE normalizar(:nome)
ORDER BY CASE WHEN normalizar(
    CASE WHEN a.idpai IS NULL THEN a.nome
    ELSE concat(a.nome, ' (', b.nome, ')')
    END
) LIKE substring(normalizar(:nome) from 2) THEN 1
ELSE 2
END, a.nome, b.nome
LIMIT 15

The performance is pretty good, the query returns in less than 50ms. Though, I'm repeating this part 3 times inside the query:

CASE WHEN a.idpai IS NULL THEN a.nome
ELSE concat(a.nome, ' (', b.nome, ')')
END

I believe this reduces the maintainability of the query and is not DRY at all. What is the recommended way to handle that? Should I create a function in my schema solely for this query? Or just store it in a variable and concatenate into the query in my back end before sending it over to the db server? Or is there any better way around?

I believe what this query does isn't the main point here, but just to give some background:

The localidade table may or may not have a relation with itself  through the idpai which references a "parent" register. Assuming this table format:

id | nome | idpai
 1 | foo  |
 2 | bar  |   1

So if the query placeholder :nome contains %foo%, it will return:

id | nome       | idpai
 1 | foo        |
 2 | bar (foo)  |   1

The ORDER BY clause is just to display results which the query parameter matches the beginning of the returned row's name before those who don't - SUBSTRING('%foo%' FROM 2) returns foo% -, and then the matching and not matching groups are ordered by a.nome and b.nome ASC as the SQL shows.

normalizar is a STABLE function that takes a string as parameter and calls LOWER() and removes accentuation returning the new string, so I can perform case-insensitive and accent-insensitive string comparison. Similar to unaccent which I couldn't properly apply on my use case due to some encoding problems (page is UTF8 and DB is latin1, if I convert the UTF8 to latin1 solely on the back end I'll have broken UTF8 pages; well this has been solved with the function above and is off-topic).

idcidade is just a foreign key which I use to reduce the result set to less than 0.1% of the table. Hence performance isn't really an issue in this specific case.

My real question is, does calling CASE multiple times passing the exact same parameters have an impact on performance? I assume the result from CASE should be STABLE and the query will be optimized automatically then. If that's the case, I can store part of the query into a variable on my back end and build the query string using that. Otherwise, if calling CASE multiple times in the same query with the same arguments affects performance, should I create a STABLE function for it then or what's the correct approach to this? Any pointers are appreciated.

I think this part:

CASE WHEN a.idpai IS NULL THEN a.nome
ELSE concat(a.nome, ' (', b.nome, ')')
END AS nome

Can be replaced with:

CONCAT(a.nome,  ' (' || b.nome || ')') AS nome

If b.nome is NULL, then "' (' || b.nome || ')'" will also be NULL (concatenating strings using '||' with NULL values will result in NULL. CONCAT() ignore null values, so will return only a.none in this case

In other words: if b.nome is present, it will output "a.nome (b.nome)", otherwise "a.nome"

You can test it yourself, here's my sqlfiddle:

http://sqlfiddle.com/#!1/d41d8/744/0

Also, you can try to use a subselect to prevent having to repeat this CONCAT;

http://sqlfiddle.com/#!1/a9571/7/0

I haven checked if this improves performance, but it reduces the amount of repeated code and may be better maintainable


HTTP Get & Post code with slow performance I had developed an application in which I hit several URLs and show data on mobile. But the problem is that it requires more time. If I check the same URL on Firebug tool I get a response in 2-3 seconds, but the same URL requires 8-15 seconds on mobile.  In my Android application I am using HTTP get and post method as follows:

public InputStream httpGetResponseStream(String url, String request) {
connectionUrl = url;
InputStream response = null;
try {
    processGetRequest();
    HttpResponse httpresponse = httpclient.execute(host, get);
    response = httpresponse.getEntity().getContent();
} catch (Exception e) {
    response = null;
}
return response;
}

public InputStream httpPostResponseStream(String url, String request) {
connectionUrl = url;
InputStream response = null;
try {
    processPostRequest();
    if (request != null) {
        InputStreamEntity streamEntity = new InputStreamEntity(
                new ByteArrayInputStream(request.getBytes()), request
                        .length());
        post.setEntity(streamEntity);
    }
    HttpResponse httpresponse = httpclient.execute(host, post);
    response = httpresponse.getEntity().getContent();
}catch (Exception e) {
    response = null;
}
return response;
}

private void processGetRequest(){
    uri = Uri.parse(connectionUrl);
    String protocol = connectionUrl.substring(0, 5).toLowerCase();
    if (protocol.startsWith("https")) {
        securedConnection = true;
    } else {
        securedConnection = false;
    }
    if (securedConnection) {
        host = new HttpHost(uri.getHost(), 443, uri.getScheme());
    } else {
        host = new HttpHost(uri.getHost(), 80, uri.getScheme());
    }
    get = new HttpGet(uri.getPath()+query_string);
}

private void processPostRequest() {
uri = Uri.parse(connectionUrl);
String protocol = connectionUrl.substring(0, 5).toLowerCase();
if (protocol.startsWith("https")) {
    securedConnection = true;
} else {
    securedConnection = false;
}
if (securedConnection) {
    host = new HttpHost(uri.getHost(), 443, uri.getScheme());
} else {
    host = new HttpHost(uri.getHost(), 80, uri.getScheme());
}
post = new HttpPost(uri.getPath());
}

Any suggestion to improve code will be appreciated

I personally haven't used anything HTTP-related in Java yet, and thus can't talk about your performance. What I can see though is the following code:

if(protocol.startsWith("https"){
    securedConnection = true;
} else {
    securedConnection = false;
}

I will assume you have securedConnection as a private classwide field, so I won't talk about the fact that you only use it to determine the port in the method scope.

the fun thing is, your code could be written in one line:

securedConnection = protocol.startsWith("https");

if you now inline protocol (Eclipse Alt + Shift + i) you get following line:

securedConnection = connectionUrl.substring(0,5).toLowerCase().startsWith("https");

you see that the call to substring is unneccesary.

You also have a large duplication in itself with the methods processPostRequest() and processGetRequest. they are actually exactly the same apart from the last line:

get = new HttpGet(uri.getPath()+query_string);
post = new HttpPost(uri.getPath());

you might want to move them into a method like this:

private enum Type{
    GET, POST
}

private HttpRequest processRequestOfType(Type t){
   uri = Uri.parse(connectionUrl);
   securedConnection = connectionUrl.toLowerCase().startsWith("https");
   host = securedConnection ? new HttpHost(uri.getHost(), 443, uri.getScheme())
              : new HttpHost(uri.getHost(), 80, uri.getScheme());
   request = t == Type.GET ? new HttpGet(uri.getPath()+query_string)
              : new HttpPost(uri.getPath());
   return request;
}

Lastly I saw that in your method httpGetResponseStream() you have the parameter request, which is never used in the code you posted.


Finding world-writable files in local directories only Over at Stack Overflow I asked how to scan a local filesystem ignoring all remote mount points. I only really wanted a push in the right direction and received it. After clacking away on my keyboard I think I might have what I what I need. I ask for an evaluation to determine if it fulfills the requirements:

Scan every local directory for world-writable files ignoring "special" directories (/sys, /proc, /dev)
Ignore directories
Ignore files on any remote filesystems which may be mounted
Ignore any files in an "exclude" list

I'm quite certain it could be drastically optimized and any suggestions are welcome as are any suggestions for a better algorithm. However, rewriting the script is secondary to performing the task properly.

I've tested it and I'm fairly confident that I've created what I want. I just need some extra eyes to confirm.

#!/usr/bin/perl

# Directives which establish our execution environment
use warnings;
use strict;
use Fcntl ':mode';
use File::Find;
no warnings 'File::Find';
no warnings 'uninitialized';

# Variables used throughout the script
my $DIR = "/var/log/tivoli/";
my $MTAB = "/etc/mtab";
my $PERMFILE = "world_writable_w_files.txt";
my $TMPFILE = "world_writable_files.tmp";
my $EXCLUDE = "/usr/local/etc/world_writable_excludes.txt";
my $ROOT = "/";
my @devNum;

# Create an array of the file stats for "/"
my @rootStats = stat("${ROOT}");

# Compile a list of mountpoints that need to be scanned
my @mounts;

open MT, "<${MTAB}" or die "Cannot open ${MTAB}, $!";

# We only want the local mountpoints
while (<MT>) {
  if ($_ =~ /ext[34]/) {
    my @line = split;
    push(@mounts, $line[1]);

  }

}

close MT;

# Read in the list of excluded files and create a regex from them
my $regExcld = do {
  open XCLD, "<${EXCLUDE}" or die "Cannot open ${EXCLUDE}, $!\n";
  my @ignore = <XCLD>;
  chomp @ignore;
  local $" = '|';
  qr/@ignore/;

};

# Create a regex containing mountpoint dev numbers to compare file device numbers to.
my $devRegex = do {
  chomp @devNum;
  local $" = '|';
  qr/@devNum/;

};

# Create the output file path if it doesn't already exist.
mkdir("${DIR}" or die "Cannot execute mkdir on ${DIR}, $!") unless (-d "${DIR}");

# Create our filehandle for writing our findings
open WWFILE, ">${DIR}${TMPFILE}" or die "Cannot open ${DIR}${TMPFILE}, $!";

foreach (@mounts) {
  # The anonymous subroutine which is executed by File::Find
  find sub {
    # Is it in a basic (non-special) directory, ...
    return if $File::Find::dir =~ /sys|proc|dev/;

    # ...a regular file, ...
    return unless -f;

    # ...local, ...
    my @dirStats = stat($File::Find::name);
    return unless $dirStats[0] =~ $devRegex;

    # ...and world writable?
    return unless (((stat)[2] & S_IWUSR) && ((stat)[2] & S_IWGRP) && ((stat)[2] & S_IWOTH));

    # Add the file to the list of found world writable files unless it is
    # in the list if exclusions
    print(WWFILE, "$File::Find::name\n") unless ($File::Find::name =~ $regExcld);

  }, $_;

}

close WWFILE;

# If no world-writable files have been found ${TMPFILE} should be zero-size;
# Delete it so Tivoli won't alert
if (-z "${DIR}${TMPFILE}") {
  unlink "${DIR}${TMPFILE}";

} else {
  rename("${DIR}${TMPFILE}","${DIR}${PERMFILE}") or die "Cannot rename file ${DIR}${TMPFILE}, $!";

}

Processing mount points

This can be improved in many ways:

# Compile a list of mountpoints that need to be scanned
my @mounts;
open MT, "<${MTAB}" or die "Cannot open ${MTAB}, $!";

# We only want the local mountpoints
while (<MT>) {
  if ($_ =~ /ext[34]/) {
    my @line = split;
    push(@mounts, $line[1]);
  }
}

close MT;

Using bare names for file handles is not recommended:
replace MT with my $MT, and <MT> with <$MT>.

The comment talks about local mount points.
That's too vague, when in fact you're looking specifically for ext3 and ext4 filesystems.

You could simplify if ($_ =~ /ext[34]/) as if (/ext[34]/).

You could put the entire while loop inside a map, and it will be still readable.

Putting it together, you can rewrite like this:

open my $MT, "<${MTAB}" or die "Cannot open ${MTAB}, $!";

# Compile a list of mountpoints that need to be scanned
# We only want the ext3 ext4 partitions
my @mounts = map((split)[1], grep(/ext[34]/, <$MT>));

close $MT;

Note that if you didn't find any mount points,
the script has nothing to process, so it should exit:

die 'no mount points to process' unless @mounts;

Try to apply these techniques everywhere in the script,
especially the bit about filehandlers.

# Read in the list of excluded files and create a regex from them
my $regExcld = do {
  open XCLD, "<${EXCLUDE}" or die "Cannot open ${EXCLUDE}, $!\n";
  my @ignore = <XCLD>;
  chomp @ignore;
  local $" = '|';
  qr/@ignore/;
};

As earlier, replace XCLD with my $XCLD or something.
You also forgot to close it.

Btw, a common way to shorten my @arr = <$fh>; chomp @arr as chomp(my @arr = <$fh>).

Most importantly,
should it be really a fatal error if the file with excluded patterns doesn't exist?
It might make sense to make this optional.
Perhaps issue a warning instead, something like this:

my $regExcld = do {
  if (open my $fh, "<${EXCLUDE}") {
      chomp(my @ignore = <$fh>);
      close($fh);
      local $" = '|';
      qr/@ignore/;
  } else {
      warn "Cannot open ${EXCLUDE}, $!\n";
      undef;
  }
};

This bit seems pointless:

# Create a regex containing mountpoint dev numbers to compare file device numbers to.
my $devRegex = do {
  chomp @devNum;
  local $" = '|';
  qr/@devNum/;
};

Because @devNum is an empty array in your script,
you never actually gave it any values.

This is quite strange:

# Create the output file path if it doesn't already exist.
mkdir("${DIR}" or die "Cannot execute mkdir on ${DIR}, $!") unless (-d "${DIR}");

You misplaced the die:
it will run if "${DIR}" is empty,
not if mkdir fails.

Also, there's no need whatsoever to double-quote ${DIR}.

Corrected and simplified:

mkdir($DIR) or die "Cannot execute mkdir on ${DIR}, $!" unless -d $DIR;


Update user country based on IPv4 decimal address I have UserSignatures, a table with clientIPs, some of which have CountryIds that are null and need to be filled in.

A second table, IpLookups, stores IPs, and a third table Country has the needed CountryId. This is workaround at the moment, because we don't fill CountryIp yet.

declare @var1 int,@var2 int,@var3 int,@ind int = 0;

SELECT * INTO #Temp
FROM [dbo].[UserSignatures]
where CountryId is null

SELECT @var1 = count(*) FROM #Temp;

while (@ind < @var1  )
begin
      select @var3 = Id from #Temp order by Id OFFSET @ind ROWS FETCH NEXT 1 ROW ONLY;

      select @var2 = CONVERT(BIGINT, PARSENAME(u.ClientIp, 1)) * POWER(256, 0)
  + CONVERT(BIGINT, PARSENAME(u.ClientIp, 2)) * POWER(256, 1)
  + CONVERT(BIGINT, PARSENAME(u.ClientIp, 3)) * POWER(256, 2)
  + CONVERT(BIGINT, PARSENAME(u.ClientIp, 4)) * POWER(256, 3) from [dbo].[UserSignatures] u
  where u.Id = @var3

      update [dbo].[UserSignatures]
        set CountryId = (
        SELECT c.Id FROM dbo.IpLookups lo with(NOLOCK)
          inner join
        [dbo].[Countries] c with(nolock)
        on
        c.Code = lo.CountryIsoCode
         WHERE
        @var2
          BETWEEN lo.[From] AND lo.[To]
        )
        where Id = @var3

      SET @ind = @ind+1;
end

DROP table #Temp

Is there any way to simplify this?

You process data Row By Agonizing Row instead of a single Update statement.

There's no need for temp tables, variables and a loop, simply write a single SELECT and use this in an UPDATE FROM. This should return the correct country ids:

    SELECT u.id, c.Id
    FROM [dbo].[UserSignatures] AS u
    JOIN dbo.IpLookups AS lo
    ON CONVERT(BIGINT, PARSENAME(u.ClientIp, 1)) * POWER(256, 0)
     + CONVERT(BIGINT, PARSENAME(u.ClientIp, 2)) * POWER(256, 1)
     + CONVERT(BIGINT, PARSENAME(u.ClientIp, 3)) * POWER(256, 2)
     + CONVERT(BIGINT, PARSENAME(u.ClientIp, 4)) * POWER(256, 3)
       BETWEEN lo.[FROM] AND lo.[TO]
    JOIN [dbo].[Countries] AS c
    ON c.Code = lo.CountryIsoCode
    WHERE u.CountryId IS NULL


Find triangles from line I have some lines that form triangles. I'd like to challenge the fastest way to find all triangles.

In particular the code should take an ArrayList of Line2D object and return an ArrayList of Triangle2D. The only constrain is to use p1.distanceToSquared(p2) < 1) as equal function.
Note:
My code does not work because it contains some triangles more time (10 triangles instead of 8), i.e. I didn't managed to write the triangle comparator.
This is my Java code (using this lib that has some standard 2D objects):
private ArrayList<Triangle2D> findTrinaglesTromLines(ArrayList<Line2D> lines) {

    class TriangleComparator implements Comparator<Triangle2D>{
        @Override
        public int compare(Triangle2D t1, Triangle2D t2) {
            TreeSet<Vec2D> pts1 = new TreeSet<Vec2D>();
            pts1.add(t1.a); pts1.add(t1.b); pts1.add(t1.c);

            TreeSet<Vec2D> pts2 = new TreeSet<Vec2D>();
            pts2.add(t2.a); pts2.add(t2.b); pts2.add(t2.c);

            if (pts1.containsAll(pts2)) return 0;
            else return 1;
        }
    }

    TreeSet<Triangle2D> triangles = new TreeSet<Triangle2D>(new TriangleComparator());
    for(int i=0; i<lines.size(); i++) {
        Line2D l1, l2, l3;
        l1 = lines.get(i);
        for (int j=0; j<lines.size(); j++) {
            Line2D m = lines.get(j);
            if(l1==m)continue;
            if(shareVertex(l1, m)) {
                l2 = m;
                for(int k=0; k<lines.size(); k++) {
                    Line2D n = lines.get(k);
                    if(l1==n || l2==n) continue;
                    l3 = n;
                    if( shareVertex(l1, l3) || shareVertex(l2, l3) ) {
                        Triangle2D t = getTraingle(l1, l2, l3);
                        if( t!= null ) {
                            triangles.add(t);
                            println(t.toString());
                        }
                    }
                }
            }
        }
    }
    return new ArrayList<Triangle2D>(triangles);
}

private boolean shareVertex(Line2D l1, Line2D m) {
    if( l1.a.distanceToSquared(m.a) < 1) return true;
    if( l1.a.distanceToSquared(m.b) < 1) return true;
    if( l1.b.distanceToSquared(m.b) < 1) return true;
    return false;
}

private Triangle2D getTraingle(Line2D a, Line2D b, Line2D c) {
    TreeSet<Vec2D> pts = new TreeSet<Vec2D>();
    pts.add(a.a); pts.add(a.b);
    pts.add(b.a); pts.add(b.b);
    pts.add(c.a); pts.add(c.b);
    if(pts.size()!=3) return null;
    Iterator<Vec2D> iterator = pts.iterator();
    Triangle2D t = new Triangle2D();
    t.a = iterator.next();
    t.b = iterator.next();
    t.c = iterator.next();
    return t;
}

Here the lines (begin point, end point):
[ {x:568.051, y:286.209}, {x:505.477, y:411.501} ]
[ {x:505.477, y:411.501}, {x:696.271, y:462.066} ]
[ {x:696.271, y:462.066}, {x:568.051, y:286.209} ]
[ {x:696.271, y:462.066}, {x:672.033, y:213.228} ]
[ {x:672.033, y:213.228}, {x:568.051, y:286.209} ]
[ {x:145.374, y:290.304}, {x:176.77, y:455.729} ]
[ {x:176.77, y:455.729}, {x:332.868, y:284.48} ]
[ {x:332.868, y:284.48}, {x:145.374, y:290.304} ]
[ {x:332.868, y:284.48}, {x:191.991, y:192.966} ]
[ {x:191.991, y:192.966}, {x:145.374, y:290.304} ]
[ {x:176.77, y:455.729}, {x:505.477, y:411.501} ]
[ {x:505.477, y:411.501}, {x:332.868, y:284.48} ]
[ {x:261.672, y:132.32}, {x:574.957, y:159.359} ]
[ {x:568.051, y:286.209}, {x:574.957, y:159.359} ]
[ {x:332.868, y:284.48}, {x:568.051, y:286.209} ]
[ {x:261.672, y:132.32}, {x:332.868, y:284.48} ]
[ {x:332.868, y:284.48}, {x:574.957, y:159.359} ]

I suggest that you start by considering what it means for two points to be the same vertex.

In the toxiclibs library, a Vec2D consists of an x- and y-coordinate, each stored as a float.  In shareVertex(), you've defined two Vec2Ds to be the "same" point if they are less than one unit apart.  (Using == to compare floats is generally a bad idea, so setting some threshold for equality is good, though 1.0 might be a bit sloppy relative to the precision that a float is capable of.)

However, in getTraingle() [sic], you use a different criterion for equality of Vec2Ds.  You're relying on a TreeSet<Vec2D> to do the de-duplication for you.  In the absence of any comparator given to the TreeSet constructor, it will use Vec2D.compareTo().  Vec2D.compareTo() is a bit weird: it compares vectors by their distance from the origin (the weirdness being that if two points are the same distance from the origin, the ordering will be somewhat arbitrary, thus breaking the Comparable consistency requirement — but that's of no concern to you).  What does matter to you, though, are:

All you care about is equality, but you'll end up doing a lot of wasted magnitude calculations instead.
Your criterion for equality in shareVertex() (less than one unit separation) is inconsistent with the criterion for equality in Vec2D.compareTo() (strict float equality).

Once you've figured out which endpoints of the line segments coincide, the actual coordinates no longer matter.  At that point, you would be better off treating this as a problem in graph theory rather than one in computational-geometry.  Your algorithm is currently O(n3) due to the three nested loops.  You could probably do better with a smarter algorithm.


Finding whether a set and an array have any strings in common I have the following code.  It looks through an array, called items, and checks to see if there are any items in that array that also exist in a certain set of strings, called stringSet.

    Set<String> stringSet = new HashSet<>();
    stringSet.add("A");
    stringSet.add("B");
    stringSet.add("C");

    String[] items = new String[2];
    items[0] = "A";
    items[1] = "D";

    boolean hasValue = false;

    for (String item : items) {
        if (stringSet.contains(item)) {
            hasValue = true;
        }
    }
    System.err.println(hasValue);

Is there any way to avoid using a for each loop to achieve same result in Java 8, like with a stream, a map, etc.?

You can use the following stream in place of for-loop

Arrays.stream(items)
            .anyMatch(stringSet::contains);

What you are trying to do is loop through the items, and any time the item is contained in the string set u want to return true.
anyMatch function breaks as soon as any of the items returns true


Validator for vertices of a polygon There is an object which has few properties. I wrote a function for test cases which check if the properties are satisfied or not. The function should throw an exception if the property is not satisfied.

But, I think there are better ways to write those test cases. A subset of the property of the object is as follows:

  x['polygon'] is a list of >= 3 integer (x,y) pairs representing the
  corners of the polygon in clockwise or anticlockwise order.

Current function is as follows:

def validate_object(x):
    """This function validates an object x that is supposed to represent an object
    inside an image, and throws an exception on failure.
    Specifically it is checking that:
      x['polygon'] is a list of >= 3 integer (x,y) pairs representing the corners
                    of the polygon in clockwise or anticlockwise order.
    """
    if type(x) != dict:
        raise ValueError('dict type input required.')

    if 'polygon' not in x:
        raise ValueError('polygon object required.')

    if not isinstance(x['polygon'], (list,)):
        raise ValueError('list type polygon object required.')

    points_list = x['polygon']
    if len(points_list) < 3:
        raise ValueError('More than two points required.')

    for x, y in points_list:
        if type(x) != int or type(y) != int:
            raise ValueError('integer (x,y) pairs required.')

    return

It would be really helpful if someone could please suggest better ways of writing those test cases.

In Python, if you do not specify the return value of a function, then by default it will return None. So no need to hard code return in your case.

You do not need to create the extra variable points_list  as you can directly write:

  if len(x['polygon']) < 3:
        raise ValueError('More than two points required.')

and:

 for x, y in x['polygon']:
    if type(x) != int or type(y) != int:
        raise ValueError('integer (x,y) pairs required.')

Of course, choosing a meaningful and significant name for the object x will be better.

When we check if stuff in some_iterable we get True or False. I mean that a ValueError exception is not the most suitable thing that can happen here.

isnstance() returns True or False, and may raise a TypeError exception under certain conditions (check the link), so here also raising a ValueError may not be appropriate.

Because when you raise the exception, you communicate to the user only what he needs to know (through the messages you specified for each case) and because of the last 2 points I mentioned previously, I suggest you to create a custom exception which message can be overridden for each situation:

class InvalidPolygonObject(Exception):
   pass

And your function can be written this way:

def validate_object(polygon_object):
    if type(polygon_object) != dict:
        raise InvalidPolygonObject('dict type input required.')

    if 'polygon' not in polygon_object:
        raise InvalidPolygonObject('polygon object required.')

    if not isinstance(polygon_object['polygon'], (list,)):
        raise InvalidPolygonObject('list type polygon object required.')

    if len(polygon_object['polygon']) < 3:
        raise InvalidPolygonObject('More than two points required.')

    for x, y in polygon_object['polygon']:
        if type(x) != int or type(y) != int:
            raise InvalidPolygonObject('integer (x,y) pairs required.')


Filter data with optional parameters I would make a filter to filter my data using next parameters:

First name
Last name
Language
Profile
A minimum date of registration
A maximum date of registration
Success

All filter options are not required for the user. Here could you find my C# code.

private IEnumerable<Registration> Filter(string voornaam,   // first name
                                         string achternaam, // last name
                                         int? language,
                                         int? profiel,      // profile
                                         DateTime? vanaf,   // min date
                                         DateTime? tot,     // max date
                                         bool? geslaagd)    // success
{
    IEnumerable<Registration> all = All(); // <-- this gets all registrations from
                                           //       the database.

    if (!string.IsNullOrEmpty(voornaam))
    {
        all = (from r in all
               where r.FirstName.ToLower().StartsWith(voornaam.ToLower().Trim())
               select r).ToList();
    }

    if (!string.IsNullOrEmpty(achternaam))
    {
        all = (from r in all
               where r.LastName.ToLower().StartsWith(achternaam.ToLower().Trim())
               select r).ToList();
    }

    if (language != null)
    {
        all = (from r in all
               where r.LanguageId == language.Value
               select r).ToList();
    }

    if (profiel != null)
    {
        all = (from r in all
               where r.ProfileId == profiel.Value
               select r).ToList();
    }

    if (vanaf != null)
    {
        all = (from r in all
               where r.RegistrationDate >= vanaf.Value
               select r).ToList();
    }

    if (tot != null)
    {
        all = (from r in all
               where r.RegistrationDate <= tot.Value
               select r).ToList();
    }

    if (geslaagd != null)
    {
        all = (from r in all
               where r.Success == geslaagd.Value
               select r).ToList();
    }

    return all;
}

The function All() gets all the registrations from the database. Next I check if the attributes are not null (or empty for the strings) and if true I set the variable all equal to the return values of the linq query's. After all I return all and load my view.

This code works for all possibilities and got a low fail percentage. Of course my code is too long and will make this code shorter. How could I do this?

IEnumerable<Registration> all = All(); // <-- this gets all registrations from
                                       //       the database.

This looks like a really bad idea. Why are you getting all data from the database first and not filter it before retrieving?

where r.FirstName.ToLower().StartsWith(voornaam.ToLower().Trim())

Most databases are case insensitive so you wouldn't need this formatting adjustment if you used server side filtering.

On the client it's better to use the StrigComparer.XIgnoreCase where the X is either the InvariantCulture or Ordinal then ToUpper/Lower because on the one hand it better communicates the intention that you are performing a case-insensitive comparison (which is not that obvious if you use ToUpper/Lower - you might be just fixing the case for whatever reason) and on the other hand you don't have to change the case twice which might introduce a new bug if you call ToUpper for the first string and ToLower for the other one or forget it altogether.

If you use EF or any other ORM you should filter the data on the server and not after downloading everything. With large databases this would be such a waste of resources.

If All is backed by Entity Framework then you could return an IQueryable and still take the advantange of a server query.

Why don't you use english variables names? Everything is in english but the parameters. This is not consistant... and it's difficult to get help with local names.


Nested layouts - BoxLayout inside BorderLayout I have this code:

JPanel controlPanel = new JPanel();
controlPanel.setLayout(new BoxLayout(controlPanel, BoxLayout.PAGE_AXIS));
TitledBorder tb2 = BorderFactory.createTitledBorder(null, "Control Panel",
        TitledBorder.CENTER, TitledBorder.DEFAULT_POSITION, myFont, new Color(0, 153, 0));
controlPanel.setBorder(tb2);

JPanel fromDate = new JPanel();
fromDate.setLayout(new BoxLayout(fromDate, BoxLayout.X_AXIS));
fromDate.add(new JLabel("From date: "));
JButton fromDateButton = new JButton("...");
fromDateButton.setMaximumSize(new Dimension(100,15));
fromDate.add(fromDateButton);
JPanel toDate = new JPanel();
toDate.setLayout(new BoxLayout(toDate, BoxLayout.X_AXIS));
toDate.add(new JLabel("Until date: "));
JButton toDateButton = new JButton("...");
toDateButton.setMaximumSize(new Dimension(100,15));
toDate.add(toDateButton);
controlPanel.add(Box.createRigidArea(new Dimension(0,10)));
controlPanel.add(fromDate);
controlPanel.add(Box.createRigidArea(new Dimension(0,10)));
controlPanel.add(toDate);
toDate.setAlignmentX(Component.LEFT_ALIGNMENT);
fromDate.setAlignmentX(Component.LEFT_ALIGNMENT);

gui.add(controlPanel, BorderLayout.WEST);

Which produce this GUI (The relevant part is shown):

I want to have a label and a button right next to it. There should be a better way to achieve this. Any suggestions?

When you create a form, a GridBagLayout is usually needed.

Here's one way to create the form in your question.

import java.awt.GridBagConstraints;
import java.awt.GridBagLayout;
import java.awt.Insets;

import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.SwingUtilities;

public class SimpleGridBagLayout implements Runnable {

    @Override
    public void run() {
        JFrame frame = new JFrame("GridBagLayout Test");
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);

        JPanel panel = new JPanel();
        panel.setLayout(new GridBagLayout());

        GridBagConstraints gbc = new GridBagConstraints();
        gbc.insets = new Insets(10, 10, 10, 10);
        gbc.gridwidth = 2;
        gbc.gridx = 0;
        gbc.gridy = 0;

        JLabel titleLabel = new JLabel("Control Panel");
        panel.add(titleLabel, gbc);

        gbc.anchor = GridBagConstraints.LINE_START;
        gbc.fill = GridBagConstraints.HORIZONTAL;
        gbc.gridwidth = 1;
        gbc.gridy++;

        JLabel fromDateLabel = new JLabel("From date:");
        panel.add(fromDateLabel, gbc);

        gbc.gridx++;

        JButton fromDateButton = new JButton("...");
        panel.add(fromDateButton, gbc);

        gbc.gridx = 0;
        gbc.gridy++;

        JLabel toDateLabel = new JLabel("To date:");
        panel.add(toDateLabel, gbc);

        gbc.gridx++;

        JButton toDateButton = new JButton("...");
        panel.add(toDateButton, gbc);

        frame.add(panel);
        frame.setSize(250, 200);
        frame.setVisible(true);
    }

    public static void main(String[] args) {
        SwingUtilities.invokeLater(new SimpleGridBagLayout());
    }

}


Construct a URL given an Enum I have a below code which constructs the URL given a FlowType enum.

private String getURL(FlowType type) throws Exception {
    StringBuilder url = new StringBuilder();

    if (TestUtils.isProduction()) {
        if (type.equals(FlowType.HOLDER)) {
            url.append(VISITOR);
        } else {
            url.append(USER);
        }
    } else {
        if (type.equals(FlowType.HOLDER)) {
            url.append(VISITOR_STAGE);
        } else {
            url.append(USER_STAGE);
        }
    }
    long version = 0;
    if (DataMapping.isInitialized()) {
        if (!TestUtils.isEmpty(DataMapping.getPartition(type))) {
            version = DataMapping.getPartition(type).getVersion();
        }
    }
    url.append("web/hasChanged?ver=" + version);

    return url.toString();
}

I wanted to review this code. Any improvements or suggestions?

Here are a few comments:

you don't seem to be using any instance fields/methods of the enclosing class. So it seems that you could make the method static
throwing Exception is not ideal - you should only throw the specific exceptions that the code can throw
finally the method does several things and you could extract each step in its own method

An initial refactoring could therefore look like:

private static String getURL(FlowType type) throws SpecificException {
  StringBuilder url = new StringBuilder();

  url.append(getUrlRoot(type));
  url.append("web/hasChanged?ver=");
  url.append(getVersion(type));

  return url.toString();
}

private static String getUrlRoot(FlowType type) {
  if (TestUtils.isProduction()) {
    return type == FlowType.HOLDER ? VISITOR : USER;
  } else {
    return type == FlowType.HOLDER ? VISITOR_STAGE : USER_STAGE;
  }
}

private static long getVersion(FlowType type) {
  if (DataMapping.isInitialized()) {
    Partition partition = DataMapping.getPartition(type);
    if (!TestUtils.isEmpty(partition)) return partition.getVersion();
  }
  return 0;
}

That leaves one code smell which is that you seem to access a lot of "static state" via the TestUtils and DataMapping classes. Without knowing what they do it's hard to tell if this is good or bad (likely: bad).

The getUrl method should probably not need to know that these static methods exist so I would try to remove the calls and pass the required information as an argument of the getUrl method.


Comparing prices with a webservice Working on a project that parses through an excel document to find price changes off of a webservice that is in place. The code works but is slower than expected. At first it was processing 28 products a minute, and now after some work I have it up to 80 products a minute.

I've used Jetbrains dottrace to determine the bottleneck is on this line of code

 xmlstr = wc.DownloadString(@"http://awebsiteservice/prices?SearchString=" + UPC);

I used System.Net.WebClient to make the calls thinking it was the fastest way.

I've done some research and do not know if there is a way to eliminate this bottleneck or not. I am posting the code below for general code review, and also in hopes that there is something simple I am missing that can help the API calls go faster or smoother.

 private static void PriceDifferences(ExcelWorksheet ws, int maxval)
    {
        bool looping = true;
        int i = 2;
        var wc = new WebClient();
        XmlDocument xml = new XmlDocument();
        string xmlstr;
        Console.WriteLine("Press X to save results and stop process");
        do
        {
            if (Console.KeyAvailable)
            {
                if (Console.ReadKey(true).Key == ConsoleKey.X)
                {
                    looping = false;
                }
            }

            if (ws.Cells[i, 8].Value != null)
            {
                if (DateTime.ParseExact(ws.Cells[i, 8].Value.ToString(), "M/dd/yyyy", null) >= DateTime.Now.AddDays(-5).Date)   //If analysis ran in last 5 days skip
                {
                    i++;
                    continue;
                }
            }

            var UPC = ws.Cells[i, 1].Value;
            var cPrice = ws.Cells[i, 2].Value;

            ws.Cells[i, 8].Value = DateTime.Now.Date.ToShortDateString();

            //Console.WriteLine("UPC is {0}, Current Price is {1}, and Lowest Price known is {2}", UPC, cPrice, lPrice);
            //Console.WriteLine("Checking for price differences");

            try
            {
                xmlstr = wc.DownloadString(@"http://awebsiteservice/prices?SearchString=" + UPC);
            }
            catch
            {
                continue;
            }

            xml.LoadXml(xmlstr);

            XmlNodeList refList = xml.GetElementsByTagName("Store");
            List<string> pricelist = new List<string>();

            GetTrustedPrices(refList, pricelist);
            UpdatePrices(ws, pricelist, i, cPrice);

            i++;
            Console.WriteLine(i);

        }
        while (looping && i < maxval);
    }

The functions GetTrustedPrices and UpdatePrices are below

private static void GetTrustedPrices(XmlNodeList refList, List<string> pricelist)
    {
        foreach (XmlNode node in refList)
        {
            if (node.ChildNodes[2].InnerText == "Trusted")
            {
                pricelist.Add(node.ChildNodes[4].ChildNodes[2].InnerText);
            }
        }
    }

private static void UpdatePrices(ExcelWorksheet ws, List<string> pricelist, int i, object cPrice)
    {
        List<double> result = pricelist.Select(x => double.Parse(x)).ToList();

        if (result.Count <= 0) return;
        result = result.Where(itm => itm >= 1).ToList();
        if (result.Count <= 0) return;

        var lowest = result.Min();
        if (!((double) cPrice > lowest)) return;
        ws.Cells[i, 6].Value = lowest;
        ws.Cells[i, 7].Value = lowest - (double) cPrice;
    }

Well you got a lot going on so we will take it one step at a time.  The improve the performance you will need to use the TPL.  This should allow things to start to process in parallel. Word of warning I didn't test all this code as I don't have the Excel sheet or website so there might be minor bugs in it

first you have a lot going on for one method we should break them out to each one does a specific thing.  Also to make things easier we going to create a class to instantiate so we can keep some state.

Lets create a couple small classes, I'd nest them in the main class, to hold some state for passing around.

First one is to hold the ExcelData we care about

private class ExcelDataSink
{
    public int ExcelPosition { get; set; }
    public string UPC { get; set; }
    public double CurrentPrice { get; set; }
}

This will be better for anyone coming after you.  Like them I had to assume cPrice is the current price.  Here it's self explanatory what it is.

Next is a class to hold the Pricing Data we get back from the WebSite plus the info we need to update the Excel Document.

private class PricingDataSink
{
    public int ExcelPosition { get; set; }
    public IList<double> Prices { get; set; }
    public double CurrentPrice { get; set; }
}

Since we are going to try processing in parallel we will want to put a rate limiter on it.  We don't want to flood a website with over 80 request a second.  I'm going to make a constant that you can test with and tinker to see what works best for your situation. Also going to switch to the HttpClient instead of the WebClient and hard code the URL in a constant as well.

public class Pricing
{
    private readonly HttpClient _httpClient = new HttpClient();
    private const string URL = @"http://awebsiteservice/prices?SearchString=";
    private const int InFlight= 5; // How many request do we want to process at once

We need to read from the Excel file and get use data.  That should be it's own method. Excel is single threaded so no point trying to parallel this code

/// <summary>
/// check if the price hasn't been updated in over 5 days and if not reads the current price and upc
/// </summary>
/// <param name="worksheet">Wrk Sheet to use</param>
/// <param name="maxValue">row to end with in Excel Worksheet</param>
/// <returns></returns>
private IEnumerable<ExcelDataSink> GetExcelData(ExcelWorksheet worksheet, int maxValue)
{
    for (var i = 2; i < maxValue; i++)
    {
        if (worksheet.Cells[1, 8].Value == null ||
            DateTime.ParseExact(worksheet.Cells[i, 8].Value.ToString(), "M/dd/yyyy", null) <
            DateTime.Now.AddDays(-5).Date) //If analysis ran in last 5 days skip
        {
            yield return new ExcelDataSink()
            {
                ExcelPosition = i,
                CurrentPrice = double.Parse(worksheet.Cells[i, 2].Value),
                UPC = worksheet.Cells[i, 1].Value
            };
        }
    }
}

Going to drop out the console stuff as the TPL standard way to cancel is with a CancellationToken  Plus this class should only care about what it needs to do and not about displaying progress or reading keyboard input.  If you need to show progress I would recommending accepting the IProgress Interface and using that for reporting.

Now the TPL DataFlow Blocks are good at making a mesh for this kind of thing so I'm going to use that.  There are other options I just find this easier to understand.

Going to use the BufferBlock to put the data we read from Excel in.  You can kind of think of this as the queue that holds the data. To load the data into the Block we will create another method and mark the buffer complete when we are done reading from Excel

/// <summary>
/// Loads data from Excel into the target Block
/// </summary>
/// <param name="sourceBlock"></param>
/// <param name="workSheet"></param>
/// <param name="maxValue"></param>
/// <param name="token"></param>
/// <returns></returns>
private async Task LoadData(ITargetBlock<ExcelDataSink> sourceBlock, ExcelWorksheet workSheet, int maxValue,
    CancellationToken token)
{
    foreach (var sink in GetExcelData(workSheet, maxValue))
    {
        await sourceBlock.SendAsync(sink, token);
    }

    sourceBlock.Complete();
}

Now we need to change the ExcelDataSink to the PricingDataSink and make the web call

/// <summary>
/// Retreives new pricing from website
/// </summary>
/// <param name="excelDataSink"></param>
/// <returns></returns>
private async Task<PricingDataSink> GetWebPricing(ExcelDataSink excelDataSink)
{
    var content = await _httpClient.GetStringAsync(URL + excelDataSink.UPC);

    var xml = new XmlDocument();
    xml.LoadXml(content);

    return new PricingDataSink()
    {
        CurrentPrice = excelDataSink.CurrentPrice,
        ExcelPosition = excelDataSink.ExcelPosition,
        Prices = GetTrustedPrices(xml).ToList()
    };
}

private IEnumerable<double> GetTrustedPrices(XmlDocument xmlDocument)
{
    return xmlDocument.GetElementsByTagName("Store").Cast<XmlNode>()
        .Where(node => node.ChildNodes[2].InnerText == "Trusted")
        .Select(node => XmlConvert.ToDouble(node.ChildNodes[4].ChildNodes[2].InnerText));
}

Now we need to update the prices in the Excel File.

/// <summary>
/// Update the Excel file with the new lower price
/// </summary>
/// <param name="pricingSink"></param>
/// <param name="worksheet"></param>
private static void UpdatePrices(PricingDataSink pricingSink, ExcelWorksheet worksheet)
{
    var lowest = pricingSink.Prices.Where(p => p >= 1).DefaultIfEmpty().Min();
    if (lowest >= 1 && lowest < pricingSink.CurrentPrice)
    {
        worksheet.Cells[pricingSink.ExcelPosition, 6].Value = lowest;
        worksheet.Cells[pricingSink.ExcelPosition, 7].Value = lowest - pricingSink.CurrentPrice;
        worksheet.Cells[pricingSink.ExcelPosition, 8].Value = DateTime.Now.Date.ToShortDateString();
    }
}

Now we just need to wire everything up.  Going to make a method called UpdatePrice for the main entry point.

public async Task UpdatePrice(ExcelWorksheet workSheet, int maxValue, CancellationToken token)
{
    // This acts like our queue
    var buffer = new BufferBlock<ExcelDataSink>(new DataflowBlockOptions()
    {
        BoundedCapacity = InFlight, // put a constraint to not dump all data in at once
        CancellationToken = token
    });

    // marking our consumer of the queue to process one items at a time
    var executionOptions = new ExecutionDataflowBlockOptions()
    {
        BoundedCapacity = 1,
        CancellationToken = token
    };

    // List of task running
    var inFlight = new List<Task>();
    var linkOption = new DataflowLinkOptions()
    {
        PropagateCompletion = true
    };

    // create a consumer for the number we want inflight
    for (var i = 0; i < InFlight; i++)
    {
        // Call our code to transform the Excel into the pricing data
        var transform = new TransformBlock<ExcelDataSink, PricingDataSink>(
            async e => await GetWebPricing(e), executionOptions);

        // Call the code to update the sheet
        var updateExcel = new ActionBlock<PricingDataSink>(p => UpdatePrices(p, workSheet), executionOptions);

        // wire the mesh together
        transform.LinkTo(updateExcel, linkOption);
        buffer.LinkTo(transform, linkOption);
        inFlight.Add(updateExcel.Completion);
    }

    // Start the load data task
    inFlight.Add(LoadData(buffer, workSheet, maxValue, token));

    // Wait for everything to be completed
    await Task.WhenAll(inFlight);
}

I would highly recommend reading up on TPL and the TPL DataFlow blocks to understand what is all happening.   To run the code you will need to create a CancellationTokenSource and if you want to cancel it you cancel that the token source.


Inherit SortedDictionary As an example, suppose I had a large amount of data about a set of Restaurants for a large set of Dates in a database that I need to analyze / output to the user.

I have a custom class that holds the data for each restaurant for each date:

Public Class DateData
    Public Property Var1 As Double = 0
    Public Property Var2 As Double = 0
    Public Property Var3 As Double = 0
    Public Property Var4 As Double = 0

    Public Sub Sub1()
      ....
    End Sub

   ... etc ....
End Class

And since I am getting this data for each date (and since date-order does matter for my calculations), I have the following class set up too (where most of the work / calculation is done):

Public Class RestaurantData
    Inherits SortedDictionary(Of Date, DateData)

   Public Property Name As String
   Public Property RestaurantLevelData1 As Double = 0
   Public Property RestaurantLevelData2 As Double = 0

   Public Sub New(ByVal strName As String, ByVal DatesList As List(Of Date))
       _Name = strName
       For Each daDate As Date In DatesList
           Me.Add(daDate)
       Next
   End Sub

   Public Overloads Sub Add(ByVal daDate As Date)
        MyBase.Add(daDate, New DateData)
   End Sub

   Public Sub Sub1()
       For i As Integer = 0 To Me.Keys.Count - 1
           Dim daDate As Date = Me.Keys(i)
           Me(daDate).Sub1()
           .... etc ....
       Next
   End Sub

   ... etc ....
End Class

And, finally, since this is for a large amount of restaurants, I have a class which holds this Dictionary on a restaurant levele:

Public Class RestaurantsDict
    Inherits Dictionary(Of String, RestaurantData)

   Public Overloads Sub Add(ByVal strName As String, ByVal Dates As List(Of Date))
       MyBase.Add(strName, New RestaurantData(strName, Dates))
   End Sub
End Class

I have 2 main questions:

Is this a good way to set this up? My original code consisted of dozens of

Dictionary(Of String, SortedDictionary(Of Date, Double))

But I am a fairly novice programmer and would love to know if there is a more efficient architecture for what I am trying to achieve.
The way I am currently setting this up is very hard to debug. For example, the Count property of my RestaurantData throws an exception of type System.TypeLoadException.

Inheriting from collections is rarely a good practice. In your case it's better to have a private collection (SortedDictionary in your case), and populate it as needed (in other words it's better to use delegation than inheritance here).

The reason why inheriting is not so good here is that your class will expose a lot of functionality that users of RestaurantData aren't actually interested in.


Load page content on hashchange I want to create a modular website with no frameworks or CMS nor server-side language.
Also, I am doing simple use of the hashchange event to help the navigation.

The structure would be as follows:

A basic container HTML page with 3 main areas: header, content, footer.
Individual HTML files - one for each page - which would contain only the HTML to be inserted in the content part
a JavaScript file with all the code needed
a CSS file with all the styling

Please bear in mind that all the code below has been cleaned to illustrate the concept: the real code is more complex (e.g. the content of the header and the footer might as well be loaded with AJAX calls, some manipulations might occur to the DOM after the content has been put in place, handling AJAX failures, and so on).

A sample version of the HTML for the container page would be like:

<!DOCTYPE html>

    <meta charset="utf-8"/>
    <title>Faux SPA</title>
    <link rel="stylesheet" href="styles.css"/>

    <button onclick="setPageHash('Page1')">View 1</button>
    <button onclick="setPageHash('Page2')">View 2</button>

A couple of sample HTML files for the content (obviously, the real-world content would be more complex):

Page1.html:

And Page2.html:

    Some text content

    second line of text content

Finally, the JavaScript code would be like:

var validHashes = ["Page1", "Page2"];
function page(pageName){
    var newPageURI = pageName + ".html";
    var container = "#content";
    $.ajax(newPageURI).done(
        function(content) {
            putContentInContainer(container, content);
            setPageHash(pageName);
        }
    );
}
function putContentInContainer(container, content) {
    $(container).html(content)
}
function loadPageFromHash() {
    var hashValue = window.location.hash || "";
    var hashIsValid;
    hashValue = hashValue.replace('#','');
    hashIsValid = (validHashes.indexOf(hashValue)>=0);
    if (hashIsValid) {
        page(hashValue);
    } else {
        console.error("no valid HASH!");
    }
}
function setPageHash(page) {
    window.location.hash = page;
}
window.onhashchange = loadPageFromHash;
$().ready(function() {
    loadPageFromHash();
});

So, my questions are:

Is there any disadvantage in loading the HTML this way?
I know that the HTML files with the content will not be valid HTML files because of the missing tags (e.g. body, html, etc.): would that represent a problem?
Is there anything else I am missing for the whole hashchange subject?

1) Loading HTML this way will exclude all your users with js disabled from browsing your site, that's why back-end scripting exists
2) They are not a problem if you inject them in a valid HTML document. If you don't, the majority of modern browsers will create html, head and body elements by themselves, but all the other browsers will possibly display nothing
3) Theoretically not


multiple if statements "forced" by database query result & external api response Following scenario:
this is the code that resides in Ajax Controller class.
It is responsible for handling input in one of the form fields.

The logic is as follows:

If user-inputted data validate as either Tax Number or Statistical Number, it's supposed to: check for presence of such data in database, and, depending on the result, either make a call to external API (in case DB query is empty) or present the results (in case of input being found in database).

If user-inputted data doesn't validate as either Tax or Statistical Number, it is supposed to perform DB query with input treated as the customer name.

Below code works as expected.
However, it is long and I was forced to repeat some parts.
I'm looking to cut down on lines while preserving exact same logic, but I'm highly unsure as to how to make it even more compressed.

class AjaxController {

  public function customerslistAction() {
    $filterChain = new Zend_Filter();
    $filterChain->addFilter(new Zend_Filter_Digits())
                ->addFilter(new Zend_Filter_Null());

    $input = $filterChain->filter($this->getRequest()->getParam('name'));
    $collection = new Def_Model_Collection_Customer();

    /* check if $input is TaxNumber */
    if (SugApi::isTaxNumber($input)) {
      $data['tax_number'] = $input;
      $allFound = $collection->findByTaxOrStatistical($data);

      // check and return customer with this TAX_NUMBER if it is in database
      if (count(($allFound)) > 0) {
        echo Zend_Json::encode($allFound);
        return;
      }
      // call external api if it isn't in database [catch NotFound Exception]
      else {
        try {
          $sugReport = SugApi::getByTaxNumber($input);
        } catch (\SugApi\Exception\NotFoundException $e) {
          $out['results'][] = array('value' => 'Unknown tax number.');
          echo Zend_Json::encode($out);
          return;
        }
      }
    }

    /* check if $input is StatisticalNumber */
    else if (SugApi::isStatNumber($input)) {
      $data['stat_number'] = $input;
      $allFound = $collection->findByTaxOrStatistical($data);

      // check and return customer with this STATISTICAL_NUMBER if it is in database
      if (count($allFound) > 0) {
        echo Zend_Json::encode($allFound);
        return;
      }
      // call external api if it isn't in database [catch NotFound Exception]
      else {
        try {
          $sugReport = SugApi::getByStatNumber($input);
        } catch (\SugApi\Exception\NotFoundException $e) {
          $out['results'][] = array('value' => 'Unknown statistical number.');
          echo Zend_Json::encode($out);
          return;
        }
      }
    }
    // if $input is neither TAX_NUMBER nor STATISTICAL_NUMBER,
    // query database for customer with $input as customer_name
    else {
      $collection = new Def_Model_Collection_Customer();
      $data = $collection->fetchAll(array('name' => $this->getRequest()->getParam('name')));

      $out = array();
      foreach ($data as $val) {
        $out['results'][] = array(
          'id' => $val['customer_id'],
          'value' => $val['customer_name'],
          'info' => 'Tax Number: '.$val['customer_tax_number']
        );
      }
      $json = Zend_Json::encode($out);
      echo $json;
      return;
    }

    // parse successful external api response into Customer object and save it
    $customer = new Def_Model_Customer();
    $customer->setOptionsFromSugReport($sugReport);
    $customer->save();

    $out['results'][] = array(
      'id'    => $customer->customer_id,
      'value' => $customer->customer_name,
      'info'  => 'Tax Number: '.$customer->customer_tax_number
    );
    echo Zend_Json::encode($out);
  }
}

If you don't want to repeat yourself (DRY) then consider using multiple methods, and set properties ($this->collection) and/or pass parameters, as necessary. If you are finding yourself nesting if statements inside a method, it is a good indication that you should be using multiple methods.

For example, for your method, it looks like you could cut the database check into their own method.

If you have a return, then there is no need for an else statement :-)

if($a){
  return;
}
// no need for else

There is no point in setting a variable $json and echo it, just echo it as you did most of the time, but not all.

It looks as though you could break the rest response into it's own method for example with the if count > 0 return true or false to determine the action.

private function method($results){
    if (count(($results)) > 0) {
        echo Zend_Json::encode($result);
        return true;
    }
    return false;
}

Check out this video "Your code sucks, let's fix it - By Rafael Dohms" to help with understanding the above.
https://www.youtube.com/watch?v=GtB5DAfOWMQ

I hope this is helpful!


Python OOP shopping cart I completed the following task:

  You need to create the foundations of an e-commerce engine for a B2C (business-to-consumer) retailer. You need to have a class for a customer
  called User, a class for items in inventory called Item, and a shopping cart class calledCart. Items go in Carts, and Users can have multiple Carts. Also, multiple items can go into Carts, including more than one of any single item.

I am a new Pythoner, and don't have good knowledge of OOP. I feel the code is not good. I welcome recommendations to improve the code.Here's my code:

class Item(object):
    def __init__(self,itemname,itemprice):
        self.__itemname = itemname
        self.__itemprice = itemprice

    def GetItemName(self):
        return self.__itemname

    def GetItemPrice(self):
        return self.__itemprice

    def ChangeItemPrice(self,newprcie):
        self.__itemprice = newprcie

class Cart(dict):      #cart dict format:  {itemname:[price,number]}
    def ShowCart(self):
        return self

class User(object):
    def __init__(self, name):
        self.name = name
        self.__cartlist = {}
        self.__cartlist[0] = Cart()

    def AddCart(self):
        self.__cartlist[len(self.__cartlist)] = Cart()

    def GetCart(self, cartindex = 0):
        return self.__cartlist[cartindex]

    def BuyItem(self, item, itemnum, cartindex = 0):
        try:
            self.__cartlist[cartindex][item.GetItemName()][1] += itemnum
        except:
            self.__cartlist[cartindex].update({item.GetItemName():[item.GetItemPrice(),itemnum]})

    def BuyCancle(self, itemname, itemnum, cartindex = 0):
        pass

if __name__ == '__main__':

    item1 = Item('apple', 7.8)
    item2 = Item('pear', 5)

    user1 = User('John')

    user1.BuyItem(item1, 5)
    print("user1 cart0 have: %s" % user1.GetCart(0).ShowCart())

    user1.BuyItem(item2, 6)
    print("user1 cart0 have: %s" % user1.GetCart(0).ShowCart())

    user1.AddCart()
    user1.BuyItem(item1, 5, 1)
    print("user1 cart1 have: %s" % user1.GetCart(1).ShowCart())

I have some notes on how you structured this, note that some are a bit Python specific.

Your attribute names should not have the class name in them. It's redundant because you'll always be accessing them from something else, ie. Item.name is simpler than Item.itemname. Also you shouldn't use double underscores to prefix the name. A Pythonic style is to use a single underscore to denote an attribute that should be treated as private and not directly accessed. But very often it's Python style to leave out any concept of setters and getters, instead just access them directly. Like this:

>>> apple = Item('apple', 7.8)
>>> print apple.name
apple
>>> apple.price = 7
>>> print apple.price
7

You could almost not have Item be a class, but your spec seems to require it so I wont suggest you change that.

Also you misspelled newitemprice as newitemprcie. You managed to make it consistent between the two (by copy pasting I assume) but keep an eye on little errors like that. They'll only cause headaches.

Your Cart is a little underdeveloped. All it does is subclass dict and then return itself when ShowCart is called? This is redundant since the Cart itself could be passed to places it's needed.

You mentioned in a comment you want to be able to do Cart['apple'] to get the count of apples, but an easier way to do that is to just have an attribute of cart that's a dictionary. For example:

class Cart:
    def __init__(self, items):
        self.items = items

Cart({"apple": 5, "pear": 7})
Cart.items["apple"]

This is a much clearer way to get at the items without needing to inherent a dictionary into the Cart itself.

Maybe the cart could more directly manage items so you could have useful features like counting how many of each item is there. As it stands, you don't do anything special with a cart that has multiple instances of the same item. Why not keep track of that so you could easier print to the user what they have, rather than expecting them to just read a list of items.

While we're on it. Why not implement a method or two to print the contents of a Cart nicer? Maybe the Cart should be where the user actually purchases things? You need to think about what a cart does, and create those functions.

Why make cartlist a dictionary? You should make it a list instead. You can still do cartlist.remove(cart_to_remove). Also don't name it cartlist especially if it's not a list, carts is better. Types don't need to be in names unless it's hard to tell what type a variable is.


Overriding static method with different static variable in java Suppose I have 3 classes, Animal, Dog, and Cat
public abstract class Animal {
    private static final List<Animal> allObjects = new ArrayList<>();

    public Animal(){
      allObjects.add(this);
    }

    // Returns immutable list of all Animal objects
    public static List<? extends Animal> getInstantiatedObjects(){
         return allObjects;
    }
}

public class Cat extends Animal {
    private static final List<Cat> allObjects = new ArrayList<>();

    public Cat(){
      super();
      allObjects.add(this);
    }

    // Returns immutable list of all Cat objects
    public static List<? extends Cat> getInstantiatedObjects(){
         return allObjects;
    }
}

While this works, I can't help but think that this is poor design, and it would be better if either the static method would be inheritable and "adapt" to the class (? extends thisClass), or it raised an implementation error if the method wasn't implemented. For instance,
public class Dog extends Animal {
    private static final List<Dog> allObjects = new ArrayList<>();

    public Dog(){
      super();
      allObjects.add(this);
    }

    /* Returns immutable list of all Dog objects
    public static List<? extends Dog> getInstantiatedObjects(){
         return allObjects;
    } */
}

returns no errors and simply uses the static method of the superclass, but that doesn't allow the code to store only objects of its class or lower. Any help would be greatly appreciated!

From time to time one sees this wish. I never needed it, but the way is to have one global repository in the base class:
public abstract class Animal {
    private static final Map<Class<? extends Animal>, List<Animal>> all =
            new HashMap<>();

    protected Animal() {
        List<Animal> species = all.computeIfAbsent(getClass(),
                type -> new ArrayList<>());
        species.add(this);
    }

    public static Stream<T extends Animal> all(Class<T> type) {
        return all.computeIfAbsent(getClass(),
               type -> new ArrayList<>()).stream().map(type::cast);
    }

Instead of storing the Animal child instance one could use a WeakReference<Animal> to let animals disappear when not used.
A call to Animal.all(Dog.class) needs no Animal instance.


Correct using of try/catch clause on database execution When a statement to a DB returns null, I am catching it this way:

private int GetLastRoundNumber(int period)
{
    int id;
    try
    {
        using (NpgsqlConnection con = new NpgsqlConnection(_entwConnection))
        {
            con.Open();
            using (NpgsqlCommand cmd = new NpgsqlCommand())
            {
                cmd.Connection = con;
                cmd.CommandText = "SELECT roundNumberId FROM race.folders WHERE period = @period ORDER BY roundNumberId DESC LIMIT 1;";
                cmd.Parameters.AddWithValue("period", period);
                return id = (int)cmd.ExecuteScalar();
            }
        }
    }
    catch (NullReferenceException e)
    {
        Debug.Write(e.Message);
        Debug.WriteLine("Because of a new period, started with 1 again. Period: " + period + ".");
        return id = 1;
    }
}

So to this codepart my questions:

Is it okay to have the try/catch block outside the using statement
block?
Is there a better way to handle the exception?
Do I need to hit the catch block?

I am asking because as I learned it by now, for me it isn't a real exception, because when a new period starts, there is no Id inside.

Is this the correct way to deal with it?

Some quick remarks:

Avoid writing ADO.NET code by hand; instead use an ORM like Dapper.
Why not use MAX() -- which I think is supported by PostgreSQL -- instead of ORDER BY roundNumberId DESC LIMIT 1?

SELECT MAX(column_name)
FROM table_name
WHERE condition;

Don't put business logic in exceptions. Instead, make this method int?
TryGetRoundNumberId() and let the calling code solve the issue when a null is returned.

Or perhaps provide a default, e.g. int TryGetRoundNumberId(int defaultValue), so you'll get something along these lines:

using (var sqlConnection = new SqlConnection(_connectionString))
{
    sqlConnection.Open();

    using (var sqlCommand = new SqlCommand())
    {
        // snip parameters etc.

        var result = (int?)sqlCommand.ExecuteScalar();
        if (result.HasValue)
        {
            return result.Value;
        }
        else
        {
            return defaultValue;
        }
    }
}


Recursively evaluate neighbors in a two dimensional grid I'm creating a puzzle mini game where there is a board of gems, varying in color, and chosen at random.

This is what my code generates:

When you click on a gem the game should traverse the board and find all connecting gems of the same color. When you click the orange square all orange square neighbors are selected.

Here is my Board class. The locateNeighbors() method recursively searches adjoining grid tiles until it reaches a non-matching gem or the edge of the board.

public class Board {

    public int rows, columns;
    public int x, y;
    private GemPool pool;
    private Gem[][] board;

    public Board(int rows, int columns) {

        x = 0;
        y = 0;

        this.rows = rows;
        this.columns = columns;

        board = new Gem[rows][columns];
        pool = new GemPool(COLOR.BLUE, COLOR.RED, COLOR.YELLOW, COLOR.GREEN, COLOR.ORANGE, COLOR.PURPLE);

        for(int i = 0; i < rows; i++) {
            for(int j = 0; j < columns; j++) {
                board[i][j] = pool.grabGem();
            }
        }

    }

    public Gem at(int row, int column) {

        return board[row][column];

    }

    public void draw(SpriteBatch batch) {

        for(int i = 0; i < rows; i++) {
            for(int j = 0; j < columns; j++) {
                String color = board[i][j].getColorName();
                //I'm aware of the magic numbers here
                batch.draw(pool.grabTexture(color), (i * 72) + x, (j * 72) + y);
            }
        }

    }

    public void dispose() {

        pool.dispose();

    }

    public void click(Vector3 touchPos) {

            //I'm aware of the magic numbers here
            int x = (int)(touchPos.x / 72);
            int y = (int)(touchPos.y / 72);

            if(x < rows && y < columns) {
                Gem g = this.at(x, y);
                HashSet<Point> matches = new HashSet<Point>();
                locateNeighbors(x, y, g.getColor(), matches);
                System.out.println(matches.size() + " matches found.");
            }

    }

    private void locateNeighbors(int x, int y, COLOR color, HashSet<Point> matches) {

        Point p = new Point(x, y);

        if(matches.contains(p))
        {
            return;
        }
        else
        {
            matches.add(p);
        }

        //Check north
        if(y + 1 < columns)
        {
            if(this.at(x, y + 1).getColor() == color)
                locateNeighbors(x, y + 1, color, matches);
        }

        //Check east
        if(x + 1 < rows)
        {
            if(this.at(x + 1, y).getColor() == color)
                locateNeighbors(x + 1, y, color, matches);
        }

        //Check south
        if(y - 1 >= 0)
        {
            if(this.at(x, y - 1).getColor() == color)
                locateNeighbors(x, y - 1, color, matches);
        }

        //Check west
        if(x - 1 >= 0)
        {
            if(this.at(x - 1, y).getColor() == color)
                locateNeighbors(x - 1, y, color, matches);
        }

    }

}

I would love your opinions on optimizations that could be made to the recursive method.

Avoid unnecessary calculations

It's a trivial performance improvement, but in

        //Check south
        if(y - 1 >= 0)
        {
            if(this.at(x, y - 1).getColor() == color)
                locateNeighbors(x, y - 1, color, matches);
        }

        //Check west
        if(x - 1 >= 0)
        {
            if(this.at(x - 1, y).getColor() == color)
                locateNeighbors(x - 1, y, color, matches);
        }

changing to

        //Check south
        if (y > 0)
        {
            if (this.at(x, y - 1).getColor() == color)
            {
                locateNeighbors(x, y - 1, color, matches);
            }
        }

        //Check west
        if (x > 0)
        {
            if (this.at(x - 1, y).getColor() == color)
            {
                locateNeighbors(x - 1, y, color, matches);
            }
        }

will be faster when the expression is false.  When it's true, it's unlikely to matter, as the compiler should optimize down to just one subtraction.  But when false, no subtractions are needed at all.  Of course, subtractions add a trivial amount of time in boards with just a few hundred squares.

Note:  I also added brackets around the single statement form if statements.  While the compiler is fine with the other syntax, it tends to lead to a coding error that can be hard to diagnose.  For that reason, many programmers always use the block form.

Recursion adds overhead

You could also try rewriting the recursive function iteratively.  Note that there is more state in a function call than you need to save.  The color doesn't change, and it always refers to the same matches Set.  Only the points change.

You don't provide all the code, so I can't test it.  But it would look something like.

        List<Point> candidates = new ArrayList<>();
        candidates.add(new Point(x, y));
        while (!candidates.isEmpty())
        {
            Point p = candidates.remove(candidates.size() - 1);
            if (matches.contains(p))
            {
                continue;
            }
            else
            {
                matches.add(p);
            }

            //Check north
            if (p.getY() + 1 < columns)
            {
                if(at(p.getX(), p.getY() + 1).getColor() == color)
                {
                    candidates.add(new Point (p.getX(), p.getY() + 1));
                }
            }

            //Check east
            if (p.getX() + 1 < rows)
            {
                if(at(p.getX() + 1, p.getY()).getColor() == color)
                {
                    candidates.add(new Point (p.getX() + 1, p.getY());
                }
            }

            //Check south
            if (p.getY() > 0)
            {
                if (at(p.getX(), p.getY() - 1).getColor() == color)
                {
                    candidates.add(new Point (p.getX(), p.getY() - 1);
                }
            }

            //Check west
            if (p.getX() > 0)
            {
                if (at(p.getX() - 1, p.getY()).getColor() == color)
                {
                    candidates.add(new Point (p.getX() - 1, p.getY());
                }
            }
        }

Note however that the compiler may optimize out the suboptimal portions of the original code.  In that case, this may not be faster.

You also should question whether this is the place in the program where optimizations are most helpful.  I would think that removing gems, adding new ones, and redrawing the board would take more time.  Shaving off a few milliseconds here won't help you if you're stuck with extra seconds elsewhere.  As a general rule, you want to finish the program and then find where optimizations would be most useful.

It's unnecessary to write this.at unless there is a conflict.  Simply saying at will almost always work.

Declare as the interface

You define matches as a HashSet.  In Java, it is more common to define objects as interfaces rather than implementations.  So

                HashSet<Point> matches = new HashSet<Point>();

would be written as

                Set<Point> matches = new HashSet<>();

instead.  This allows you to change the implementation quickly in one place without affecting the rest of the program.

Assuming that you are using Java 8, you don't have to write Point twice.  The compiler will figure out the appropriate type for you.


A method to capitalize the first letter of all words in a String I recently was given a task to capitalize the first character of a string provided the string does not begin with a non-alphabet. Below are the requirements and my implementation. Just want to know if there is a more efficient way to implement this.

Requirements

  Please implement this method to capitalize all first letters of the words in the given String. All other symbols shall remain intact. If a
  word starts not with a letter, it shall remain intact too.Assume that
  the parameter String can only contain spaces and alphanumeric
  characters.
          NOTE: please keep in mind that the words can be divided by single or multiple spaces.The spaces also can be found at the beginning or
  the end of the parameter string,and you need to preserve them.

public static String capitalizeFirstLetters(String s) {
        StringBuilder wordsWithFirstLetterCapitalized = new StringBuilder();
        s = s.trim();
        String words[] = s.split("\\ ");
        if (words.length <= 1) {
            if(words.toString().matches("^[A-Za-z].*$")) {
            words.toString().toUpperCase();
            wordsWithFirstLetterCapitalized.append(words);
            }
            else {
                wordsWithFirstLetterCapitalized.append(words);
            }
        }

        else {
            for (String word : words) {
                if (word.matches("^[A-Za-z].*$")) {
                    word = word.substring(0, 1).toUpperCase() + word.substring(1);
                    wordsWithFirstLetterCapitalized.append(word);
                    wordsWithFirstLetterCapitalized.append(" ");
                }
                else {
                    wordsWithFirstLetterCapitalized.append(word);
                }
            }
        }

        return wordsWithFirstLetterCapitalized.toString();
    }

It appears whitespace is destroyed instead of preserved, see your "NOTE". I suspect input like " 123 456 " will output "123456" destroying outer and inner space.  Similarly "a     b" will become "A B", as split will return a lot of empty strings when splitting more than one space.  How about treating it as a stream and if the previous character is whitespace and the next character is in a-z, upper case it, and emit the character and update your previous character placeholder.  You could probably do this with a single regular expression.