Restricting search to a user's own department, if the user is not a manager I hate having almost two identical blocks do almost the exact same thing.

I am working with Entity Framework 4.4.0.0
Asp.Net MVC

Right now if the user is a "manager" I want to show them the entire list, if they are a normal user or a Team Lead.  I only want to show them their specific department.  The only different from both model is that one add's a where and the other does not.

public ActionResult Index()
{
    AD_CurrentUserProfile currentUP = new ActiveDirectory().GetUserProfile(User.Identity.Name.Split("\\".ToCharArray())[1].ToString());

    IQueryable<IPACS_DT_MasterList> model;

    if (currentUP.currentUser.isManager)
    {
        model = (from d in db.IPACS_Department
                        join f in db.IPACS_Function on d.departmentID equals f.departmentID
                        join pg in db.IPACS_Process on f.functionID equals pg.functionID
                        join sop in db.IPACS_Procedure on pg.processID equals sop.processID
                        select new IPACS_DT_MasterList
                        {
                            departmentID = d.departmentID,
                            functionID = f.functionID,
                            processID = pg.processID,
                            procedureID = sop.procedureID,
                            departmentName = d.name,
                            functionName = f.name,
                            processName = pg.name,
                            procedureName = sop.name,
                            owner = sop.owner
                        });
    }
    else
    {
        model = (from d in db.IPACS_Department
                        where d.name == currentUP.currentUser.department // only change
                        join f in db.IPACS_Function on d.departmentID equals f.departmentID
                        join pg in db.IPACS_Process on f.functionID equals pg.functionID
                        join sop in db.IPACS_Procedure on pg.processID equals sop.processID
                        select new IPACS_DT_MasterList
                        {
                            departmentID = d.departmentID,
                            functionID = f.functionID,
                            processID = pg.processID,
                            procedureID = sop.procedureID,
                            departmentName = d.name,
                            functionName = f.name,
                            processName = pg.name,
                            procedureName = sop.name,
                            owner = sop.owner
                        });
    }

    return View(model);
}

Merge your if statement into the where clause.

public ActionResult Index()
{
    AD_CurrentUserProfile currentUP = new ActiveDirectory().GetUserProfile(User.Identity.Name.Split("\\".ToCharArray())[1].ToString());

    IQueryable<IPACS_DT_MasterList> model =
        (from d in db.IPACS_Department
                where (currentUP.currentUser.isManager || d.name == currentUP.currentUser.department)
                join f in db.IPACS_Function on d.departmentID equals f.departmentID
                join pg in db.IPACS_Process on f.functionID equals pg.functionID
                join sop in db.IPACS_Procedure on pg.processID equals sop.processID
                select new IPACS_DT_MasterList
                {
                    departmentID = d.departmentID,
                    functionID = f.functionID,
                    processID = pg.processID,
                    procedureID = sop.procedureID,
                    departmentName = d.name,
                    functionName = f.name,
                    processName = pg.name,
                    procedureName = sop.name,
                    owner = sop.owner
                });

    return View(model);
}

If the current user is a manager, the first condition will return true and cause the where statement to short-circuit; otherwise it will compare the department name to the user's department.


Reading and displaying an image from a folder I have a question concerning the correctness of the reading and displaying the image in ASP.NET Core MVC web application.

This is the way I am reading image names from a specific folder (I am using IHostingEnvironment _hostingEnvironment to get the rootPath):

   public class GetRandomImageForGalleryView : IGetRandomImageFromFolder
    {
        private string rootPath;
        public GetRandomImageForGalleryView(string rootPath)
        {
            this.rootPath = rootPath;
        }
        public string[] getImage()
        {
            return ReadPhotosFromDirectory();
        }
        private string[] ReadPhotosFromDirectory()
        {

            string[] fileLocations = Directory.GetFiles(rootPath+"\\lib\\Images\\Nature").Select(path => Path.GetFileName(path))
                                     .ToArray();
            return fileLocations;
        }
    }

And this is the way I am displaying them:

@model IGetRandomImageFromFolder
@{
    ViewBag.Title = "Gallery";
}

        @{
            foreach (var item in Model.getImage())
            {

            }
        }

Basically I just replace the image name.

Is this a good way? If not, why? Any other remarks would be helpful as well.

You could simplify your getImage() method pretty significantly to just:

public IEnumerable<string> GetImage() =>
    Directory.GetFiles(rootPath + @"\lib\Images\Nature").Select(Path.GetFileName);

Things I changed:

Changed capitalization to follow .Net naming guidelines.
Removed unnecessary method ReadPhotosFromDirectory().
Removed unnecessary variable fileLocations.
Used verbatim string to avoid having to escape backslashes.
Used method group to delegate conversion instead of lambda.
Got rid of ToArray(), which didn't serve any purpose, and changed return type to IEnumerable<string>.
Used C# 6.0 expression bodied method.


Storing lines in an array Rewrite readlines to store lines in an array supplied by main, rather than calling alloc() to maintain storage. How much faster is the program?

Here is my solution:

int readlines(char **lineptr, char *storage, int maxlines, int storage_size) {

    int len;
    int nlines;

     nlines = 0;
     while((len = _getline(storage, MAXLEN))) {
        if(nlines >= maxlines || !(storage_size - len)) {
            return -1;
        }
         else {
            storage[len - 1] = '\0';
            lineptr[nlines] = storage;

            storage += len; // next free space in the buffer supplied by main
            storage_size -= len;
            nlines++;
        }
    }
    return nlines;
}

Now, the function takes 2 more arguments: a pointer to the array that will store all the read lines and its size. There is no need now to use another array to store the line temporary, I just can pass portions of the storage to getline.

The condition !(storage_size - len) verifies if there is enough space in storage to store the line that was just read. At each iteration storage is incremented to point to the next free space in the array and the value of the variables storage_size is reduced.

(This exercise can be found at page 124 in K&R.)

I don't know what _getline is; perhaps it's something like the file-input function described here.

This looks strange ...

int nlines;

nlines = 0;

I think that C now (like C++) allows you to delay defining a local variable until you initialize it. So there's no need to declare your variables at the top of the function.

Passing MAXLEN is probably wrong: because if (MAX_LEN > storage_size) then you give _getline the opportunity to write past the end of the buffer.

Allowing a buffer overrun is a bug.

Instead you could do something like:

int n_lines = 0;

for (;;)
{
    // define local buffer inside function
    char buffer[MAX_LEN];
    int line_length = _getline(buffer, MAXLEN);
    if (line_length == 0)
        break;
    if (n_lines >= maxlines)
        return -1;
    if (line_length > storage_size)
        return -1;
    buffer[line_length - 1] = '\0';
    // copy local buffer to passed-in buffer
    strcpy(storage, buffer);
    lineptr[n_lines] = storage;
    storage += line_length;
    storage_size -= line_length;
    ++n_lines;
}

return n_lines;

Or you can read directly into the passed-in buffer, to avoid strcpy and avoid defining MAXLEN:

int line_length = _getline(buffer, storage_size);
if (line_length == storage_size)
{
    // fail unless there's nothing more to read
    char temp[1];
    if (0 != _getline(temp, 1))
        return -1;
}

I'm not sure what this statement does ...

storage[len - 1] = '\0';

... if the end-of-line marker is a two-character "\r\n" sequence (which it is on Windows).

You could possibly edit your variable names; if you're going to use_underscores as you did for storage_size, then perhaps:

maxlines -> max_lines
len -> line_length
lineptr -> lines or lines_array
nlines -> num_lines or line_number or line_counter

The least obvious variable name was len.


Unit Testing Express Middleware That Verifies Auth Tokens I am writing an Express middleware component that'll run on Node 4, which has limited ES2015 support.

The fundamental purpose of this component is to verify JSON web tokens, using the jsonwebtoken module. Here is said component:

const jwt = require("jsonwebtoken");

module.exports = {
  ensureAuthenticated: function(req, res) {
    return new Promise(function(resolve, reject) {
      const token = req.query.token;
      if (!token) {
        return reject({
          status: 401
        });
      } else {
        jwt
          .verify(token, "secret", function(err, decoded) {
            if (err) {
              return reject({
                status: 401
              });
            } else {
              req.user = decoded;
              resolve();
            }
          });
      }
    });
  }
};

I also wrote some unit tests, using Mocha and Chai:

const chai = require("chai");
const proxyquire = require("proxyquire");
const httpMocks = require("node-mocks-http");

const expect = chai.expect;

suite("ensureAuthenticated", function() {

  test("should return 401 when token is \"undefined\"", function() {
    const sut = require("../../middleware");
    const req = httpMocks.createRequest();

    return sut
      .ensureAuthenticated(req)
      .catch(function(outcome) {
        expect(outcome.status).to.equal(401);
      });
  });

  test("should return 401 when token is invalid", function() {
    const jsonwebtoken = { };
    jsonwebtoken.verify = function(token, secret, cb) {
      cb({});
    };
    const req = httpMocks.createRequest({
      query: {
        token: "bar"
      }
    });
    const sut = proxyquire("../../middleware", {
      jsonwebtoken: jsonwebtoken
    });

    return sut
      .ensureAuthenticated(req)
      .catch(function(outcome) {
        expect(outcome.status).to.equal(401);
      });
  });

  test("should add decoded data to req object when token is valid", function() {
    const decoded = {
      username: "dummy username"
    };
    const jsonwebtoken = { };
    jsonwebtoken.verify = function(token, secret, cb) {
      cb(undefined, decoded);
    };
    const req = httpMocks.createRequest({
      query: {
        token: "dummy token"
      }
    });
    const sut = proxyquire("../../middleware", {
      jsonwebtoken: jsonwebtoken
    });

    return sut
      .ensureAuthenticated(req)
      .then(function() {
        expect(req.user).to.equal(decoded);
      });
  });
});

As you can see, the component returns a Promise, which makes it unit-testable. I got the idea from this talk.

I would please like to hear any thoughts you have about the quality of my tests. I am also interested in any general criticism.

I only looked at the tests briefly, but they look good and I like your approach

The following changes will help the readability of your main code:

Prefer early returning guard clauses over if/else.  It's easier to follow, uses less code, and avoids a level of indentation.
Name concepts clearly and move them to helper functions, so that your main body expresses its intent better: see notAuthorized()
Arguably a personal preference, but consider it: For short, one-line if statements, don't use brackets and put them on a single line.

Rewrite (note this hasn't been tested, so it's possible there are trivial errors):

module.exports = {
  ensureAuthenticated: function(req, res) {
    return new Promise(function(resolve, reject) {

      // Use guard clause rather if / else
      const token = req.query.token;
      if (!token) return notAuthorized();

      jwt.verify(token, "secret", function(err, decoded) {

        if (err) return notAuthorized();
        req.user = decoded;
        resolve();
      });

    });
    function notAuthorized() { return reject({ status: 401}) }
  }
};


Fire Scheduling with Triggers in CMMS I worked on cmms project .
I have some scheduling , esch of schedule have some triggers (e.g,. Bymeter , byActivity ,ByPeriod).

I wrote some methods to generate workOrder for each Schedule if schedule is ok.

public async Task<IEnumerable<TodayScheduleTaskViewModel>> GetTodayTasks(int page, int recordsPerPage = 50)
    {
        int resultForSkip = page * recordsPerPage;
        var _result = new List<Schedule>();

        bool repeat = true;
        do
        {
            var data = await GetExpression(page, recordsPerPage).Cacheable().ToListAsync();
            if (data.Count == 0) { repeat = false; break; };
            _result.AddRange(Calculate(data).ToList());
            page++;
        } while ((_result.Count < recordsPerPage && repeat));

        return _result.Select(row => new TodayScheduleTaskViewModel
        {
            //map fields

        }).ToList();
    }

and :

 private IQueryable<Schedule> GetExpression(int page, int recordsPerPage = 50)
    {
        int resultForSkip = page * recordsPerPage;

        return _schedules.OrderBy(row => row.Code).Skip(() => resultForSkip).Take(() => recordsPerPage);
    }

and Calculate method :

 private IEnumerable<Schedule> Calculate(List<Schedule> schedules)
    {
        List<Schedule> result = new List<Schedule>();
        foreach (var schedule in schedules)
        {

            bool IsAllTriggersRequired = schedule.WhenFire == GenerateWorkOrderWhen.AllOfTheTriggersFire ? true : false;
            bool dontContinue = false;

            bool timeFalg = false, meterFlag = false, workFlag = false;
            int countersuccessTriggers = 0;

            foreach (var item in schedule.Triggers)
            {

                switch (item.TypeId)
                {

                    case TriggerType.ByWork:

                        if (dontContinue && !IsAllTriggersRequired) break;
                        int workValue = _equipmentFunctionService.GetAssetWorkValue(schedule.AssetId.Value,schedule.CalculatingType);
                        if ((workValue >= item.WorkValue) && !dontContinue)
                        {
                            workFlag = true;
                            dontContinue = true;
                            countersuccessTriggers++;
                        };
                        break;
                }

            }
            var AllTriggersAvailabe = countersuccessTriggers == schedule.Triggers.Count && IsAllTriggersRequired;
            var AnyTriggersAvailabe = timeFalg || meterFlag || workFlag;

            if (AllTriggersAvailabe || (!IsAllTriggersRequired && AnyTriggersAvailabe))
                result.Add(schedule);
        }
        return result;
    }

let's describe method and my goal:

I want to speed up my query.

1- Why use do...While in GetTodayTask :
my GridView Page size is const and I want Just return same records, First I get some Records and pass them to Calculate() Method to check some condition and maybe in Calculate() Method some records removed  (result count changed in calculate())

Some notes on your coding style:

Why do you do int resultForSkip = page * recordsPerPage; at the start of GetTodayTasks? You never use this variable.
var _result = new List<Schedule>();: the underscore prefix should be used to indicate class-level fields, not variables inside a method. Why even call it _result, when schedules is far more clear?
data is also way too generic as a name.
GetExpression doesn't return an expression.
Why is the parameter of Calculate(List<Schedule> schedules) a List<T>? An IEnumerable<T> suffices.
Don't abbreviate "do not" to "don't" in variable names: dontContinue.
The "Falg" in timeFalg should be "Flag".
countersuccessTriggers is improperly capitalized, but it's also badly named. Perhaps successTriggerCount would be better.
There are several places where random blank lines appear. Please clean these.
Use descriptive names: foreach (var item in schedule.Triggers). Why call it an item when it is a trigger?
Why is the property called TypeId? Judging from the rest of the code it is an enum value, so why not call it Type?
Method scope variables should be camelCase, so IsAllTriggersRequired, AllTriggersAvailabe, AnyTriggersAvailabe are incorrect.
Also pay attention to verb conjugations: IsAllTriggersRequired should be areAllTriggersRequired.
However, looking at IsAllTriggersRequired, AllTriggersAvailabe and AnyTriggersAvailabe there are more problems beyond those already mentioned: the naming is inconsistent and feels incorrect. Boolean names like "IsAvailabe" are great as property names, but don't work well as variable names. In this case I'd go for allTriggersAreRequired, allTriggersAreAvailabe and anyTriggerIsAvailabe.


Different return types with same function I'm implementing my own time unit class. This class provides a function for fetching a specific time value (milliseconds, seconds etc.). In order to prevent defining several get() functions for each time unit, I tried to implement a single function to serve this purpose.

typedef unsigned char u_char;
typedef unsigned short u_short;
typedef unsigned long long u_ll;
typedef u_ll DAYS_DTYPE;
typedef u_char HOURS_DTYPE;
typedef u_char MINUTES_DTYPE;
typedef u_char SECONDS_DTYPE;
typedef u_short MILLISECONDS_DTYPE;

// used class members; max. values are handled class-internal
DAYS_DTYPE          m_days;         // max.: 18.446.744.073.709.551.615
HOURS_DTYPE         m_hours;        // max.: 23
MINUTES_DTYPE       m_minutes;      // max.: 59
SECONDS_DTYPE       m_seconds;      // max.: 59
MILLISECONDS_DTYPE  m_milliseconds; // max.: 999

// class-internal enum
enum TimeUnit { Milliseconds, Seconds, Minutes, Hours, Days };

// used member function
template <typename T>
T getTime(TimeUnit unit)
{
    switch (unit)
    {
    case Milliseconds: return m_milliseconds;
    case Seconds: return m_seconds;
    case Minutes: return m_minutes;
    case Hours: return m_hours;
    case Days: return m_days;
    default:
        break;
    }
}

In order to fetch a specific time unit I have to use sth. like this:

getTime<MILLISECONDS_DTYPE>(Time::Milliseconds))

Is this approach acceptable? In case it's not, what should I change?

I prefer something like this, as its less to type and compile-time, you probably always know what kind of time you need on compile-time and even if not you can hack around that problem, so:

template<TimeUnit time>
struct TimeUnitImpl
{ using type = decltype(m_milliseconds); };

template<>
struct TimeUnitImpl<Seconds>
{ using type = decltype(m_seconds); };

template<>
struct TimeUnitImpl<Minutes>
{ using type = decltype(m_minutes); };

template<>
struct TimeUnitImpl<Hours>
{ using type = decltype(m_hours); };

template<>
struct TimeUnitImpl<Days>
{ using type = decltype(m_days); };

// used member function
template <TimeUnit unit = Seconds>
typename TimeUnitImpl<unit>::type getTime()
{
    switch (unit)
    {
    case Milliseconds: return m_milliseconds;
    case Seconds: return m_seconds;
    case Minutes: return m_minutes;
    case Hours: return m_hours;
    case Days: return m_days;
    default:
        break;
    }
}

In order to fetch a specific time unit you would have to use something like this:

getTime<Time::Milliseconds>()

Try it online!

In order to keep a "good style" I would change the function name here, too. Probably simply to time rather than getTime, that get-prefix is pretty much useless here.

But anyways, you might consider just using getTimeMilliseconds or something like that.


Finding the length of the largest run Note: A run is a series of adjacent chars that are the same.

I just want to answer a few questions:

Is this readable?
Does it need commenting or is it self-descriptive enough?
Are there any efficiency tweaks this could have?
Is this a good way to test?
Which is the preferred/conventional syntax?

int maxCheck = 0,
    maxRun = 1,
    currentRun = 1;`

vs.

int maxCheck = 0, maxRun = 1, currentRun = 1;

vs.

int maxCheck = 0;
int maxRun = 1,
int currentRun = 1;

I surmise this probably varies from individual to individual, but for this challenge in particular my instincts were to exclude spaces, but the challenge didn't specify this, and in the last challenge: my intuition, just as the first answer, also wanted me to include digits larger than 9, but that was actually 'wrong' and outside of the specifications of the challenge -- as was evidenced with failed tests (provided by the challenge).

Should I just take 'requirements' literally most of the time to avoid constructing technically 'incorrect' code? I ask since the tests in this case don't include spaces, and it's definitely something I'm wondering in case of larger, more abstract projects.

public class Standford2 {
    public static void main(String[] args) {
        System.out.println("Test 1: " + (1  == maxRun("123")));
        System.out.println("Test 2: " + (2  == maxRun("1223")));
        System.out.println("Test 3: " + (2  == maxRun("112233")));
        System.out.println("Test 4: " + (3  == maxRun("1112233")));
        System.out.println("Test 5: " + (2  == maxRun("hoopla")));
        System.out.println("Test 6: " + (3  == maxRun("hoopllla")));
        System.out.println("Test 7: " + (3  == maxRun("abbcccddbbbxx")));
        System.out.println("Test 8: " + (0  == maxRun("")));
        System.out.println("Test 9: " + (3  == maxRun("hhhooppoo")));
    }

    public static int maxRun(String str) {
        if (str.length() == 0) { return 0; }

        int maxCheck = 0,
            maxRun = 1,
            currentRun = 1;

        if (str.length() == 1) { return maxRun; }

        for (int i = 1; i < str.length(); i++) {
            if (str.charAt(i) == str.charAt(i - 1)) {
                currentRun++;
                if (currentRun > maxRun) { maxRun = currentRun; }
            }
            else { currentRun = 1; }
        }
        return maxRun;
    }
}

The maxCheck variable is unused.  You also don't need the str.length() == 1 special case.

The whitespace around the else is unconventional.

The efficiency is fine.  Your algorithm, like any reasonable algorithm for this problem, is O(L), where L is the length of the input string.  You could try to squeeze out a small performance improvement, but it's almost certainly not worth it.

For a simple function like this, I don't think you need code comments.  However, a JavaDoc comment would always be useful.

The code works equally well on any CharSequence, not just a String, and should therefore be generalized.

/**
 * Finds the length of the longest run of identical characters.
 */
public static int maxRun(CharSequence seq) {
    if (seq.length() == 0) { return 0; }

    int maxRun = 1,
        currentRun = 1;

    for (int i = 1; i < seq.length(); i++) {
        if (seq.charAt(i) == seq.charAt(i - 1)) {
            currentRun++;
            if (currentRun > maxRun) { maxRun = currentRun; }
        } else {
            currentRun = 1;
        }
    }
    return maxRun;
}

The tests that you wrote are fine for a beginner, but the de facto standard for Java is to use junit.


Monitoring progress in Parallel.ForEach every minute I'm using Parallel.ForEach to download 500K URLs and I want to monitor the number of URLs that have been successfully downloaded each minute.

        int elapsedMinutes = 0, cnt = 0;
        Parallel.ForEach(list, tuple =>
        {
            var currMinutes = (int) ((DateTime.Now - startTime).TotalMinutes);
            lock (Object)
            {
                if (currMinutes > elapsedMinutes)
                {
                    elapsedMinutes = currMinutes;
                    Console.WriteLine("{0}: Extracted {1} urls", DateTime.Now, cnt);
                }
            }

            //download urls ...

            Interlocked.Increment(ref cnt);
        });

It looks fine now but the lock statement part seems a bit ugly to me. Is there a better way to achieve this?

Instead of checking the time from each thread on each iteration, I would use a Timer:

int count = 0;

TimeSpan reportPeriod = TimeSpan.FromMinutes(0.1);

using (new Timer(
    _ => Console.WriteLine("{0}: Extracted {1} urls", DateTime.Now, count),
    null, reportPeriod, reportPeriod))
{
    Parallel.ForEach(
        list, tuple =>
        {
            //download urls ...

            Interlocked.Increment(ref count);
        });
}

Console.WriteLine("{0}: Done", DateTime.Now);

I have also renamed cnt to count, those two characters are not worth it to make your code less readable.


Node comparison using priority queue for Dijkstra's Shortest Path Algorithm Instead of using a MinHeap that will allow me to change the vertex priorities I am using the Java library PriorityQueue.

Here is how I have implemented the vertices which will generate the vertex array. Also, I have not maintained any separate array for storing the shortest distance to a vertex from source. Instead, I have a class variable will handle that and more importantly allow the priority queue to sort the elements by implementing a different version of compareTo method.

class indivisualvertex implements Comparable<indivisualvertex> {

int data;                                  //data associated with the vertex
double distanceFromSource;                 //will store the distances from the source vertex
int previousVertexOnShortestPathFromSource;//will store the previous vertex on the path
neighbours adjacentPointsList;             // neighbouring vertices stored in a linked list

//for sorting the vertices in the priority queue according to their distances from the source
public int compareTo(indivisualvertex other) {
    if (this.equals(other)) {
        return 0;
    } else if (this.distanceFromSource > other.distanceFromSource) {
        return 1;
    } else {
        return -1;
    }

}
}

Here's also my implementation for the edge relaxation method used after a vertex is chosen from the priority queue. Since there isn't any method to modify the elements inside of a priority queue, the code picks out the desired vertex, changes its distanceFromSource and previousVertexOnShortestPathFromSource variables and then puts it back, sorting the queue in the process.

 private void relax(indivisualvertex indx) {

 //traverse the neighbouring edges
 for (neighbours nbr = indx.adjacentPointsList; nbr != null; nbr = nbr.next){

        //condition where a vertex is unvisited and also provides a shorter path
        if (visited[nbr.pointer_of_vertex] == false && vertices[nbr.pointer_of_vertex].distanceFromSource > vertices[nbr.from].distanceFromSource + nbr.weight) {
            pq.remove(vertices[nbr.pointer_of_vertex]);
            vertices[nbr.pointer_of_vertex].distanceFromSource = vertices[nbr.from].distanceFromSource + nbr.weight;
            vertices[nbr.pointer_of_vertex].previousVertexOnShortestPathFromSource = nbr.from;
            pq.add(vertices[nbr.pointer_of_vertex]);
        }
    }
}

The implementation works fine. I need to know if there can be any disadvantages to to not building our own minimum heap for this purpose. Also, while relaxing the edges the method extracts the vertex and then puts it back. Is this an expensive operation? How much does it affects the performance of the algorithm?

Stylistic issues

"Indivisual" is not an English word.  It seems likely that you mean "individual".

Please use CamelCase with an initial capital letter for class and interface names.  It makes your code much easier for others to read.  Specifically, indivisualvertex --> IndividualVertex, and neighbours --> Neighbours.

Please use camelCase with an initial lowercase letter for method, field, and parameter names.  In particular, pointer_of_vertex --> pointerOfVertex, or, more precisely, vertexIndex.

Class names generally should be singular nouns, for a class always describes one thing, even if that thing is an aggregate.

Descriptive class and variable names are generally good form, but it is possible to go overboard.  The name previousVertexOnShortestPathFromSource is considerably too long for my taste, and at the same time a bit inaccurate.  I tend to read it as "absurdly long name", which defeats the purpose of descriptive naming.  It would be even worse if there were other names of such length to distinguish among.  For this particular case, I would certainly choose something more pithy, such as previousVertexIndex.

Where descriptive comments for variables are needed, I generally prefer to see them on the preceding line, not as trailing in-line comments.  The latter work for me only if they are quite short.  However, do resist adding such comments at all when they add nothing to what your nice, descriptive variable names already say.

It's at minimum unnecessarily verbose to compare boolean variables with boolean literals, such as in the expression visited[nbr.pointer_of_vertex] == false.  It is usually better form to just use the boolean variable either directly or subject to boolean negation: !visited[nbr.pointer_of_vertex].

Correctness issues

indivisualvertex.compareTo() is flawed.  Given distinct indivisualvertex instaces v1 and v2 with the same values of distanceFromSource, v1.compareTo(v2) and v2.compareTo(v1) will both return -1.

Just because you're comparing floating-point values does not mean that you can ignore the equality case.

Performance considerations

You wrap up your question by raising a series of performance considerations:

  can there be any disadvantages to to not building our own minimum heap for this purpose?

Of course there can.  You already said yourself that the standard library's PriorityQueue does not offer some of the operations you really would like to have, and therefore that you have to use a workaround.  On the other hand, the standard library's PQ has to be very general; all the operations it does implement, it must implement in a generic way.  Although I have every reason to expect the result is robust and efficient -- and obviously, it's already there -- it is likely that additional efficiency would be possible from an implementation tuned to your need.

  Also, while relaxing the edges the method extracts the vertex and then puts it back. Is this an expensive operation?

It's at least twice as costly as rearranging a min heap appropriately without removing the modified element could be made to be.  You've given no basis or context for putting that on absolute grounds.

  How much does it affects the performance of the algorithm?

For the overall algorithm, that depends on the data.  For a single relaxation, see above.


Rails model for publishable posts I'm trying to dry up this Rails post model:

class Post < ActiveRecord::Base
  extend FriendlyId
  friendly_id :title, use: [:slugged, :finders]
  validates_presence_of :title
  scope :published, -> { where('hidden = ? AND published_at IS NOT NULL AND published_at < ?', false, DateTime.now).order(published_at: :desc) }
  paginates_per 10
  attr_accessor :published_at_date, :published_at_time
  acts_as_taggable
  mount_uploader :image, ImageUploader

  after_initialize :set_default_values, :if => :new_record?

  def set_default_values
    self.ad ||= false
  end

  def published?
    if self.hidden == true || self.published_at.blank?
      false
    else
      self.published_at < DateTime.now ? true : false
    end
  end

  def not_published?
    if self.hidden == true || self.published_at.blank?
      true
    else
      self.published_at < DateTime.now ? false : true
    end
  end

  def can_publish?
    !published_at.blank? && hidden?
  end
end

The first thing I am removing is the default value section as that can be changed to a database default to set the initial value of the ad attribute to false each time.

after_initialize :set_default_values, :if => :new_record?

def set_default_values
  self.ad ||= false
end

The next part I am struggling with is the published? and not_published? methods. They should return a boolean value. In trying to try up the published? by removing self and the hidden = true for hidden?. In the else part of the conditional, I remove the ? true : false as the evaluation of the published_at < DateTime.now will return boolean from the method.

def published?
  if hidden? || published_at.blank?
    false
  else
   published_at < DateTime.now
  end
end

The not_published? method follows the same, but flipping the greater than sign to >:

 def not_published?
  if hidden? || published_at.blank?
    true
  else
    published_at > DateTime.now
  end
end

Then I realized that I could just call published? with a not !... duh.

def not_published?
  !published?
end

It is starting to look pretty good but I'd like to reduce it further. How can I reduce the published? method such that it does not need to the false ?

You could just do:

def published?
  !hidden? && published_at.present? && published_at < DateTime.now
end

However I would probably define something like:

def visible?
  !hidden?
end

or possible even

def visible?
  !hidden? && published_at.present?
end

And then

def published?
  visible? && published_at < DateTime.now
end


Solution to Google Code Jam 2008 round 1C problem B This is my solution to Google Code Jam 2008 round 1C problem B (Ugly Numbers). I think it's very elegant. However, I wonder if it's too concise. What could I improve here?

The problem:

  Once upon a time in a strange situation, people called a number ugly
  if it was divisible by any of the one-digit primes (2, 3, 5 or 7).
  Thus, 14 is ugly, but 13 is fine. 39 is ugly, but 121 is not. Note
  that 0 is ugly. Also note that negative numbers can also be ugly; -14
  and -39 are examples of such numbers.

  One day on your free time, you are gazing at a string of digits,
  something like:

  123456

  You are amused by how many possibilities there are if you are allowed
  to insert plus or minus signs between the digits. For example you can
  make

  1 + 234 - 5 + 6 = 236

  which is ugly. Or

  123 + 4 - 56 = 71

  which is not ugly.

  It is easy to count the number of different ways you can play with the
  digits: Between each two adjacent digits you may choose put a plus
  sign, a minus sign, or nothing. Therefore, if you start with D digits
  there are 3^D-1 expressions you can make.

  Note that it is fine to have leading zeros for a number. If the string
  is "01023", then "01023", "0+1-02+3" and "01-023" are legal
  expressions.

  Your task is simple: Among the 3^D-1 expressions, count how many of
  them evaluate to an ugly number.

  Input

  The first line of the input file contains the number of cases, N. Each
  test case will be a single line containing a non-empty string of
  decimal digits.

  Output

  For each test case, you should output a line

  Case #X: Y

  where X is the case number, starting from 1, and Y is the number of
  expressions that evaluate to an ugly number.

Code:

from functools import lru_cache

M = 2*3*5*7
uglies = list(filter(lambda n: (int(n)%2 == 0 or
                                int(n)%3 == 0 or
                                int(n)%5 == 0 or
                                int(n)%7 == 0),
                     range(M)))

@lru_cache(maxsize=None)
def f(line, k=None):
    if k == None:
        return sum([f(line, k) for k in uglies])

    return sum([
        (1 if (int(line) % M) == k else 0),

        sum([f(line[:p],
               (k-int(line[p:])) % M)
             for p in range(1, len(line))]),

        sum([f(line[:p],
               (k+int(line[p:])) % M)
             for p in range(1, len(line))]),
        ])

if __name__ == '__main__':
    import sys
    data = sys.stdin.read().splitlines()[1:]
    case = 1
    for line in data:
        print('{:.0%}'.format(case/len(data)), file=sys.stderr)
        print('Case #{}: {}'.format(case,
                                    f(line)))
        case += 1

M = 2*3*5*7
uglies = list(filter(lambda n: (int(n)%2 == 0 or
                                int(n)%3 == 0 or
                                int(n)%5 == 0 or
                                int(n)%7 == 0),
                     range(M)))

Why the int(...) calls? Surely range returns integers?

@lru_cache(maxsize=None)
def f(line, k=None):
    if k == None:
        return sum([f(line, k) for k in uglies])

I'm not really a Python programmer so I'm not sure how Pythonic it is, but to me this overloading is a code smell. I would prefer to split out one function which takes just the line and another which does the real work.

Also, what is k? I managed to figure it out by reverse engineering, but a short comment would have helped a lot.

    return sum([
        (1 if (int(line) % M) == k else 0),

        sum([f(line[:p],
               (k-int(line[p:])) % M)
             for p in range(1, len(line))]),

        sum([f(line[:p],
               (k+int(line[p:])) % M)
             for p in range(1, len(line))]),
        ])

Again, it would be nice to have a short comment explaining why the recursion works on suffixes (and proving that it's correct, which isn't obvious, because it would seem to allow inserting a - before the first digit of the line in contradiction to the spec).

It would also be nice to see a justification for working on string slices rather than using integers with div/mod by powers of 10.


Implementing a logging system in C++17 I've been programming for what is a probably a little bit and made a very simple logging system for my personal projects. While it works out fairly well so far, I would like advice on how to make it perform better and stuff I can do to make it more usable in general.

Main questions include:

Is my code thread safe? If not, how do I make it thread safe?
Is there a way to make the interface a bit cleaner for use? Right now #define feels a bit hacky.
Is there a way to avoid the use of a global unique_ptr?
Should I just make it a header only library? Is there any benefits in doing so?

.hpp:

#pragma once

/*
Use it by including this header in every file required, and in your main function start a new log.
    Logger::startLog("Log.txt");
Use the various error levels by naming them and simply passing the info and what you want to output.
    Logger::log(ERROR, "Something went wrong.");
*/

// For the unique pointers.
#include <memory>
// Filestream.
#include <fstream>
// String class for names and parameters passed around.
#include <string>

#define FATAL Logger::Level::Fatal
#define ERROR Logger::Level::Error
#define WARNING Logger::Level::Warning
#define INFO Logger::Level::Info
#define DEBUG Logger::Level::Debug

namespace Logger {
    // Severity level enum.
    enum class Level {
        Fatal,
        Error,
        Warning,
        Info,
        Debug
    };

    // Initialize the log.
    void startLog(const std::string& filepath);

    // Log a message.
    void log(Level s, const std::string& msg);

    // Logging class.
    class Log {
    public:
        Log(const std::string& filepath);
        void addLog(Level s, const std::string& msg);
        ~Log();
    private:
        // File for logging.
        std::ofstream m_logfile;
        std::string levels[5] = {"Fatal", "Error", "Warning", "Info", "Debug"};
    };
}

.cpp:

#include "Log.hpp"

namespace Logger {
    // Global Logging Object.
    std::unique_ptr<Log> g_log;

    // Initalize our logging object.
    void startLog(const std::string& filepath) {
        g_log = std::make_unique<Log>(filepath);
        Logger::log(Level::Info, "Started logging system.");
    }

    // Method which logs.
    void log(Level s, const std::string& msg) {
        g_log->addLog(s, msg);
    }

    // Create our global logging object.
    Log::Log(const std::string& filepath) : m_logfile{} {
        m_logfile.open(filepath);
    }

    // Add a message to our log.
    void Log::addLog(Level s, const std::string& msg) {
        if (m_logfile.is_open()) {
            m_logfile << levels[static_cast<int>(s)] << ": " << msg << std::endl;
        }
    }

    Log::~Log() {
        addLog(Level::Info, "Stopped logging system.");
        m_logfile.close();
    }
}

Is my code thread safe? If not, how do I make it thread safe?

No, of course it's not thread-safe. You don't do anything to make it thread-safe.

A more nuanced answer would be: It's thread-safe as long as you don't use it in an unsafe way. For example, calling Logger::log(FATAL, "hello world") from two different threads concurrently would of course be unsafe. But if your program has only one thread... :)

If you want to allow calling Logger::log from two threads concurrently, you'll have to do something to eliminate the data race on m_logfile which is caused by the two threads' both calling m_logfile << levels[static_cast<int>(s)] at the same time. For example, you could throw a mutex lock around addLog.

  Is there a way to make the interface a bit cleaner for use? Right now #define feels a bit hacky.

The only place you use #define is in #define FATAL Logger::Level::Fatal and so on. (By the way, technically #define ERROR ... triggers undefined behavior, because all macros of the form EXXXX are reserved for use by POSIX error codes.) My question is, if you wanted these values to be referred to as FATAL, ERROR, etc., why didn't you just declare them that way?

inline constexpr int FATAL = 0;
inline constexpr int ERROR = 1;
inline constexpr int WARNING = 2;
// ...

Or, probably better:

namespace Logger {
    enum Level {
        FATAL, ERROR, WARNING, // ...
    };
}

Making this an enum (rather than an enum class) allows your user to refer to the enumerators without needing to redundantly name the enum type: just Logger::FATAL, Logger::ERROR, et cetera.

Personally, I would consider writing convenience functions to eliminate the boilerplate:

namespace Logger {
    void log_fatal(const std::string& msg) { log(FATAL, msg); }
    void log_error(const std::string& msg) { log(ERROR, msg); }
    // ...
}

By the way, I think your numbering scheme is backwards. "Level" represents the severity of the message, right? Some messages have higher severity than others? So how would I test whether one message had a higher severity than another? Well, I think I'd write:

if (one_severity > another_severity) ...

But with the values you gave your enumeration, this is actually going to be completely backwards! And so my code for testing severity levels is going to have a bug (or else I'll catch the bug, but then have to write unintuitive code that uses < to mean "greater than"). So, I recommend switching the values around.

  Is there a way to avoid the use of a global unique_ptr?

Sure; declare it static! And to make it really non-global, stick it in a function. It'll still have static lifetime, though. There's not much getting around that.

inline Log *get_glog() {
    static std::unique_ptr<Log> glog = std::make_unique<Log>();
    return glog.get();
}

void startLog(const std::string& filepath) {
    Log *glog = get_glog();
    glog->set_filepath_and_open(filepath);
}

  Should I just make it a header only library? Is there any benefits in doing so?

The big benefit of a header-only library is that it's super easy to incorporate into another project — the user just drops the header file into his include/ directory and he's good to go.

Single-header libraries are particularly nice because they can easily be dropped into Godbolt or Wandbox.

In your case, the tradeoff is that presumably this header is going to get included all over the place (because logging is ubiquitous), and so the bigger you make it, the more work you're forcing the compiler to do in every translation unit.

Since you said "C++17", consider rewriting all your void foo(const std::string& s) signatures into void foo(std::string_view sv) signatures. (Yes, pass string_view by value.)


Reverse the words in string The task:

  Given a string of words delimited by spaces, reverse the words in
  string. For example, given "hello world here", return "here world
  hello"

  Follow-up: given a mutable string representation, can you perform this
  operation in-place?

My solution:

const reverseStr = s => s.split(' ').reverse().join(' ');

console.log(reverseStr("hello world here"));

My "in-place" solution:

const reverseStrInPlace = s => {
  let exp = `\\w+$`;
  const startLen = s.lastIndexOf(s.match(/\w+$/)[0]);
  const strLen = s.length;
  const len = (s.match(/\s/g) || []).length;
  for (let i = 0; i < len; i++) {
    exp = `(\\w+)\\s${exp}`;
    s += ` ${s.match(new RegExp(exp), 'g')[1]}`;
    exp = `(\\w+)\\s${exp}`;
  }
  return s.substring(startLen, startLen + strLen);
}

console.log(reverseStrInPlace('hello world here'));

"In-place" algorithms are defined as:

  An in-place algorithm transforms the input without using any extra
  memory. As the algorithm executes, the input is usually overwritten by
  the output and no additional space is needed for this operation.

Not sure whether this is the right approach for the in-place solution, because, even though I didn't create for the string a new datastructure, i.e. during the operation I overwrite the input and return the overwritten input as the output, but I created other variables, e.g. exp, len, i, etc. i.e. additional space is created for the operation.

In javascript, strings are immutable

  For character access using bracket notation, attempting to delete or assign a value to these properties will not succeed. The properties involved are neither writable nor configurable. (See Object.defineProperty() for more information.)

So this was either a trick question or the creator was not aware.

The only "mutable string representation" of a javascript string would be an array.

So if the question accepts using an array as the representation of the string, you could do something like this:

function reverse(str){
  const mutable = str.split("");

  for(let i = 0; i < mutable.length/2; i++){
    const j = mutable.length-1-i;
    const c = mutable[i];
    mutable[i] = mutable[j];
    mutable[j] = c;
  }

  return mutable.join("");
}

const res = reverse("hello world");

console.log(res);


Cross-browser double form submit prevention We developed a potential solution for the double form submit prevention and we need some review on it. To be able to execute this code on asp.net we needed to add our function call directly into the onsubmit event of the form. This cannot be handled by jQuery because asp.net use a dopostback function and called form.submit(). If onsubmit attribute of the form is empty then it will not execute the code. We don't want to depend on a bloq-UI or disabled button actions.

This is our form tag

 <form id="form1" runat="server" onsubmit="return preventDoubleSubmit(event);">

And this is our javascript that handles the double submit prevention:

//Double submit preventions
var _preventDoubleSubmit = false;

function preventDoubleSubmit(e) {

    if (_preventDoubleSubmit) {
        return cancelDoubleSubmit(e);
    }
    else {
        _preventDoubleSubmit = true;
        return true;
    }
}

function cancelDoubleSubmit(e) {
    if (!e) {
        e = window.event;
    }

    if (e.returnValue != undefined) {
        e.returnValue = false;
    }

    if (e.cancelBubble != undefined) {
        e.cancelBubble = true;
    }

    if (e.stopPropagation) {
        e.stopPropagation();
    }

    if (e.stopImmediatePropagation) {
        e.stopImmediatePropagation();
    }

    if (e.preventDefault) {
        e.preventDefault();
    }

    return false;
}
//END - Double submit prevention

Any review on this would be appreciated.

Can't you just overwrite the onsubmit handler with the first submission?

function preventDoubleSubmit(e) {
    e = e || window.event;
    var form = e.target || e.srcElement;

    form.onsubmit = function() {
        return false;
    };
}


Optimal way to output a dictionary Finished
Someone helped me see that i was framenting the output by writing to each file inside
the for loop instead i needed to have one big for loop for each file so that the harddrive
didnt have to move its head everytime

https://stackoverflow.com/questions/9913788/outputing-dictionary-optimally

I have 4 Dictionarys that contain 800k strings with 200 to 6000 characters.
when i load it into memory it takes up about 11 gigs of memory.
it is taking me 2 minutes to parse the data and 2 minutes to output the data.
Is there anyway to output the data faster than what I am using below?
I am only getting 20-31 MB per second disk IO and I know the hard drive can do 800ish

var hash1 = new Dictionary<int, Dictionary<string, string>>(f.Count + 2);
var hash2 = new Dictionary<int, Dictionary<string, string>>(f.Count + 2);
var hash3 = new Dictionary<int, Dictionary<string, string>>(f.Count + 2);
var hash4 = new Dictionary<int, Dictionary<string, string>>(f.Count + 2);

...

foreach (var me in mswithfilenames)
{
    filename = me.Key.ToString();
    string filenamef = filename + "index1";
    string filenameq = filename + "index2";
    string filenamefq = filename + "index3";
    string filenameqq = filename + "index4";

    StreamWriter sw = File.AppendText(filenamef);
    StreamWriter sw2 = File.AppendText(filenameq);
    StreamWriter swq = File.AppendText(filenamefq);
    StreamWriter sw2q = File.AppendText(filenameqq);

    for (i = 0; i <= totalinhash; i++)
    {
        if (hashs1[i].ContainsKey(filenamef))
        {
            sw.Write(hashs1[i][filenamef]);
        }
        if (hashs2[i].ContainsKey(filenameq))
        {
            sw2.Write(hashs2[i][filenameq]);
        }
        if (hashs3[i].ContainsKey(filenamefastaq))
        {
            swq.Write(hash4[i][filenamefastaq]);
        }

        if (hash4[i].ContainsKey(filenameqq))
        {
            sw2q.Write(hash4[i][filenameqq]);
        }
    }
    sw.Close();
    sw2.Close();
    sw3.Close();
    sw4.Close();
    swq.Close();
    sw2q.Close();
}

One way to speed up the lookups is to use TryGetValue() instead of ContainsKey() and then the indexer. So, for example:

if (hashs1[i].ContainsKey(filenamef))
{
    sw.Write(hashs1[i][filenamef]);
}

would become:

string value;
if (hashs1[i].TryGetValue(filenamef, out value))
{
    sw.Write(value);
}

But it's hard to tell how much would that help you.

Another thing that might help you would be combining all the hashes into one that contains object that contains all the inner hashes. Something like:

class Hashes // or struct?
{
    public Dictionary<string, string> hash1 { get; set; }
    public Dictionary<string, string> hash2 { get; set; }
    public Dictionary<string, string> hash3 { get; set; }
    public Dictionary<string, string> hash4 { get; set; }
}

…

var mainHash = new Dictionary<int, Hashes>(f.Count + 2);

Combining the two approaches lowers the number of potential lookups per iteration from 16 to 5.

Also, unrelated to performance, but you should name your variables by their contents, not implementation. I think hash1 is a very bad name.


Design Pattern - Strategy Pattern in PHP Take a look at the following code and let me know if its right, and if there is any room for improvements.

Strategy Class

<?php
/**
 * Draw Strategy class
 */
abstract class DrawStrategy
{
    abstract function draw(Shape $shape);
}
/**
 * This is ConsoleDraw and takes the shape object and draws accordingly, Drawign Algo is
 * implemented
 * here
 */
class ConsoleDraw extends DrawStrategy
{
    public function draw(Shape $shape)
    {
        echo "---------------";
    }
}

class WebDraw extends DrawStrategy
{
    public function draw(Shape $shape)
    {
        echo "=-=-=-=-=-=-=-=-=-";
    }
}

Shape Class ( That Composites Strategy Class )

<?php
/**
 * Design Pattern Impl. of Composite Pattern.
 *
 * Background:
 *
 * Shape is the Object, It can be specialised as Square or Rectangle and It can be drawn on
 * Web, or on Console / commandline
 *
 * The Drawing Algo is abstracted in Strategy Class and its used in the Shape Class
 *
 * Shape Class Uses DrawingStrategy and used accordingly, Uses ConsoleDraw, WebDraw as required
 *
 *
 */
include "strategy.php";

/**
 * Abstract Base Class Shape (Generalisation)
 *
 */
abstract class Shape
{
    public $length = 0;

    public function __construct( $_length )
    {
        $this->length = $_length;
    }

    /**
     * [draw this abstracts draw functionality]
     * @return [type] [description]
     */
    abstract function draw();

    /**
     * [area This is another function that calculates area]
     * @return [type] [description]
     */
    abstract function area();

}

/**
 *  SQuare Class Extends Shape and defines the implementation,
 *  It uses DrawStrategy as a component and so that it can draw either on the console or
 *  on the web.
 *
 */
class Square extends Shape
{
    public $length = 0;

    private $drawstrategy;

    public function __construct( $_length , DrawStrategy $_strategy )
    {
        $this->length = $_length;

        $this->drawstrategy = $_strategy;
    }

    public function area(){
        return $this->length * $this->length;
    }
    /**
     * draw - Takes up the Draw Strategy and draw on the current object
     * wether it is on console or web
     * @return [type] [description]
     */
    public function draw()
    {
        $this->drawstrategy->draw( $this );
    }

}

class Rectangle extends Shape
{
    private $drawstrategy;

    public function __construct( $_length, $_breadth, DrawStrategy $_strategy )
    {
        $this->length = $_length;
        $this->breadth = $_breadth;

        $this->drawstrategy = $_strategy;
    }

    public function area()
    {
        return $this->length * $this->breadth;
    }

    public function draw()
    {
        $this->drawstrategy->draw( $this );
    }
}

$shape[] = new Square( 4 , new ConsoleDraw );
echo "Shape 0 " .$shape[0]->area();
$shape[0]->draw();

$shape[] = new Square( 10, new WebDraw );
echo "Shape 1";
$shape[1]->draw();

I think there might be room for improvement.

First of all, Strategy pattern relies on interfaces / common classes and you are combining Shape with concrete Strategy passing it to the constructor.

To be more specific, here you bound Square with ConsoleDraw and WebDraw:

$shape[] = new Square( 10, new ConsoleDraw );
$shape[] = new Square( 10, new WebDraw );

The question is, what if I passed any DrawStrategy to the Square object? Should it work? Everytime? This is my point. I think you should hide allowed strategies on list behind the concrete Shapes and pass context.

Based on context, pick the right strategy (let's say, we are in Console Context - pick the ConsoleDraw. If we were on DesktopAppliction context - throw Exception "Unsupported" or don't draw at all).


Getting partition boundaries in sorted integer array using only equalTo operator For my task, I need to find partition boundaries in sorted input. Values are expected to be repeated, I just need to find range for each value. Please check comment in following output for example.
For some reason, I can not use < or > operation, I only have equality operator.

/*
Input:
Index :  Value
0 : 1
1 : 1
2 : 1
3 : 2
4 : 2
5 : 2
6 : 2

Output:
Value 1 is till 2 index
Value 2 is till 6 index
   */
   public void printPartitionBoundaries(ArrayList<Integer> array)
   {
      int f = 0;
      int l = array.size()-1;
      while (f < l) {
         int cp = ((Integer)array.get(f)).intValue();
         int of = f;
         boolean done = false;
         while (!done)
         {
            int m = (f + l)/2;
            if ((l-f) <= 1) {
               if ( l == array.size() -1 )
                  System.out.println("Value " + cp + " is till " + l + " index");
               else
                  System.out.println("Value " + cp + " is till " + (l-1) + " index");
               done = true;
               break;
            }
            if (array.get(f).equals(array.get(l)) == false) {
               if (array.get(f).equals(array.get(m)) == false)
                  l = m;
               else
                  f = m;
            } else {
               f = m;
            }
         }
         f = l;
         l =  array.size()-1;
      }
   }

In general single letter variables are okay, but here short names like first and last would be more readable.

Also java has a very strong convention of always using braces {}, and an indentation of 4 spaces. (Historically C/C++ had normally 3, and one whished less nesting of code due to the "superior" quality of the new language.)

List as interface is more generic than ArrayList, so do not overspecify, be too restrictive.

Using an exclusive upperbound is often done in computer science, and indeed in the java APIs themselve. Here it would mean having partions [f, l> like [a, b>, [b, c>, [c, d>, [d, e>. So at the end of the range searching loop f = l;.

   public void printPartitionBoundaries(List<Integer> array)
   {
      int f = 0;
      int l = array.size(); // Upperbound exclusive
      while (f < l) {
         int cp = array.get(f);

of (old first) is a leftover.

The loop with done: done could be removed, a while (true) would
work as well. Hence use the first if-condition as while-condition.

Also m was calculated too early.

Also == false and == true should really not be seen.

The nested ifs can be simplified.

Mind that the condition treats two cases of 1 element and 0 elements. Simpler would be while (f < l).

         while ((l-f) > 1)
         {
            int m = (f + l)/2;
            if (!array.get(f).equals(array.get(l))
                   && !array.get(f).equals(array.get(m))) {
               l = m;
            } else {
               f = m;
            }
         }

I wonder about the correctness; somehow one expects f = m + 1; to have a loop variant (l-f) diminishing every step.

The following if-statement on l == array.size() should become:

         System.out.println("Value " + cp + " is till " + (l-1) + " index");

Check, maybe the while-condition should become > 0, and extra step more.

         f = l;
         l =  array.size();
      }
   }

To verify the correctness I leave up to you yourself.

Resulting code:

   public void printPartitionBoundaries(List<Integer> array)
   {
      int f = 0;
      int l = array.size(); // Upperbound exclusive
      while (f < l) { // f != l
         int cp = array.get(f);
         while ((l-f) > 1) // maybe better f != l too
         {
            int m = (f + l)/2;
            if (!array.get(f).equals(array.get(l))
                   && !array.get(f).equals(array.get(m))) {
               l = m;
            } else {
               f = m;
            }
         }
         System.out.println("Value " + cp + " is till " + (l-1) + " index");
         f = l;
         l =  array.size();
      }
   }


Can this FindString function be optimized further, in terms of speed? int FindStr(char* str, int strsize, char* fstr, int from)
{
    for(int i=from, j=0; i<strsize; i++)
    {
        if(str[i]==fstr[j])
            j++;
        else
            {i-=j; j=0;}

        if(fstr[j]=='\0')
            return i-j+1;
    }

    return -1;
}

The function searches for a string fstr in str and returns its index in str if found, otherwise, it will return -1.  It's also possible to specify where to start searching in the string.

My question is, can I optimize this function further? Also, do you see any potential problems in this function?

Some comments to add to the first two of @200_success:

strstr does a good job. Why reinvent?
sizes are often passed as size_t rather than int
modifying the loop variable within the loop is generally considered bad
practice
i -= j executes on every loop unless there is a match.  Mostly in this
case j is zero so the line has no effect, but it still executes
if fstr is an empty string it returns the wrong result (1)
add some spaces around operators and after if, for


Simple chat server in Haskell I've been struggling with concurrent programming in Haskell for a while.  It's so hard to reason about, especially when exceptions come into the picture.

As a learning exercise, I implemented a simple chat server.  Clients connect using telnet on port 1337, and start typing text.  Each message is broadcast to all connected clients.  Clients are distinguished by an incrementing counter.

Requires the stm-linkedlist package

import Control.Concurrent
import Control.Concurrent.STM
import Control.Exception as E hiding (handle)
import Control.Monad
import Network
import Data.STM.LinkedList (LinkedList)
import System.IO

import qualified Data.STM.LinkedList as LL

type Client = TChan String

main :: IO ()
main = do
    clients <- LL.emptyIO
    sock <- listenOn (PortNumber 1337)
    let loop n = do
            (handle, host, port) <- accept sock
            putStrLn $ "Accepted connection from " ++ host ++ ":" ++ show port
            _ <- forkIO $ serve clients handle n
            loop $! n+1
     in loop 0

serve :: LinkedList Client -> Handle -> Integer -> IO ()
serve clients handle n = do
    hSetBuffering handle LineBuffering
    send_chan <- newTChanIO
    receiver <- myThreadId

    let sendLoop = forever $
            atomically (readTChan send_chan) >>= hPutStrLn handle

        receiveLoop = forever $ do
            line <- hGetLine handle
            broadcast $ "<client " ++ show n ++ ">: " ++ line

        broadcast message =
            atomically $ LL.toList clients >>= mapM_ (flip writeTChan message)

        start = do
            node   <- atomically $ LL.append send_chan clients
            sender <- forkIO $ sendLoop `onException` killThread receiver
            return $ do
                atomically $ LL.delete node
                killThread sender

     in bracket start (\finish -> finish) (\_ -> receiveLoop)

One hurdle is that I can't send and receive simultaneously in the same thread.  Thus, I'm forced to send and receive in separate threads.

In the code below, I spawn two threads per client:

A "receive" thread, which gets lines from the client and broadcasts them.
A "send" thread, which reads this client's TChan (written to by "receive" threads) and sends each message to the client.

Using a TChan, rather than having other threads write directly to this thread's Handle, has two benefits:

If sending produces an exception, it doesn't kill the innocent sending client's thread.  It only kills the "send" thread of the target client.
It keeps message text from getting interleaved.

When the "receive" thread hits an exception, its finalizer removes the client's send channel from the linked list and kills the "send" thread.  When the "send" thread hits an exception, it kills the "receive" thread, causing its finalizer to be invoked.

Is this a good approach?  Is there a better way to organize network code like this?

One hurdle is that I can't send and receive simultaneously in the same thread. Thus, I'm forced to send and receive in separate threads.

Naturally. This is because "send" and "receive" for a chat server behave asynchronously, and block on different events. Haskell's threads are lightweight, so you can write your code using blocking calls and multiple threads, and rest assured that behind the scenes Haskell is using epoll.

You could modify the chat server so that the client is required to give a special command in order to read new messages, in that case only one thread would be needed, since all messages from you to the client would depend directly on that particular client's input. But then it wouldn't feel much like a chat program.

  [Haskell] so hard to reason about, especially when exceptions come into the picture.

Exceptions are best avoided in Haskell, but you seem to have been able to reason about them fairly well. Did you have any specific issues reasoning about your code? I think it might be fair to replace [Haskell] with [Concurrent programming]; exceptions and concurrency are inherently at odds, in any language.

  Is this a good approach? Is there a better way to organize network code like this?

I think it's pretty good. I don't do much networking code so take my opinion with a grain of salt.

Clearly this is just some simple example code, but you might want to consider allowing the client to send a special message to stop interacting with the server, so that disconnecting is not entirely dependent on exceptions.


Convert big collection into simple array Is there a better way to do this?

const data = [{id: 1, name: 'item1'}, {id: 2, name: 'item2'}]
const a = []
data.forEach(x => { a[x.id] = x.name })
return a // ['1': 'item1', '2': 'item2']

Yes. Use array.reduce and use an object.

const a = data.reduce((items, item) => (items[item.id] = item.name, items), {});

// a = {1:{...}, 2: {...};

You can still use the bracket notation to access the items, i.e. a[id].

A problem with using IDs as array indices is that you introduce gaps in the array. You also get a false array length. Take the following example:

const a = [];
a[999] = {/* User data */};
a.length // 1000;

You only added a single item, with id "999" but now the array reports 1000 in length. Items 0 to 998 are actually empty.


I want reduce the time complexity for this ladder problem to find the minimum ladders required I was trying to attempt below given challenge. I tried multiple ways to solve the problem but none of them were good enough to pass the time limit. Please advise if you have any idea to improve on this.
Problem:
You are visiting a building of N floors. On every floor, only one ladder of specified length is present. If the length of the ladder is x units, you can reach y floors above from the current one.
You can leave the ladder in between in order to change the ladder, but you can only start from the starting floor of the ladder.
You are given Q questions. In each question, you will be given a floor number. For each question, you have to tell the least number of ladders required to reach given floor.
Initially, you are on the ground floor.
Input Format:
The first line contains an integer T, indicating the number of test cases.
For each test case:
The first line contains an integer N, indicating number of floors in the building.
Next line contains N space separated positive integers which denote the length of ladder at each floor (First integer corresponds to ladder length on ground floor, second integer corresponds to ladder length on first floor and so on) .
Next line contains an integer Q, indicating number of questions.
Following Q lines contain an integer each, denoting the floor number for which answer is to be computed.
Output Format:
For each question, print the least number of ladders required required to reach given floor.
Answer for each question should come in a new line.
Sample Input:
1
10
2 2 1 1 2 2 3 1 1 1
10
5
4
3
2
1
10
9
8
7
6

Sample Output:
4
3
2
1
1
6
5
5
5
4

Problem Reference
Solution:
class Program
{
    static void Main(string[] args)
    {
        int T = Convert.ToInt32(Console.ReadLine());
        StringBuilder sb = new StringBuilder("");
        for (int i = 0; i < T; i++)
        {
            int N = Convert.ToInt32(Console.ReadLine());
            sb.EnsureCapacity(sb.Length + (5 * N));
            int[] numOfStairs = new int[N + 1];
            numOfStairs[0] = 0;
            string[] stairs = Console.ReadLine().Split();
            int[] reaches = new int[N];
            for (int j = 0; j < stairs.Length; j++)
            {
                int length = Convert.ToInt32(stairs[j]);
                reaches[j] = j + length;
            }
            int Q = Convert.ToInt32(Console.ReadLine());

            for (int j = 0; j < Q; j++)
            {
                int target = Convert.ToInt32(Console.ReadLine());
                sb.AppendLine(GetShortestPath(target, reaches.Take(target).ToArray(), numOfStairs).ToString());
            }
        }
        Console.WriteLine(sb.ToString());
    }
    static int GetShortestPath(int target, int[] reaches, int[] numOfStairs)
    {
        if (numOfStairs[target] == 0 && target != 0)
        {
            List<int> connectedFloors = GetPreviousFloorList(target, reaches);
            int[] shortestPaths = new int[connectedFloors.Count];
            for (int i = 0; i < connectedFloors.Count; i++)
            {
                int shortestPath = GetShortestPath(connectedFloors[i], reaches.Take(connectedFloors[i]).ToArray(), numOfStairs);
                shortestPaths[i] = shortestPath;
            }
            int result = shortestPaths.Min() + 1;
            numOfStairs[target] = result;
            return result;
        }
        else
        {
            return numOfStairs[target];
        }
    }
    static List<int> GetPreviousFloorList(int target, int[] reaches)
    {
        List<int> floors = new List<int>(target / 2);
        for (int i = 0; i < target; i++)
        {
            if (reaches[i] >= target)
                floors.Add(i);
        }
        return floors;
    }
}

Applied few minor optimisations to the code from the OP's answer. Also fixed naming issues (locals name starts from a lower-cased letter).
class Program
{
    static void Main(string[] args)
    {
        int t = int.Parse(Console.ReadLine());
        StringBuilder sb = new StringBuilder();
        for (int i = 0; i < t; i++)
        {
            int n = int.Parse(Console.ReadLine());
            int[] numOfStairs = new int[n + 1];
            int floorsMarked = 0;
            string[] str = Console.ReadLine().Split();
            for (int j = 0; j < str.Length; j++)
            {
                int length = int.Parse(str[j]);
                int reach = j + length > n ? n : j + length;
                while (floorsMarked < reach)
                {
                    floorsMarked++;
                    numOfStairs[floorsMarked] = numOfStairs[j] + 1;
                }
                if (reach == n)
                    break;
            }
            int q = int.Parse(Console.ReadLine());
            for (int j = 0; j < q; j++)
            {
                int target = int.Parse(Console.ReadLine());
                sb.Append(numOfStairs[target]).AppendLine();
            }
        }
        Console.Write(sb);
    }
}


Need to pass cookie information to a child iFrame - Are there any security concerns? I have a child iframe in my page that will load content from a different web application in a different virtual directory than the parent page.

The DOM will look roughly like this:

The user will log in to www.contoso.com/WebApp1.
The user will load www.contoso.com/WebApp1/Home which contains an iframe with content from www.contoso.com/WebApp2/Home.
To load content from www.contoso.com/WebApp2/Home the user must be logged in.

My Question:

Could I simply load the iframe with src=www.contoso.com/WebApp2/Home?sessionId={sessionIdFromTheParentCookie}, and write server-side code to trust the incoming sessionId and set a cookie for the child iframe?
At present, I can't see how this is any worse than the reality that a user could tamper with his/her client-side cookie an insert an arbitrary sessionId in it.
The sessionId must always be validated at the server, so what's the harm in supplying an arbitrary sessionId in the query string and asking the server to set the cookie?

My Answer (best guess)

Tag is correct, out of security concer i would still not pass the session id as parameter(only if there is no other way)
It makes it harder to tamper with, if it is httplony see wikipedia
Links with sessionids could be bookmarked and after the end of the session they wont work, or they could beforwarded, and sesssions could be passed/shared, ...

Btw.: i found this link https://security.stackexchange.com/questions/14093/why-is-passing-the-session-id-as-url-parameter-insecure covering more or less the same topic.

I hope it helps


Displaying categories and subcategories in php having different tables Just wanted to ask if there is an other way of PHP coding for displaying my subcategories and subsubcategries than mine.

My tables:

Categories
-------------------------
cat_ID | cat_name
------------------
4      | Baby & Kids
5      | Bicycles
6      | Boats
7      | Books & Comics
....
13     | Clothes & Accessories
....
35     | Sport & Fitness
36     | Study
....
38     | Toys & Games
....

Subcategories
-------------------------
subcat_ID | cat_ID | extra_cat_ID | subcat_name
------------------------------------------------
....
15        | 4      | 13           | Baby clothes
16        | 4      | 0            | Baby products
17        | 4      | 13           | Kids clothes
18        | 4      | 38           | Toys
19        | 5      | 0            | Bycicles
20        | 5      | 0            | Bycicle gear & Accessories
21        | 6      | 0            | Boat parts
22        | 6      | 0            | Other Boats
23        | 6      | 0            | Power Boats
24        | 6      | 0            | Sailboats
25        | 6      | 35           | Windsurf & Surfing
26        | 7      | 0            | Antiquarian
27        | 7      | 0            | Books
28        | 7      | 38           | Childrens books
29        | 7      | 0            | Comics
30        | 7      | 0            | Magazines & Newspapers
31        | 7      | 36           | Study & Training

Subsubcategories
-------------------------
subsubcat_ID | subcat_ID | subsubcat_name
-----------------------------------------
...
470          | 15        | Baptism outfits
471          | 15        | Bibs
472          | 15        | Body warmers
473          | 15        | Bodysuits
....
496          | 16        | Baby bath
497          | 16        | Baby books
498          | 16        | Baby inserts
499          | 16        | Baby monitors
....
548          | 17        | Belts
549          | 17        | Blouses & Shirts
550          | 17        | Body warmer
551          | 17        | Boots
....
....
740          | 26        | Music
741          | 26        | Navy
742          | 26        | Novel
743          | 26        | Photography
....
....
867          | 30        | Animals
868          | 30        | Arts and Culture
869          | 30        | Branch
870          | 30        | Cars
870          | 30        | Computers
....
....
etc.

I know it's just super basic HTML, but what I want to do is this:

subcat_name 1

subsubcatname 1
subsubcatname 2
subsubcatname 3
subsubcatname 4
subsubcatname 5
subsubcatname 6
subsubcatname 7

subcat_name 2

subsubcatname 8
subsubcatname 9
subsubcatname 10
subsubcatname 11
subsubcatname 12

subcat_name 3

subsubcatname 13
subsubcatname 14
subsubcatname 15
subsubcatname 16
subsubcatname 17
subsubcatname 18

etc ...

My code I use right now:

$result=mysqli_query($connect,"SELECT subcategories.subcat_name, subsubcategories.subsubcat_name, subcategories.subcat_ID FROM subcategories INNER JOIN subsubcategories ON subcategories.subcat_ID=subsubcategories.subcat_ID WHERE subcategories.cat_ID = $cat_ID OR subcategories.extra_cat_ID = $cat_ID ORDER BY subcategories.subcat_name, subsubcategories.subsubcat_name ASC");
$lastcat = 0;
while ($row = mysqli_fetch_array($result)){
    if($lastcat != $row['subcat_ID']){
        $lastcat = $row['subcat_ID'];
        echo "
";
        echo $row['subcat_name'];
        echo "
";
        echo "
";
        }
    echo $row['subsubcat_name'];
    echo "
";
    }

My code is working fine but what I want to know is if there is a better or safer way of handling it in PHP? Or am I doing it the right way?

Yes, let's see:

You are vulnerable to SQL injection: By including variables directly in the query, you are making yourself vulnerable to SQL injection attacks. Use prepared statements. Further reading: How can I prevent SQL Injection in PHP?
Don't use
 for vertical spacing control. You should use proper semantic elements, for example:

Subcat 1

Subsubcat 1

Subsubcat 2

Subsubcat 3

Aside from that, LGTM. Good job! :)

Example for prepared statements:

$stmt = mysqli_prepare($connect, "SELECT subcategories.subcat_name, subsubcategories.subsubcat_name, subcategories.subcat_ID FROM subcategories INNER JOIN subsubcategories ON subcategories.subcat_ID=subsubcategories.subcat_ID WHERE subcategories.cat_ID = ? OR subcategories.extra_cat_ID = ? ORDER BY subcategories.subcat_name, subsubcategories.subsubcat_name ASC");
mysqli_stmt_bind_param($stmt, "ii", $cat_ID, $cat_ID);
mysqli_stmt_execute($stmt);


Word ladder solution in Java using matrix Given two words (start and end), and a dictionary, find the length of shortest transformation sequence from start to end, such that only one letter can be changed at a time and each intermediate word must exist in the dictionary. For example, given:

start = "hit"
end = "cog"
dict = ["hot","dot","dog","lot","log"]

  One shortest transformation is "hit" -> "hot" -> "dot" -> "dog" -> "cog", the program should return its length 5.

The following is my implementation of Word Ladder problem. Can anybody review the code to compare its efficiency with other solutions?

package arrays;

import java.util.Hashtable;
import java.util.Set;

public class WordLadder {

public static void main(String[] args) throws Exception {
    wordLadder();
}

private static void wordLadder() throws Exception {
    String source = "hit";
    String destination = "cog";
    Hashtable<Integer, String> dict = new Hashtable<Integer, String>();
    dict.put(1, source);
    dict.put(2, "hot");
    dict.put(3, "dot");
    dict.put(4, "dog");
    dict.put(5, "lot");
    dict.put(6, "log");
    dict.put(7, destination);
    formHammingDistanceMat(dict);
}

private static void formHammingDistanceMat(
        Hashtable<Integer, String> dict) throws Exception {
    int mat[][] = new int[dict.size()][dict.size()];
    for (int[] row : mat) {
        java.util.Arrays.fill(row, 99999);
    }

    Set<Integer> keySet = dict.keySet();
    for (int i = 0; i < keySet.size() - 1; i++) {
        String mainString = dict.get(i + 1);
        for (int j = i + 2; j <= dict.size(); j++) {
            if (j == 7)
                System.out.println();
            mat[i][j - 1] = findHamming(mainString, dict.get(j));
        }
    }
    System.out.println(findShortestPath(mat, 0, 1,dict));
}

private static int findShortestPath(int[][] mat, int row, int col,
        Hashtable<Integer, String> dict) {
    if (col == dict.size()-1) {
        return 0;
    }
    if (mat[row + 1][col + 1] == 1) {
        return 1 + findShortestPath(mat, row + 1, col + 1, dict);
    } else if (mat[row][col + 1] == 1) {
        return 1 + findShortestPath(mat, row, col + 1, dict);
    } else
        return 1 + findShortestPath(mat, row + 1, col, dict);
}

private static int findHamming(String mainString, String str)
        throws Exception {
    if(mainString.length()!=str.length()) throw new Exception();
    int count = 0;
    for (int i = 0; i < mainString.length(); i++) {
        if (mainString.charAt(i) != str.charAt(i))
            count++;
    }
    return count;
}
}

package arrays;

Wrong, unless it's your name and you've registered the top-level domain arrays. Seriously, use a personal package prefix, even for toy code.

import java.util.Hashtable;

Hashtable still exists? :D Forget it, and use HashMap (it's about the same, but not synchronized and much more in use).

Hashtable<Integer, String> dict = new Hashtable<Integer, String>();
dict.put(1, source);
dict.put(2, "hot");

That's what arrays are good for, you need no map here.

java.util.Arrays.fill(row, 99999);

Feel free to import it. Is 99999 enough? Sure, but the classical choice is Integer.MAX_VALUE. But this may lead to overflow, so sticking with a smaller constant is fine. Just declare the constant as

private final int VERY_FAR = 99999;

so that your code is not filled with essentially random numbers.

for (int i = 0; i < keySet.size() - 1; i++) {
    String mainString = dict.get(i + 1);
        for (int j = i + 2; j <= dict.size(); j++) {

You shouldn't have started the numbering with 1. Now there's i, i+1, i+2, pretty confusing. Actually, you shouldn't numbered the words at all, simply

List<String> dict = Arrays.asList(
    new String[]{"hot","dot","dog","lot","log"});

would do. Actually, you could stick with an array like

String[] dict = {"hot","dot","dog","lot","log"};

but lists are nearly always preferred.

You're computing the hamming distance of all word pairs. Google says

  The number of words in the English language is: 1,025,109.8.

which means some 1e12 pairs. You'd need a pretty smart algorithm to avoid this pairing, so let's forget it for now.

However, the problem is that you compute all the distances, while

you don't really need them
they don't really help

You don't need the distance, all you need is to know if two words are neighbors (i.e., a boolean instead of an int). But actually, the question is not "given two words, determine their distance", but "starting with dog, where can I go?". So what you need is something like

Map<String, Set<String>> neighbors;

and then a path-finding algorithm (which actually could make use of knowing the distance, but that's a different story).

if(mainString.length()!=str.length()) throw new Exception();

That's too brutal. Mathematically, returning infinity would be perfect. Returning some big number would be fine. Throwing an IllegalArgumentException or some exception of your would be fine, too.

However, it's good that you think about it. Doing nothing would be too bad (distance("hot", "hotdog") would be zero).


Word bucketing in Python First I'm not sure what this is called formally but whatever.

Basically I need to form "wildcard buckets". I think the best way to explain what I need to do is to take an example. For instance, say I have the three words "aaa", "aab", and "baa".

Let me use * as the wildcard. Then from aaa I can form -aa, a-a, aa-, --a, a--, a-a, and ---. I then add these to a dictionary where the keys are the buckets and values are sets of (complete) words that fit these buckets. I repeat for all my words. So in the end I get that, for instance, -a- fits all three of the example words that I have above.

I have working code, but it is very slow in forming these buckets when I have a word list that has hundreds of thousands of words.

for numberSpaces in range(0, len(word) + 1):  # loop through all spaces of word
    buckets = itertools.combinations(range(0, len(word)), numberSpaces)
    for indicesToReplace in buckets:  # for each index in this combination
        newWord = word
        for index in indicesToReplace:  # replace character with "*"
            newWord = newWord[0:index] + "*" + newWord[index + 1:]
        if newWord not in bucketDictionary.keys(): # Add to dictionary
            bucketDictionary[newWord] = set()
        bucketDictionary[newWord].add(word)

I would like to know how to best optimize this code. I've tried recursion but I'm not sure if that is any better than this iterative method.

Thank you!

I think the way to optimize it is to try an entirely different approach -- given a pattern that matches all the words in a set, and a new word to add to the set, how do you modify the pattern to match the new word as well?  Simple answer: as long as the non-wild characters in the pattern match the new word, leave them; otherwise change the pattern to have a wildcard at that position.  You could implement this either recursively or iteratively.

from typing import List

WILD = "*"

def build_common_pattern(words: List[str]) -> str:
    """Build a pattern matching all input words.
    WILD matches any single character."""
    pattern = words[0]
    for word in words:
        assert len(word) == len(pattern)
        for i in range(len(word)):
            assert word[i] != WILD
            if pattern[i] != word[i]:
                pattern = pattern[:i] + WILD + pattern[i+1:]
    return pattern

print(build_common_pattern(["aaa", "aab", "baa"]))

This is also a potential use case for functools.reduce:

def build_common_pattern(words: List[str]) -> str:
    """Build a pattern matching all input words.
    WILD matches any single character."""
    def union(a: str, b: str) -> str:
        result = ""
        assert len(a) == len(b)
        for i in range(len(a)):
            result += a[i] if a[i] == b[i] else WILD
        return result

    return reduce(union, words)


JUnit testing for Calculator in Java I made a simple calculator.

I would also like to write some simple unit tests for my CalculatorEngine class. I had to make some of my private methods as package-private over this. How is it usually better to do it - to enhance access modifier or to make inner test class?

And what another more complicated cases it's better to test?

My CalculatorEngineTest class is:

import org.junit.Before;
import org.junit.Test;

import static junit.framework.Assert.assertTrue;
import static junit.framework.TestCase.assertEquals;

public class CalculatorEngineTest {

    CalculatorEngine calcEngine = new CalculatorEngine();

    @Before
    public void runBeforeEveryTest() {
        calcEngine.currentTotal = 0;
    }

    @Test
    public void testGetTotalStringInt() throws Exception {
        calcEngine.currentTotal = 50;
        assertTrue(calcEngine.currentTotal % 1.0 == 0);
    }

    @Test
    public void testGetTotalStringDouble() throws Exception {
        calcEngine.equal("50.5");
        assertTrue(calcEngine.currentTotal % 1.0 != 0);
    }

    @Test
    public void testEqual() throws Exception {
        calcEngine.equal("20");
        assertEquals(calcEngine.currentTotal, 20.0);
    }

    @Test
    public void testAdd() throws Exception {
        calcEngine.add(20);
        assertEquals(calcEngine.currentTotal, 20.0);

    }

    @Test
    public void testSubtract() throws Exception {
        calcEngine.subtract(20);
        assertEquals(calcEngine.currentTotal, -20.0);
    }

    @Test
    public void testMultiplyByZero() throws Exception {
        calcEngine.multiply(10);
        assertEquals(calcEngine.currentTotal, 0.0);
    }

    @Test
    public void testDivideByZero() throws Exception {
        calcEngine.divide(10);
        assertEquals(calcEngine.currentTotal, 0.0);
    }

    @Test
    public void testMultiply() throws Exception {
        calcEngine.add(10);
        calcEngine.multiply(10);
        assertEquals(calcEngine.currentTotal, 100.0);
    }

    @Test
    public void testDivide() throws Exception {
        calcEngine.add(10);
        calcEngine.divide(10);
        assertEquals(calcEngine.currentTotal, 1.0);
    }
}

Do you mean the default access modifier (i.e. none specified) by package-private? If you can offer your CalculatorEngine's code as another review then I'm sure some of us can give you good-quality advice there. As pointed out by OP, it's actually in this question: Simple calculator in Java using Swing and AWT. :)

Running down your list of tests...

First, why do all your tests throws Exception? Are they expected to? If they are, shouldn't you want JUnit to catch it and assert the right Exception is thrown? Your tests should be written in the minimal way possible, as you can also think of it as a form of documentation describing what your methods can or cannot do. If another developer (e.g. "You" six months down the road) sees the throws Exception, they may be mislead into thinking "an Exception may be thrown here... somehow.".

@Before
public void runBeforeEveryTest() {
    calcEngine.currentTotal = 0;
}

Your CalculatorEngine should probable have a reset() method for this. Manipulating a field directly in this way is prone to errors.

@Test
public void testGetTotalStringDouble() throws Exception {
    calcEngine.equal("50.5");
    assertTrue(calcEngine.currentTotal % 1.0 != 0);
}

So what does this do? Presumably, equal("50.5") sets currentTotal as 50.5, and we're testing that it's not an integer...? I wouldn't be worried about this. If the intention is to test whether equal() accepts a decimal-number-as-a-String, test that directly.

@Test
public void testMultiplyByZero() throws Exception {
    calcEngine.multiply(10);
    assertEquals(calcEngine.currentTotal, 0.0);
}

@Test
public void testDivideByZero() throws Exception {
    calcEngine.divide(10);
    assertEquals(calcEngine.currentTotal, 0.0);
}

The method names are misleading here, because the argument you use isn't zero, and your other method names do not read as e.g. addToZero(). Originally, I thought the division test was a division by zero test, and wanted to point out that doesn't give 0.0... Testing should involve edge cases too.

@Test
public void testMultiply() throws Exception {
    calcEngine.add(10);
    calcEngine.multiply(10);
    assertEquals(calcEngine.currentTotal, 100.0);
}

@Test
public void testDivide() throws Exception {
    calcEngine.add(10);
    calcEngine.divide(10);
    assertEquals(calcEngine.currentTotal, 1.0);
}

Ok, so now these are the 'normal' multiplication and division tests... It's still slightly odd that you have to do an 'add' first, which seems to be a merely convenient method (pun intended) of 'initializing' currentTotal as 10. You may want to look into this particular API design too.


Text file reading and printing data MyText.txt

This is line 1
This is line 2
Time Taken for writing this#      0 days 0 hrs 1 min 5 sec
Nothing Important
Sample Text

Objective

To read the text file and find if "Sample Test is present in the file. If present print the time taken to write the file (which is a value already inside the file)"

My Code

with open('MyText.txt', 'r') as f:
    f.readline()
    for line in f:
        if 'Sample Text' in line:
            print "I have found it"
            f.seek(0)
            f.readline()
            for line in f:
                if 'Time Taken' in line:
                    print line
                    print ' '.join(line.split())
f.close()

The code is working fine.  I want to know if this code can be made even better. Considering I am new to Python, I am sure there would be a better way to code this.  Can anyone suggest alternative/faster approaches for this?

as you say, it does look like this code is doing exactly what you want. But it also looks like you are repeating yourself in a couple of places.

If it was me, I would prefer to loop through the file only once and check for both Sample Text and Time Taken at the same time, and then only print out if the sample text is there:

def my_read(my_text):
    time_taken, sample_text = False, False
    with open(my_text) as f:
        for i in f:
            time_taken = ' '.join(i.split()) if 'Time Taken' in i else time_taken
            sample_text = 'I have found it!' if 'Sample Text' in i else sample_text

    # now, if sample text was found, print out
    if sample_text:
        print sample_text
        print time_taken
    return

my_read('MyText.txt')

By testing for both, you avoid having to use f.seek(0) and start looping through the file again. Of course, this is based on the sample input above - and assumes that there isn't multiple lines with 'Time taken' or 'Sample Text' in them.

Hope this helps


Python Stack Implementation This is my Stack implementation in Python.

class Stack:
    def __init__(self):
        self.head = None
        self.size = 0

    def push(self, item):
        node = Node(item)
        if not self.head:
            self.head = node
        else:
            node.next = self.head
            self.head = node
        self.size += 1

    def pop(self):
        if self.size == 0:
            raise ValueError('Popping off an empty stack!')
        item = self.head.val
        self.head = self.head.next
        return item

    def peek(self):
        if self.size == 0:
            raise ValueError('Peeking into an empty stack!')
        return self.head.val

    def __iter__(self):
        return self

    def __next__(self):
        if self.head:
            curr = self.head
        else:
            raise StopIteration()
        self.head = self.head.next
        return curr.val

class Node:
    def __init__(self, val):
        self.val = val
        self.next = None

Need comments on primarily 3 aspects -

Is there anything, just plain wrong?
How to improve efficiency of this design?
What more can I add to make it an actually usable data structure beyond the ADT specification?

In terms of implementation, and making it more efficient, I don't think the linked list adds any particular value. I would rather see you implement the stack using a built-in list, which will improve both memory performance and speed.

So far, you have implemented push, pop, peek and iteration. Why iteration? I'm glad that it's destructive, but I don't understand why you felt you needed that.

I would suggest that you look at Python's Common Sequence Operations to see which you can implement. If you choose not to make the stack transparent, that's okay, but it might limit applicability. A transparent stack should be able to support in, count, maybe indexing, etc.

Likewise, have a look at the Mutable Sequence Types section and common operations. Also, check out the Abstract Base Classes for collections, and add whatever parent class(es) you implement.

Keep in mind that Python is a "consenting adult language." There is not a great regard for private data and access control. Instead, the expectation is that you will implement as much as you can, and not be surprised if someone wants to peek. So, I'd suggest trying to build as transparent and well-integrated a class as possible: allow accessing items on the stack via s[i] notation, support things like .find() and .count(), and inherit from whatever ABCs you support. If your stack is iterable, make sure you implement an Iterable abc. (Or Sequence, or Collection, or ...)


Simple event dispatcher Here is my very first event dispatcher. I would like to get both, style and code review, as well as some ideas to improve this implementation (new features etc.)

I tried to write code in C++17 style as much as possible.

IEventDispatcher.h

#pragma once

#include <map>
#include <functional>

template<typename EventType>
class IEventDispatcher
{
public:
    using EventHandler = std::function<void()>;

    virtual void addEventListener(EventType eventToAdd, EventHandler handler) = 0;
    virtual void removeEventListener(EventType eventToRemove) = 0;
    virtual void dispatch(EventType eventToDispatch) = 0;
};

EventDispatcher.hpp

#pragma once

#include "IEventDispatcher.h"

template<typename EventType>
class EventDispatcher : public IEventDispatcher<EventType>
{
public:
    using EventHandler = std::function<void()>;

    EventDispatcher();
    ~EventDispatcher();

    void addEventListener(EventType eventToAdd, EventHandler handler);
    void removeEventListener(EventType eventToRemove);
    void dispatch(EventType eventToDispatch);

private:
    std::multimap<EventType, EventHandler> eventListeners;
};

template<typename EventType>
EventDispatcher<EventType>::EventDispatcher()
{}

template<typename EventType>
EventDispatcher<EventType>::~EventDispatcher()
{}

template<typename EventType>
void EventDispatcher<EventType>::addEventListener(EventType eventToAdd, EventHandler handler)
{
    eventListeners.emplace(eventToAdd, handler);
}

template<typename EventType>
void EventDispatcher<EventType>::removeEventListener(EventType eventToRemove)
{
    eventListeners.erase(eventToRemove);
}

template<typename EventType>
void EventDispatcher<EventType>::dispatch(EventType eventToDispatch)
{
    for (const auto &[event, handler] : eventListeners)
    {
        if (event == eventToDispatch)
        {
            handler();
        }
    }
}

Design

Is there a need for IEventDispatcher? Defining it as an abstract base class doesn't provide much utility if there's only ever going to be one derived class.
Is there any requirement for EventDispatcher::eventListeners to be of type std::multimap<EventType, EventHandler>? It doesn't seem like the ordering of elements has much relevance, so a std::unordered_multimap<EventType, EventHandler> might provide better performance. But: The current code doesn't even use any std::multimap specific functionality, so the easiest solution might actually be a std::unordered_map<EventType, std::vector<EventHandler>>.
There is no way to remove a specific EventHandler, only all EventHandlers registered for an EventType.
There's no way to pass arguments to an event handler. This might be necessary for some use cases, e.g. to indicate what changed.

  There's no easy way to provide both general event dispatching (handling multiple different EventTypes) and event-specific parameter passing (different parameter types for different events). Some solutions:

  Redefining the scope of EventDispatcher: Instead of handling multiple events in each EventDispatcher instance each instance is only responsible for handling one specific EventType. The parameter types can then be taken as template parameters.
  Make EventType a base class and inherit from it when parameters need to be passed (so more like EventArgs in C#). Requires EventType instances to be passed as references or pointers, and EventHandlers need to downcast internally to the concrete derived class. Many possible ways to get this wrong, and the compiler might not be able to help much.

Implementation issues

EventDispatcher::dispatch iterates over all entries in EventDispatcher::eventListeners. Why not use std::multimap::equal_range to then only iterate over all entries that match?

  This would be \$\mathcal{O}(k + \log n)\$ on average instead of \$\mathcal{O}(n)\$ (where \$n\$ denotes the number of all elements in the container, and \$k\$ denotes all elements for the relevant key), or \$\mathcal{O}(k)\$ on average for std::unordered_multimap.

  The proposed replacement std::unordered_map<EventType, std::vector<EventHandler>> would also have complexity \$\mathcal{O}(k)\$ (\$\mathcal{O}(1)\$ to find the vector, \$\mathcal{O}(k)\$ to iterate it) - even in the worst case.

Rule of Five violation: A custom destructor is declared for EventDispatcher, but no custom copy constructor, copy assignment operator, move constructor and move assignment operators are declared. This prevents the compiler from providing default implementations for those (or soon will, it currently is deprecated for compilers to do so in case of copy constructor and copy assignment operator).

  It would be easiest to just use the default compiler-provided destructor.

Inconsistent naming: IEventDispatcher.h has file extension .h, whereas EventDispatcher.hpp has file extension .hpp.
In EventDispatcher, all member functions inherited and overridden from IEventDispatcher lack the override keyword.
EventDispatcher::dispatch could be const.