Opening and closing active items I'm trying to have the 3 buttons to each open its own list below whilst closing any current active lists open. I've tried my best with the jQuery, but it seems so bloated. Is there a way to streamline this jQuery code?

At the moment I have a very rough working example.

/***********************
Mobile Menus
*********************************/

(function() {
   var nav = $('.social_icons_list ul');
    $.fn.navtoggle = function(speed) {
         var $this = $(this);

        return $this.animate({
            'height': 'toggle'
        }, 500);
    }

    $('.socialNav').on('click', function() {
        nav.stop(true, true).navtoggle(300);
        $("ul").not(nav).hide();
        e.preventDefault();
    });
})();

(function() {
   var nav = $('.dogz_list ul');
    $.fn.navtoggle = function(speed) {
         var $this = $(this);

        return $this.animate({
            'height': 'toggle'
        }, 500);
    }

    $('.dogzNav').on('click', function() {
        nav.stop(true, true).navtoggle(300);
        $("ul").not(nav).hide();
        e.preventDefault();
    });
})();

 (function() {
   var nav = $('.navigation_list ul');
    $.fn.navtoggle = function(speed) {
         var $this = $(this);

        return $this.animate({
            'height': 'toggle'
        }, 500);
    }

    $('.Nav').on('click', function() {
        nav.stop(true, true).navtoggle(300);
        $("ul").not(nav).hide();
        e.preventDefault();
    });
})();

I strongly recommend refactoring your HTML. At the moment it works, and I have based a solution around your HTML, however I strongly recommend changing it into nested ul tags to make it easier to manipulate with jquery e.g.

Item a

Sub item

Another sub-item

That said:

I have removed display: none from your UL and am using JS to hide the nav divs on page load (this can be moved to the css, but this is just proof of concept).

The javascript has paired the class of the a tag to the class of the nav div in the path variable.

When the user clicks on the nav link, all nav divs are hidden and the corresponding nav div for the clicked link is animated open.

var path = {
  'socialNav':'social_icons_list',
  'dogzNav':'dogz_list',
  'Nav':'navigation_list',
};

var navDivs = $('.social_icons_list,.dogz_list,.navigation_list');

$(document).on('ready', function(){
  navDivs.hide();
  $('a.socialNav, a.dogzNav, a.Nav').on('click', function(){
    navDivs.hide();
    navDivs.filter('.' + path[$(this).attr('class')]).slideDown(500);
  });
});
body {
  background: #ccc;
  font-family: 'proxima_nova_rgregular';
  color: #524a45;
}
.header {
  height: 129px;
  border-bottom: 1px solid #666;
}
.header .container {
  padding: 0;
}
/*social_icons desktop version*/
.social_icons {
  float: left;
  margin-top: 25px;
  width: 90%;
}
.social_icons .twitter {
  float: left;
  width: 24px;
  height: 50px;
  display: block;
  margin: 0 32px 0 0;
  background: url(../img/logos_icons/twitter.svg) no-repeat 0 center;
}
.social_icons .twitter:hover {
  background: url(../img/logos_icons/twitter_hover.svg) no-repeat 0 center;
  cursor: pointer;
}
.social_icons .facebook {
  float: left;
  width: 24px;
  height: 50px;
  display: block;
  margin: 0 32px 0 0;
  background: url(../img/logos_icons/fb.svg) no-repeat 0 center;
}
.social_icons .facebook:hover {
  background: url(../img/logos_icons/fb_hover.svg) no-repeat 0 center;
  cursor: pointer;
}
.social_icons .pinterest {
  float: left;
  width: 24px;
  height: 50px;
  display: block;
  margin: 0 32px 0 0;
  background: url(../img/logos_icons/pin.svg) no-repeat 0 center;
}
.social_icons .pinterest:hover {
  background: url(../img/logos_icons/pin_hover.svg) no-repeat 0 center;
  cursor: pointer;
}
.social_icons .instagram {
  float: left;
  width: 24px;
  height: 50px;
  display: block;
  margin: 0 32px 0 0;
  background: url(../img/logos_icons/insta.svg) no-repeat 0 center;
}
.social_icons .instagram:hover {
  background: url(../img/logos_icons/insta_hover.svg) no-repeat 0 center;
  cursor: pointer;
}
.logo {
  position: absolute;
  float: left;
  background: url(../img/logos_icons/logo_mobile.svg) no-repeat center center;
  width: 100%;
  height: 115px;
  display: block;
  margin-top: 10px;
}
.hold {
  float: left;
  width: 100%;
  margin-top: 25px;
  display: none;
}
.bag {
  float: right;
  width: 47px;
  height: 36px;
  background: #fff url(../img/logos_icons/bag.svg) no-repeat 7px center;
  border-radius: 5px;
  border: 1px solid #c6c6c5;
  padding: 7px;
  text-align: right;
}
.nav {
  margin: 0;
  float: left;
  width: 100%;
}
.navbar-nav > li {
  float: none;
  margin: 0;
}
.navbar-inverse .navbar-nav > li > a {
  color: #524a45;
  text-transform: uppercase;
  font-family: 'proxima_nova_rgregular';
  letter-spacing: 1.5px;
  padding: 15px 15px;
}
.dropdown-menu > li > a {
  color: #524a45;
  text-transform: uppercase;
  font-family: 'proxima_nova_rgregular';
  letter-spacing: 1.5px;
  clear: both;
  color: #333;
  display: block;
  font-weight: normal;
  line-height: 1.42857;
  padding: 20px 20px;
  white-space: nowrap;
}
.navbar-inverse .navbar-nav > .active > a,
.navbar-inverse .navbar-nav > .active > a:hover,
.navbar-inverse .navbar-nav > .active > a:focus {
  color: #524a45;
  background-color: #fff;
}
.navbar-inverse .navbar-nav > li > a:hover,
.navbar-inverse .navbar-nav > li > a:focus {
  color: #524a45;
  /*border-bottom:1px solid #524a45;*/
  background: #524a45;
  color: #fff;
}
.dropdown-menu {
  padding: 0;
}
.dropdown-menu > li > a:hover,
.dropdown-menu > li > a:focus {
  background-color: #efefee;
}
.navbar-inverse .navbar-nav .open .dropdown-menu > li > a:hover,
.navbar-inverse .navbar-nav .open .dropdown-menu > li > a:focus {
  background-color: transparent;
  color: #524a45;
}
.navbar {
  background: #fff;
  min-height: 178px;
}
.navbar-header {
  float: none;
  background: #fff;
}
.navbar-collapse.collapse {} .navbar.shrink {
  min-height: 135px;
}
.navbar.shrink * {
  transition: 1s;
}
.navbar.shrink .header {
  height: 85px;
}
.navbar.shrink .logo {
  background: url(../img/logos_icons/logo_small.svg) no-repeat top 10px center;
  margin: 0
}
.navbar.shrink .navbar-brand {
  font-size: 25px;
}
.navbar-toggle {
  display: block;
}
.nav > li {
  display: block;
  position: relative;
}
.navbar-inverse .navbar-collapse,
.navbar-inverse .navbar-form {
  border: none;
}
.navbar-collapse {
  padding: 0;
}
.navbar-inverse .navbar-toggle {
  border-color: #c6c6c5;
  border-radius: 5px;
}
.navbar-inverse .navbar-toggle:hover,
.navbar-inverse .navbar-toggle:focus {
  background-color: #524a45;
}
.navbar-inverse .navbar-toggle:hover .icon-bar {
  background-color: #fff;
}
/* button mobile social_icons */
.navbar-header > .social_icons {
  display: block;
  margin: 10px;
  width: 20%;
}
.navbar > .social_icons_list ul {
  padding: 0;
  margin-top: 10px;
  width: 100%;
  background: #eee;
  float: left;
}
.navbar > .social_icons_list ul li {
  display: block;
  float: none;
  height: 50px;
  width: 100%;
}
.navbar-header > .social_icons a.socialNav {
  display: block;
  background: url(../img/logos_icons/mobile_social_icon.svg) no-repeat center center;
  width: 47px;
  height: 36px;
  border-radius: 5px;
  border: 1px solid #c6c6c5;
}
.navbar-header > .social_icons a.socialNav:hover {
  background: #524a45 url(../img/logos_icons/mobile_social_icon_hover.svg) no-repeat center center;
  width: 47px;
  height: 36px;
  border-radius: 5px;
  border: 1px solid #c6c6c5;
}
.social_icons_list ul li a {
  padding-left: 50px;
  display: block;
  height: 50px;
  padding-left: 50px;
  line-height: 50px;
  color: #524a45;
  text-transform: uppercase;
  letter-spacing: 1.5px;
}
.social_icons_list .twitter {
  background: url(../img/logos_icons/twitter.svg) no-repeat 10px center;
}
.social_icons_list .twitter:hover {
  color: #fff;
  background: #524a45 url(../img/logos_icons/twitter_hover.svg) no-repeat 10px center;
  cursor: pointer;
}
.social_icons_list .facebook {
  background: url(../img/logos_icons/fb.svg) no-repeat 10px center;
}
.social_icons_list .facebook:hover {
  color: #fff;
  background: #524a45 url(../img/logos_icons/fb_hover.svg) no-repeat 10px center;
  cursor: pointer;
}
.social_icons_list .pinterest {
  background: url(../img/logos_icons/pin.svg) no-repeat 10px center;
}
.social_icons_list .pinterest:hover {
  color: #fff;
  background: #524a45 url(../img/logos_icons/pin_hover.svg) no-repeat 10px center;
  cursor: pointer;
}
.social_icons_list .instagram {
  background: url(../img/logos_icons/insta.svg) no-repeat 10px center;
}
.social_icons_list .instagram:hover {
  color: #fff;
  background: #524a45 url(../img/logos_icons/insta_hover.svg) no-repeat 10px center;
  cursor: pointer;
  float: left;
}
/* button mobile dogz */
.navbar-header > .dogz {
  display: block;
  margin: 10px;
  width: 20%;
  float: left;
}
.navbar > .dogz_list {
  display: block;
  width: 100%;
  float: left;
}
.navbar > .dogz_list ul {
  padding: 0;
  margin-top: 10px;
  width: 100%;
  background: #eee;
}
.navbar > .dogz_list ul ul {
  padding: 0 0 0 20px;
  margin: 0;
}
.navbar > .dogz_list ul li {
  display: block;
  float: none;
  width: 100%;
}
.navbar > .dogz_list ul li > a {
  color: #524a45;
  text-transform: uppercase;
  font-family: 'proxima_nova_rgregular';
  letter-spacing: 1.5px;
  clear: both;
  color: #333;
  display: block;
  font-weight: normal;
  line-height: 1.42857;
  padding: 15px;
  white-space: nowrap;
}
.navbar > .dogz_list ul li > a:hover {
  color: #fff;
  background: #524a45;
}
.navbar-header > .dogz a.dogzNav {
  display: block;
  background: url(../img/logos_icons/paw.svg) no-repeat center center;
  width: 47px;
  height: 36px;
  border-radius: 5px;
  border: 1px solid #c6c6c5;
}
.navbar-header > .dogz a.dogzNav:hover {
  background: #524a45 url(../img/logos_icons/paw_hover.svg) no-repeat center center;
  width: 47px;
  height: 36px;
  border-radius: 5px;
  border: 1px solid #c6c6c5;
}
.navbar-header > .hold {
  float: left;
  margin-top: 10px;
  width: 20%;
  display: block;
}
.navbar-header > .dogz a.Nav {
  display: block;
  background: url(../img/logos_icons/paw.svg) no-repeat center center;
  width: 47px;
  height: 36px;
  border-radius: 5px;
  border: 1px solid #c6c6c5;
}
.navbar-header > .dogz a.Nav:hover {
  background: #524a45 url(../img/logos_icons/paw_hover.svg) no-repeat center center;
  width: 47px;
  height: 36px;
  border-radius: 5px;
  border: 1px solid #c6c6c5;
}
.navbar > .navigation_list {
  display: block;
  width: 100%;
  float: left;
}
.navbar > .navigation_list ul {
  padding: 0;
  margin-top: 10px;
  width: 100%;
  background: #eee;
}
.navbar > .navigation_list ul li ul {
  display: none;
}
.navbar > .navigation_list ul li ul li a {
  color: #ccc;
}
.navbar > .navigation_list ul ul {
  padding: 0;
  margin: 0;
}
.navbar > .navigation_list ul li {
  display: block;
  float: none;
  width: 100%;
}
.navbar > .navigation_list ul li > a {
  color: #524a45;
  text-transform: uppercase;
  font-family: 'proxima_nova_rgregular';
  letter-spacing: 1.5px;
  clear: both;
  color: #333;
  display: block;
  font-weight: normal;
  line-height: 1.42857;
  padding: 15px;
  white-space: nowrap;
}
.navbar > .navigation_list ul li > a:hover {
  color: #fff;
  background: #524a45;
}

0

      social

      dognav

      nav

About

FAQ's

Gifting

Her

Product 1

Product 2

Product 3

His

Product 1

Product 2

Product 3

Our Craft

Blog

Contact

Tweet

Like

Pin

Share

Her

His


Design Pattern for initializing Properties of a ViewModel which only serves a display purpose It's a question which lies me for years on the tongue.

Given a LoginViewModel:

public class LoginViewModel : ViewModelBase
{
    public string Email { get; set; }
    public string Password { get; set; }
    public bool RememberMe { get; set; }
    public string ReturnUrl { get; set; }

    public IEnumerable<AuthenticationScheme> AuthenticationSchemes { get; set; }
}

Typically in Mvc, you need this ViewModel in a GET Request, and (if the request failed) in a POST request.

So here we have a simple LoginController:

public class LoginController : Controller
{
    private readonly ConmaniaSignInManager _signInManager;

    public LoginController(ConmaniaSignInManager signInManager)
    {
        _signInManager = signInManager;
    }

    [HttpGet]
    public async Task<IActionResult> Index(string returnUrl = null)
    {
        var model = new LoginViewModel
        {
            ReturnUrl = returnUrl,
            AuthenticationSchemes = await _signInManager.GetExternalAuthenticationSchemesAsync() //call 1
        };
        return View(model);
    }

    [HttpPost]
    public async Task<IActionResult> Index(LoginViewModel model)
    {
        if (!ModelState.IsValid)
        {
            model.AuthenticationSchemes = await _signInManager.GetExternalAuthenticationSchemesAsync(); //call 2
            return View(model);
        }
        var result = await _signInManager.PasswordSignInAsync(model.Email, model.Password, model.RememberMe, true);
        if (result.Succeeded)
        {
            return LocalRedirect(Url.GetLocalUrl(Url.GetLocalUrl(model.ReturnUrl)));
        }

        //... Removed for readability

        model.AuthenticationSchemes = await _signInManager.GetExternalAuthenticationSchemesAsync(); //call 3
        ModelState.AddModelError(string.Empty, "Invalid login attempt.");
        return View(model);
    }
}

The first 4 properties of the view model doesn't matter, since they are fields which are getting populated on Post by the ModelBinder.

However the property AuthenticationSchemes gets populated 3 times, even in this simple Login case. Other ViewModels in a typical application can have many more such properties.

In previous Versions of Mvc I have cheated by retrieving the data in the ViewModel by using the getter. That was ofcorse bad design.

So my concern is, is there any Design Pattern to make my controller less painfull (DRY etc)?

I thought about making a Method void PopulateLoginViewModel(LoginViewModel model), and calling this 3 times* instead, but I am not sure if this would be correct.

I also thought about a Service which is injected into the View which has a GetExternalAuthenticationSchemesAsync() method, so the ViewModel does not need to know it. But from what I have read, that is also bad practise.

* I do not want to populate the ViewModel if the user will get redirected anyway.

More of a flow problem. Populating the property is necessary for the GET. No getting around that even if you were to create a function to do it for you. The POST however can be restructured to make only one call.

public class LoginController : Controller {
    private readonly ConmaniaSignInManager _signInManager;

    public LoginController(ConmaniaSignInManager signInManager) {
        _signInManager = signInManager;
    }

    private async Task populateLoginViewModel(LoginViewModel model) {
        model.AuthenticationSchemes = await _signInManager.GetExternalAuthenticationSchemesAsync();
    }

    [HttpGet]
    public async Task<IActionResult> Index(string returnUrl = null) {
        var model = new LoginViewModel {
            ReturnUrl = returnUrl
        };
        await populateLoginViewModel(model); //call 1
        return View(model);
    }

    [HttpPost]
    public async Task<IActionResult> Index(LoginViewModel model) {
        if (ModelState.IsValid) {
            var result = await _signInManager.PasswordSignInAsync(model.Email, model.Password, model.RememberMe, true);
            if (result.Succeeded) {
                return LocalRedirect(Url.GetLocalUrl(Url.GetLocalUrl(model.ReturnUrl)));
            }

            ModelState.AddModelError(string.Empty, "Invalid login attempt.");
        }
        //...if we get this far something was wrong. repopulate model and return
        await populateLoginViewModel(model); //call 2
        return View(model);
    }
}

If the model state is not valid then ModelState is already populated. If the login attempt was unsuccessful then we populate the model state and continue to the end of the action.


Two Java methods for easy writing and reading bytes in files I have these easy-to-use methods for reading and writing binary data in files.

FileTools.java:

package net.coderodde.file;

import java.io.BufferedOutputStream;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.FileOutputStream;
import java.io.IOException;
import java.util.Objects;

/**
 * This class contains utility methods for writing and reading binary data in
 * files.
 *
 * @author Rodion "rodde" Efremov
 * @version 1.6 (Feb 29, 2016)
 */
public class FileTools {

    /**
     * This method returns the byte array that represent the contents of
     * {@code file}.
     *
     * @param  file the file to read.
     * @return the array of bytes representing the contents of the input file.
     */
    public static byte[] readFile(File file)
    throws IOException, FileNotFoundException {
        Objects.requireNonNull(file, "The input file is null.");
        long size = file.length();
        checkSize(size);

        byte[] data;
        int bytesRead;

        try (FileInputStream stream = new FileInputStream(file)) {
            data = new byte[(int) size];
            bytesRead = stream.read(data);
        }

        if (bytesRead != size) {
            throw new IllegalStateException(
                    "File size and read count mismatch. File size: " +
                    size + ", bytes read: " + bytesRead);
        }

        return data;
    }

    /**
     * Writes the byte array {@code data} to the file {@code file}. After
     * successful operation of this method, the input file will contain exactly
     * the contents of the input data.
     *
     * @param file the file to write to.
     * @param data the data array to write.
     * @throws java.io.IOException           if file IO fails.
     * @throws java.io.FileNotFoundException if file does not exist.
     */
    public static void writeFile(File file, byte[] data)
    throws IOException, FileNotFoundException {
        Objects.requireNonNull(file, "The input file is null.");
        Objects.requireNonNull(data, "The input data to write is null.");

        try (BufferedOutputStream stream = new BufferedOutputStream(
                                           new FileOutputStream(file))) {
            stream.write(data);
        }
    }

    // This method ensures that file size is small enough to be represented
    // using a variable of type 'int'.
    private static final void checkSize(long size) {
        if (size > Integer.MAX_VALUE) {
            throw new IllegalStateException(
                    "The target file is too large: " + size + " bytes. " +
                    "Maximum allowed size is " + Integer.MAX_VALUE +
                    "bytes.");
        }
    }
}

FileToolsTest.java:

package net.coderodde.file;

import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.util.Arrays;
import java.util.Random;
import org.junit.Test;
import static org.junit.Assert.*;

public class FileToolsTest {

    private static final int ITERATIONS = 100;
    private static final int MAXIMUM_DATA_LENGTH = 20_000;

    @Test
    public void test() {
        long seed = System.nanoTime();
        Random random = new Random(seed);
        System.out.println("Seed = " + seed);

        for (int i = 0; i < ITERATIONS; ++i) {

            File file = new File("funky.txt");
            byte[] data = new byte[random.nextInt(MAXIMUM_DATA_LENGTH)];
            random.nextBytes(data);

            try {
                FileTools.writeFile(file, data);
                byte[] result = FileTools.readFile(file);
                assertTrue(Arrays.equals(result, data));
            } catch (FileNotFoundException ex) {
                fail(ex.getMessage());
            } catch (IOException ex) {
                fail(ex.getMessage());
            }

            file.delete();
        }
    }
}

Please, tell me anything that comes to mind.

From your "reinventing-the-wheel" tag, I assume you're already aware of Guava's and Apache Commons' utilities. So my only nitpick is that the int cast might be clearer if it was closer to the size check:

long sizeLong = file.length();
checkSize(sizeLong);
int size = (int)sizeLong;

Or if you want to be a little more "clever":

int size = checkSize(file.length());
//...
private static final int checkSize(long size) {
    //...
    return (int)size;
}


Program that requests two floating-point numbers and prints the value of their difference divided by their product From what I understand this site help to check if my solution is proper or not..so forgive me if the header is bad.

This is my solution for the question:

#include <stdio.h>

int main()

{

    double num1, num2, different, product, answer;

    printf("please enter 2 floatig point numbers:\n");
    printf("number one is?\n");
    scanf("%lf", &num1);
    printf("number two is?\n");
    scanf("%lf", &num2);

    if (num1 > num2)
    {
        different = num1 - num2;
    }

    if (num2 > num1)

    {
        different = num2 - num1;
    }

    if (num1 == num2)

    {
        different = 0;
    }

    product = num1*num2;

    answer = different/product;

    printf("%lf", answer);

}

How bad is it?

For a newbie, your solution is ok. You can simplify it quite a bit though by using the function fabs. It's a fairly simple function: fabs(x) returns x if x >= 0, and returns -x if x < 0. So all of your if checks can be replaced by the line:

different = fabs(num1 - num2);

Note that you need to #include <math.h> for this.

Also, what happens if one of your numbers is 0? Then your product will also be 0 and your program will crash. It's always a good idea to add some basic checks. Unfortunately, with floating point numbers this can be a bit tricky, so simply doing something like:

if(product == 0.0) { ... }

may not actually work! Fixing this might be a bit tricky for someone who is just learning, but it's something to always keep in mind.

Finally, main should return an int - generally, 0 is what is returned if everything works as it should - so down the bottom should be a return 0;.


Random permutation of int[] and ArrayList I wrote a simple program using int[] and ArrayList<Integer> which aims to get a random permutation output between 1 to 10, where each number will not be repeated in each line of output (each line of output will have number 1 until 10 in a different order). Both classes work nicely with the desire output. Is there any other possible way to shorten my code?

Random arrays using brute force:

  3 5 2 9 7 8 10 1 4 6
  4 10 5 2 3 9 6 1 7 8
  7 9 1 10 3 8 5 4 2 6
  5 3 8 9 10 2 6 7 1 4
  8 5 9 7 10 6 4 2 1 3
  4 10 2 9 8 6 3 7 5 1
  8 9 10 2 5 4 6 7 3 1
  7 2 4 9 5 10 6 8 1 3
  9 2 8 1 10 7 6 4 5 3
  8 1 10 3 9 7 4 2 5 6

This is my BrutePermutationGenerator file:

import java.util.Random;

public class BrutePermutationGenerator
{

  public int[] getRandomPermutation()
  {
  Random rand = new Random();
  int[] array = new int[10];

  int r = 0;
  int count = 0;
  boolean fill;

  int low = 1;
  int high = 10;
  int range = high - low + 1;

  do
  {
    fill = true;
    r = rand.nextInt(10) + 1;

     for (int i = 0; i < array.length; i++)//loop for random number between 1 to 10
     {
        if (array[i] == r)
        {
           fill = false;
        }
     }

     if (fill == true)
     {
        array[count] = r;
        count++;
     }
   }
   while (count < 10);
      for (int i = 0; i < array.length; i++)//loop for element in array
      {
         System.out.print(array[i] + " ");
      }
   return array;
  }

public void nextPermutation()
 {
   for (int i = 0; i < 10; i++) //loop for permutation 10 times
   {
      getRandomPermutation();
      System.out.println();
   }
  }
}

This is my SmartPermutationGenerator file:

import java.util.ArrayList;
import java.util.Random;

public class SmartPermutationGenerator
{
   private int size;
   private Random rand = new Random();

   public SmartPermutationGenerator()
   {
      this.size = 10;
   }

   public ArrayList<Integer> getRandomPermutation()
   {
      ArrayList<Integer> unused = new ArrayList<>();

      for (int i = 0; i < size; i++) // loop for element in array
      {
         unused.add(i + 1);
      }

      ArrayList<Integer> perm = new ArrayList<>();

      for (int k = 0; k < size; k++)  //loop for random number between 1 to 10
     {
         int pos = rand.nextInt(unused.size());
         perm.add(unused.get(pos));
         unused.remove(pos);
     }

    return perm;
    }

public void nextPermutation()
{
   for(int i = 0; i < size; i++) //loop for permutation 10 times
   {
       for(Integer item : getRandomPermutation())
       {
           System.out.print(item + " ");
       }
    System.out.println();
   }
 }
}

This is my PermutationGeneratorViewer file:

public class PermutationGeneratorViewer
{
   public static void main(String[] args)
   {
      BrutePermutationGenerator brute = new BrutePermutationGenerator();
      SmartPermutationGenerator smart = new SmartPermutationGenerator();

      System.out.println("\n" + "Random arrays using Brute Force: ");
      brute.nextPermutation();

      System.out.println("\n" + "Random arrays using Smart Force: ");
      smart.nextPermutation();
   }
}

Use for-each

Use for-each idiom to loop over an array when possible, and you can drop this comment:

//loop for element in array
for (int i = 0; i < array.length; i++) {
    System.out.print(array[i] + " ");
}

Like this, and without the pointless comment:

for (int item : array) {
    System.out.print(item + " ");
}

Use a different list implementation for unused

In SmartPermutationGenerator,
you're using an ArrayList for the unused numbers,
and randomly remove items from it.
An ArrayList is backed by an array,
and removing items from arrays is an expensive operation:
elements after the removed element must be moved to fill the gap.
You can improve that point by using a LinkedList instead,
where removal of elements is cheap.

public List<Integer> getRandomPermutation() {
    List<Integer> unused = new LinkedList<>();
    for (int i = 0; i < size; i++) {
        unused.add(i + 1);
    }

    List<Integer> permutation = new ArrayList<>();
    for (int k = 0; k < size; k++) {
        int pos = random.nextInt(unused.size());
        permutation.add(unused.get(pos));
        unused.remove(pos);
    }

    return permutation;
}

Use boolean expressions directly without ==

You don't need to write == true when evaluating boolean expressions like this:

if (fill == true) {

This is exactly the same but shorter, simpler, intuitive:

if (fill) {

Remove unused code

These variables are completely unused, remove them:

  int low = 1;
  int high = 10;
  int range = high - low + 1;

Printing arrays

In both implementations, you're using a for loop to print arrays.
There's are easier ways.

The toString method of List implementations gives a nice representation,
so you can print them directly:

List<Integer> list = Arrays.asList(1, 2, 3, 4, 5);
System.out.println(list);
// prints: [1, 2, 3, 4, 5]

To print arrays in the same format, you can use Arrays.toString:

int[] arr = {1, 2, 3, 4, 5};
System.out.println(Arrays.toString(arr));
// prints: [1, 2, 3, 4, 5]

Formatting

Your code doesn't follow the common formatting used by auto-formatting of editors like Eclipse, IntelliJ, NetBeans. Instead of this:

  do
  {
    fill = true;
    r = rand.nextInt(10) + 1;

     for (int i = 0; i < array.length; i++)//loop for random number between 1 to 10
     {
        if (array[i] == r)
        {
           fill = false;
        }
     }

Following the same style as recommended by these tools,
your code would look like this:

do {
    fill = true;
    r = rand.nextInt(10) + 1;

    for (int item : array) {
        if (item == r) {
            fill = false;
        }
    }


Detecting cycles in a directed graph without using mutable sets of nodes I recently came across the classic algorithm for detecting cycles in a directed graph using recursive DFS. This implementation makes use of a stack to track nodes currently being visited and an extra set of nodes which have already been explored. The second set is not strictly required, but it is an optimization to prevent iterating over path suffixes that have previously been determined not to be part of a cycle.

I had no trouble implementing this in Python by simply creating a (mutable) set object and sharing it between all branches of the recursion.

My attempt to reimplement this algorithm in Haskell turned out to be much worse (and still isn't as efficient as the original).

I would like some pointers about how to restructure this so that it isn't a mess of recursive folds, but without giving up the set of visited, nodes, which lets me avoid taking branches that have already been explored.

import Data.Maybe
import qualified Data.IntMap.Strict as M
import qualified Data.Char as C

import Debug.Trace

edges :: String
edges =
  "a b\n\
  \a c\n\
  \b c\n\
  \b d\n\
  \c d\n\
  \c e\n\
  \e f\n\
  \e g\n\
  \f g\n\
  \g c"

type Node = Int

parseGraph :: String -> M.IntMap [Node]
parseGraph = foldr go M.empty . lines
  where go line m = let [key, rule] = map ruleToKey (words line)
                    in M.insertWith inserter key [rule] m
        inserter [rule] olds = rule:olds

ruleToKey :: String -> Node
ruleToKey rule = C.ord (head rule) - 97

keyToRule :: Node -> String
keyToRule key = return $ C.chr (key + 97)

hasCycle :: M.IntMap [Node] -> Maybe [Node]
hasCycle m = reverse <$> ret
  where dummyM = M.insert phantom (reverse $ M.keys m) m
        phantom = -1
        (_, _, ret) = hasCycleHelper dummyM phantom ([], [], Nothing)

hasCycleHelper :: M.IntMap [Node] -> Node -> ([Node], [Node], Maybe [Node]) -> ([Node], [Node], Maybe [Node])
hasCycleHelper rules rule (visited', visiting', cyc) =
  trace rendered $
    case () of
      _ | isJust cyc || rule `elem` visited' -> (visited', visiting', cyc)
        | rule `elem` visiting' -> ([], [], Just (takeWhile (/= rule) visiting' ++ [rule]))
        | otherwise ->  returned
  where
    children = M.findWithDefault [] rule rules
    (visited, _, ret) = foldr (hasCycleHelper rules) acc children
    returned = (rule:visited, visiting', ret)
    acc = (visited', rule:visiting', Nothing)
    rendered =    "Current '" ++ keyToRule rule ++ "', "
               ++ "Visiting '" ++ map (head . keyToRule) visiting' ++ "', "
               ++ "Visited '" ++ map (head . keyToRule) visited' ++ "', "
               ++ "Found " ++ show (map (head . keyToRule) <$> cyc)

main :: IO ()
main = do
  let rules = parseGraph edges
      cyc = map keyToRule <$> hasCycle rules
  print cyc

With output:

Current '`', Visiting '', Visited '', Found Nothing
Current 'a', Visiting '`', Visited '', Found Nothing
Current 'c', Visiting 'a`', Visited '', Found Nothing
Current 'e', Visiting 'ca`', Visited '', Found Nothing
Current 'g', Visiting 'eca`', Visited '', Found Nothing
Current 'c', Visiting 'geca`', Visited '', Found Nothing
Current 'f', Visiting 'eca`', Visited 'g', Found Just "gec"
Current 'd', Visiting 'ca`', Visited 'eg', Found Just "gec"
Current 'b', Visiting 'a`', Visited 'ceg', Found Just "gec"
Current 'b', Visiting '`', Visited 'aceg', Found Just "gec"
Current 'c', Visiting '`', Visited 'aceg', Found Just "gec"
Current 'e', Visiting '`', Visited 'aceg', Found Just "gec"
Current 'f', Visiting '`', Visited 'aceg', Found Just "gec"
Current 'g', Visiting '`', Visited 'aceg', Found Just "gec"
Just ["c","e","g"]

Because I'm using foldr, it can't abort the search after discovering a cycle. It has to iterate over all the nodes in the graph at the top level, carrying along the result. I think that's pretty miserable, but I thought I'd ask about it before rewriting everything.

An easy way to detect a cycle is through the implementation of a disjoint set structure, sometimes called a Union-Find structure.

You start with a source node and attempt to add a node to that set.  If your   Find() call for the source node and the other node returns the same root node, then a cycle world result if the nodes are unioned.

UPDATE:

You could implement a topological sort, arranging the nodes with edges going from left to right. A topological sort can only be completed successfully if and only if the graph is a Directed Acyclic Graph. So, if the algorithm detects any cycles, it will stop.

The runtime is O(|V| + |E|), which is better than DFS, in the case that repetition occurs during traversal.

Topological Sorting


Two keyboard handlers for a video game character The code below is equivalent. I can see pros and cons for both versions. Which is better: the short, clever way, or the long, ctrl+c way?

Short version:

character.on("key",function(key){
    var action = ({
            "a":{axis:"x",direction:-1},
            "d":{axis:"x",direction:1},
            "w":{axis:"y",direction:1},
            "s":{axis:"y",direction:-1}})[key[1]],
        stop = key[0]=="-";
    if (action)
        if (stop)
            this.walkdir[action.axis] = 0;
        else
            this.walkdir[action.axis] = this.lookdir[action.axis] = action.direction;
});

Long version:

character.on("key",function(key){
    switch (key){
        case "+a":
            this.walkdir.x = -1;
            this.lookdir.x = -1;
        break;
        case "+d":
            this.walkdir.x = 1;
            this.lookdir.x = 1;
        break;
        case "+w":
            this.walkdir.y = 1;
            this.lookdir.y = 1;
        break;
        case "+s":
            this.walkdir.y = -1;
            this.lookdir.y = -1;
        break;
        case "-a":
            if (this.walkdir.x == -1)
                this.walkdir.x = 0;
        break;
        case "-d":
            if (this.walkdir.x == 1)
                this.walkdir.x = 0;
        break;
        case "-w":
            if (this.walkdir.y == 1)
                this.walkdir.y = 0;
        break;
        case "-s":
            if (this.walkdir.y == -1)
                this.walkdir.y = 0;
        break;
        case "space":
            this.setStance("jumping");
        break;
    };
});

The short code wins hands down. I understood it immediately, and more importantly, I can trivially verify that the code is reasonably error free. This is much harder with the longer code.

You say that the longer code is easier to understand but I claim that this is objectively wrong.

Case in point, the long code uses lots of magic numbers: 1, 0, -1, … what do these stand for? Ah, the short code tells us: they are directions.

The longer code also makes us scroll (depending on the screen size) to see the whole method. This significantly impacts ease of understanding. I believe there were even studies demonstrating this empirically (but I cannot cite them; Code Complete would probably be the relevant reference here).

The one thing I would change in the short code is the lookup itself: define the dictionary separately, maybe even outside the method, and perform the lookup as follows:

var action = movement_commands[key[1]];

And maybe think about tokenising key properly, i.e. assigning the parts to variables before using them. However, I think that the method is short enough to make this unnecessary.

You also said that the longer code is more efficient but I’d like to see a benchmark before I believe that. You probably think that the first code is slower because of the dictionary lookup. But consider that JavaScript is a dynamic language – every single variable access is potentially a dictionary lookup internally. So there is no difference in performance – indeed, the short code could be faster since there’s less variable lookup involved.

(Of course the two code snippets do different things: the long version handles jumping, and they behave differently when the character was previously walking in one direction and now you cancel walking into a different direction.)


Initial starting player - v2 This is the updated code using all suggestions from the previous question that I made here.

I have revised the naming of the variables, and i have included the classes that this part of code is using, in order to have a better understanding.

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

  It is like a round based game.

  Every player can have multiple units to control, but can only use one of them each round..
  Every unit has an int CurrentSpeed variable;

  I need to have in 2 variables who attacks first (Player firstPlayer)
  and who attacks second (Player secondPlayer), based on the speed of
  the active Unit.

At the moment I have this code:

public class Player {
    public int Id { get; set; }
    public string Name { get; set; }
    public List<Unit> Party { get; set; }
    public Unit Active { get; set; }
}

public class Unit {
    public string Name;
    public int InitialSpeed { get; set; }
    public int CurrentSpeed { get; set; }
}

public class BattleSystem {

    private void BattleTurn(){
        Player firstPlayer = null; //Player attacking first
        Player secondPlayer = null; //Player attacking second

        //Check the speed of the units, and determine who attack first
        firstPlayer = player_1.Active.CurrentSpeed > player_2.Active.CurrentSpeed ? player_1 : player_1.Active.CurrentSpeed < player_2.Active.CurrentSpeed ? player_2 : null;

        //if firstPlayer is null (active units have the same CurrentSpeed)
        if(firstPlayer == null){
            //Randomize who goes first
            System.Random rnd = new System.Random();
            int rng = rnd.Next(2);
            firstPlayer = rng == 0 ? player_1 : player_2;
            secondPlayer = rng == 0 ? player_2 : player_1;
        }else{
            secondPlayer = firstPlayer.Id == player_1.Id ? player_2 : player_1;
        }
    }
}

The part of determine the firstPlayer is ok, but I don't like the checks and assignments of the secondPlayer, like checking the ids.

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

Do without the ternary expressions: because a ternary expression is very good for initializing one variable, but is much less good for initializing two variables.

// Declared but not initialized (to null), in order to guarantee that
// they are each initialized in the subsequent if statements.
Player firstPlayer;
Player secondPlayer;

if (player_1.Active.CurrentSpeed > player_2.Active.CurrentSpeed)
{
    firstPlayer = player_1;
    secondPlayer = player_2;
}
else if (player_1.Active.CurrentSpeed < player_2.Active.CurrentSpeed)
{
    firstPlayer = player_2;
    secondPlayer = player_1;
}
else
{
    //Randomize who goes first
    ... etc ...
}


Pick out excel cells from a long horizontal list based on an adjacent numerical value I've already written this macro that does exactly what I want it to do. However, it's quite large, and I may wish to expand on it in future. Basically, the purpose is to copy and paste items from a horizontal list (where RNG is equal to the number of items there), and take one cell from each. Any suggestions in cutting this down? It feels as if there should be a way, since so little changes from line to line.

Sub Test()

Dim Rng As Range
Dim i As Long
i = 3
While i <= 300
    Set Rng = Range("J" & i)
    If Rng = 0 Then
        i = i + 1
    ElseIf Rng = "" Then
        i = i + 1
    ElseIf Rng = 1 Then
        i = i + 1
    ElseIf Rng = 2 Then
        Rng.Offset(, -9).Resize(, 125).Copy
        Rng.Offset(, -9).Insert Shift:=xlDown
        i = i + 2
    ElseIf Rng = 3 Then
        Rng.Offset(, -9).Resize(, 125).Copy
        Rng.Offset(, -9).Insert Shift:=xlDown
        Rng.Offset(, -9).Resize(, 125).Copy
        Rng.Offset(, -9).Insert Shift:=xlDown
        i = i + 3
    ElseIf Rng = 4 Then
        Rng.Offset(, -9).Resize(, 125).Copy
        Rng.Offset(, -9).Insert Shift:=xlDown
        Rng.Offset(, -9).Resize(, 125).Copy
        Rng.Offset(, -9).Insert Shift:=xlDown
        Rng.Offset(, -9).Resize(, 125).Copy
        Rng.Offset(, -9).Insert Shift:=xlDown
        i = i + 4
    Else
        Stop
    End If
Wend

Using a Select will definitely shorten up your code.

While i <= 300
    Set Rng = Range("J" & i)
    Select Case Rng.Value
        Case 0, 1, vbNullString
            i = i + 1
        Case 2
            Rng.Offset(, -9).Resize(, 125).Copy
            Rng.Offset(, -9).Insert Shift:=xlDown
            i = i + 2
        Case 3
            Rng.Offset(, -9).Resize(, 125).Copy
            Rng.Offset(, -9).Insert Shift:=xlDown
            Rng.Offset(, -9).Resize(, 125).Copy
            Rng.Offset(, -9).Insert Shift:=xlDown
            i = i + 3

        ' etc.

    End Select
Wend

The next step would be to extract a method to eliminate all of the duplication. It should take in a Range parameter and perform your copy/insert. Basically anytime you hit Ctrl + C you should stop and write a method instead.


C program to delete each character in s1 that matches any character in s2 Works fine. Not looking for any specific input.

#include <stdio.h>
#include <stdlib.h>
#define MAX_SIZE 10000

void delete_str(char *s1, char *s2){
    for(int i = 0; *(s1+i) != '\0'; ++i){
        for(int j = 0; *(s2+j) != '\0'; ++j){
            if(*(s1+i) == *(s2+j)){
                for(int n = i; *(s1+n) != '\0'; ++n){
                    *(s1+n) = *(s1+n+1);
                }
                --i;
                break;
            }
        }
    }
}

int main(void){
    char s1[MAX_SIZE];
    char s2[MAX_SIZE];

    puts("Enter s1");
    if(fgets(s1, sizeof(s1), stdin) == NULL){
        puts("Error reading s1");
        exit(EXIT_FAILURE);
    }

    puts("Enter s2");
    if(fgets(s2, sizeof(s2), stdin) == NULL){
        puts("Error reading s2");
        exit(EXIT_FAILURE);
    }

    delete_str(s1, s2);

    printf("%s", s1);

    return 0;
}

This compiles cleanly with no warnings, so a positive there.

You may find it easier to use command-line input rather than fgets() for the following reasons:

You don't need to decide on a maximum input size in advance
You can have any character (including newline) in your strings

On the flip side, you do have to be able to write the strings as single arguments; you'll have to be comfortable with your shell's quoting and character escapes.

I would re-write the main() like this:

int main(int argc, char **argv)
{
    if (argc != 3) {
        fprintf(stderr, "Usage: %s string chars-to-delete\n", argv[0]);
        return EXIT_FAILURE;
    }

    delete_str(argv[1], argv[2]);
    puts(argv[1]);
}

Two other things I changed:

Error message should go to stderr.  Your use of EXIT_FAILURE is perfectly right, and I've kept that (though I find plain return simpler than exit(), and in C++ it may be better to ensure destructors get called).
We don't necessarily have a terminating newline in the string (as we did with fgets()), so we can use puts() instead of printf("%s", s1) - but well done for avoiding the trap of printf(s1) which would be wrong and dangerous.

Turning now to delete_str itself, you might want to change s2 from char* to const char*, as we're not modifying the second argument.  That helps clarify which argument is which, too.

The first thing I note is that the array indexing *(s1+i) is usually written s1[i], and doing so will make your intent clearer to other C programmers:

void delete_str(char *s1, const char *s2)
{
    for (int i = 0;  s1[i] != '\0';  ++i) {
        for (int j = 0;  s2[j] != '\0';  ++j) {
            if (s1[i] == s2[j]) {
                for (int n = i;  s1[n] != '\0';  ++n) {
                    s1[n] = s1[n+1];
                }
                --i;
                break;
            }
        }
    }
}

One thing that will slow your program down is that each time you delete any of the matched characters, you copy the whole of the rest of s1 to close up the gap.  Instead of doing that, you can keep track of two positions: the next character to be read and the next character to be written.  These will start off at the beginning of s1, but each time a character is skipped, the read position will advance, and the write will not.  I'll call these src and dest, and I'll make them pointers (rather than indexes into s1).  We get to replace the inner (n) loop:

void delete_str(char *s1, const char *s2)
{
    char *dest = s1;
    for (const char *src = s1;  *src != '\0';  ++src) {
        *dest++ = *src;
        for (int j = 0;  s2[j] != '\0';  ++j) {
            if (*src == s2[j]) {
                --dest;
                break;
            }
        }
    }
    *dest = '\0';
}

We can also lean on the standard library.  There's strchr(const char *s, int c), which will return a pointer into s if it contains c, or a null pointer if not.  So we can test if *src is in s2 with a simple call (replacing the j loop):

#include <string.h>

void delete_str(char *s1, const char *s2)
{
    char *dest = s1;
    for (const char *src = s1;  *src != '\0';  ++src) {
        if (strchr(s2, *src) == NULL)
            *dest++ = *src;
    }
    *dest = '\0';
}

We can do better: the standard library also contains strspn and strcspn which tell us how many characters to skip and how many to copy.  We can use that to copy whole chunks of input, like this:

void delete_str(char *s1, const char *s2)
{
    char *dest = s1;
    for (const char *src = s1;  *src;  src += strspn(src, s2)) {
        int n = strcspn(src, s2);
        memmove(dest, src, n);
        dest += n;
        src += n;
    }
    *dest = '\0';
}

I note also that we never use s1 again, so we could just use that in place of dest.

My version

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

void delete_str(char *s1, const char *s2)
{
    for (const char *src = s1;  *src;  src += strspn(src, s2)) {
        int n = strcspn(src, s2);
        memmove(s1, src, n);
        s1 += n;
        src += n;
    }
    *s1 = '\0';
}

int main(int argc, char **argv)
{
    if (argc != 3) {
        fprintf(stderr, "Usage: %s string chars-to-delete\n", argv[0]);
        return EXIT_FAILURE;
    }

    delete_str(argv[1], argv[2]);
    puts(argv[1]);
}

It is possible (and perhaps faster) to use a table-based approach.  Create an array of booleans (i.e. any integer type, but probably uint_fast8_t from <stdint.h> for speed, or char if size is most important), and toggle each position that's mentioned in s2 (remember to convert to unsigned, as plain char may or may not be a signed type).  You might want to benchmark to see which is faster, and for which inputs:

#include <limits.h>
#include <stdint.h>
#include <string.h>

void delete_str(char *s1, const char *s2)
{
    typedef uint_fast8_t flag_type;
    flag_type allowed[1 << CHAR_BIT];
    memset(allowed, 1, sizeof allowed);
    while (*s2)
        allowed[(unsigned char)*s2++] = 0;

    for (const char *src = s1;  *src;  ++src) {
        if (allowed[(unsigned char)*src])
            *s1++ = *src;
    }
    *s1 = '\0';
}


MySQL Connection URI Regular Expression I'm currently working on a project that involves a MySQL database. To make life a little easier, I've been working on a regex that should validate and pull apart the different sections of the connection URI. All of my testing shows that it does work correctly, but I'd like to challenge everyone to see if they can find any errors in the expression.

^(mysqlx?:\/\/)(?:([\w$_]*)(?::([\w$_]+))?@)?([\w!#$%&'()*+,\-./;=?@[\]_~]*)(?::(\d{1,5}))?(?:\/[\w$_])?$

Please note that this regular expression is using the JavaScript regex engine. It was developed in accordance with the MySQL documentation.

Just using your pattern on regex101, against the given examples on the documentation link you provided, the pattern fails for a lot of scenarios.

A few points to consider:

Pattern for valid IPv6 targets
Allow for percent encoding
\- You don't have to escape the hyphen if you move the - (hyphen) at the beginning or the end of matchset so that it does not act as rangeset.
Does not support an attribute in the URI in its current form

  PS: You can test the pattern against the given mysql examples for
  initial validations.


Correcting punctuation spacing in a string I'm working on a program that corrects all punctuation spacing in a String.

Examples:

// "Hello ,World"  ->  "Hello, World"
// "9 :00 A .M ."  ->  "9:00 A.M."
// "can 't even."  ->  "can't even."

Here's how it works:

My process is to first add a space between all punctuation and then remove any unnecessary spaces. This allows me to tokenize both punctuation and words/numbers correctly.

For example, the acronym String s = " A .M ." is transformed into "A . M .".

When split by a space, it returns the list ["A", ".", "M", "."].

I then attempt to "fix" the remaining line spacing in the String, so the result will become "A.M.".

Here's an example of what the code looks like for correcting acronyms:

In FixAcronym.java:

/**
 * Fix acronyms.
 */
private static String fix(String line, SubtitleObject so) {
    StringBuilder builder = new StringBuilder();
    String[] split = so.split(RegexEnum.SPACE, line); // same as line.split(" ") but cached
    String prevPrevPrev = null, prevPrev = null, prev = null, current = null;
    boolean addSpace;
    for (int i = 0; i < split.length; ++i) {
        prevPrevPrev = prevPrev;
        prevPrev = prev;
        prev = current;
        current = split[i];
        addSpace = true;
        if (prevPrevPrev != null && prevPrevPrev.length() == 1
            && Character.isLetter(prevPrevPrev.charAt(0))) {
            if (prevPrev.equals(".")) {
                if (prev.length() == 1 && Character.isLetter(prev.charAt(0))) {
                    if (current.equals(".")) {
                        StringBuilderUtil.deleteSpaceAt(builder, builder.length() - 2);
                        StringBuilderUtil.deleteOnlyIfSpaceAt(builder, builder.length() - 3);
                        addSpace = false;
                    }
                }
            }
        }

        if (i > 0 && addSpace) {
            builder.append(' ');
        }
        builder.append(current);
    }

    return builder.toString();
}

In StringBuilderUtil.java:

/**
 * Delete space at index within StringBuilder.
 */
public static void deleteSpaceAt(StringBuilder builder, int index) {
    assert builder.charAt(index) == ' ';
    builder.deleteCharAt(index);
}

/**
 * Delete only if space at index within StringBuilder.
 */
public static void deleteOnlyIfSpaceAt(StringBuilder builder, int index) {
    if (builder.charAt(index) == ' ') {
        builder.deleteCharAt(index);
    }
}

Here's the problem: Because of all the different grammatical rules, there are a lot of different "fix" methods, and each subsequent "fix" uses the same technique.

line = FixTime.fix(line, this);
line = FixContractions.fix(line, this);
line = FixAcronym.fix(line, this);
// etc...

Each fix splits the line by a space, iterates through the tokens, deletes the space(s) in the StringBuilder when necessary, and appends the space and/or token.

The problem with this process is hypothetically I only need to iterate through the split line once to get the corrected String. This also makes the continuous creations, deletions, and appendings of the StringBuilder extraneous.

So the question is: Is there a better way to generalize this process while still preserving modularity, or is this as good as it gets?

I would split it into three stages, and two concepts.

Conceptually, split the problem into:

String manipulation (reading and writing)
Logical problems (what exists and what needs doing)

The stages the process might be split into:

The first stage involves reading the string, parsing it from beginning to end, recording the logical aspects of what exists.
The second stage involves manipulating the logical record that represents the tokens, transforming it from a representation of what exists, to a representation of what string manipulations are required.
The third stage is executing the string manipulation.


Prime Number Generator (6n + 1 or 6n - 1) This generator is like most where it brute forces an integer: it see whether the integer is divisible by any of the primes; if so, then it's not a prime and vice versa. This though only compares values 6n + 1 and 6n - 1 starting with n as 1 as all primes larger than 3 can be written as 6n + 1 or 6n - 1 where n is a positive integer.

Examples: 7 is 6(1) + 1 (n = 1) and 17 is 6(3) - 1 (n = 3).

Code:

primes = [2, 3]
limit = 0
n = 1
is_prime = True

while True:
    try:
        limit = int(raw_input("Enter a positive integer larger than 3: "))
    except ValueError:
        print "Please enter a positive integer larger than 3!"
    else:
        if limit <= 3:
            print "Please enter a positive integer larger than 3!"
        else:
            break

while True:
    minus_prime = 6*n - 1
    plus_prime = 6*n + 1

    if minus_prime < limit:
        for x in primes:
            if minus_prime % x == 0:
                is_prime = False
                break
        if is_prime:
            primes.append(minus_prime)
        is_prime = True
    elif minus_prime > limit:
        print primes
        break

    if plus_prime < limit:
        for x in primes:
            if plus_prime % x == 0:
                is_prime = False
                break
        if is_prime:
            primes.append(plus_prime)
        is_prime = True
    elif plus_prime > limit:
        print primes
        break

    n += 1

Basically, given preset primes 2 and 3, find all primes that are less than limit by producing numbers with 6*n + 1 and 6*n - 1 (n increases by one every loop) and checking first if they are over the limit. If so, stop the loop and print out the primes (as a list). If not, brute force through primes to see if the number is prime. If so, add to primes. Else, continue. limit is checked in the first while loop to ensure validity.

I am wondering how to improve the performance of my program, improve its structure, and make it more Pythonic. Try it here

How about:

import math

limit = 100000
primes = [2,3]

def calc_potential_primes(n):
    return [6*n-1, 6*n+1]

def is_prime(x):
    for p in primes:
        if p <= math.sqrt(x):
            if x % p == 0:
                return False
        else:
            break
    return True

n = 1
while True:
    possible_primes = calc_potential_primes(n)
    new_primes = [x for x in possible_primes if is_prime(x) and x < limit]
    primes += new_primes

    if sum(i > limit for i in possible_primes) > 0:
        break
    n += 1

print primes

We don't have to check every prime #, only primes up to sqrt(x).

You run the same code block on 2 different inputs, which can lead to potential bugs where you forget to update variable names as pointed out by Aemyl. If you're going to re-use the same code, it's better to put it into a function to keep the code readable and prevent potential bugs.

Maintaining our code logic inside a function also prevents us from having to update a global variable, which can also lead to additional bugs if at any point we forget to or accidentally update it.

Encapsulating your prime calculations within a function that returns a list of primes will also allow us to make updates to how we generate our primes without having to update the rest of our code. By the same logic, if we want to update how we check if a # is prime (maybe we think of a more efficient solution), we only have to update it in one place, instead of twice in the current case. In this case, functions keep our code maintainable and easy to follow.

By always returning a list, if at any point we want to scale or change how we generate prime number candidates, we can simply update the code logic and return the same object as before (a list) and the rest of our code should work as is. In the current case, we would have to set additional variables, then repeat the prime check code block for each additional variable.

Combining lists with Python's list comprehensions allow us to clearly and easily filter for the items (prime #s) that we want, while maintaining readability and efficiency.

Basically, consider using functions and list comprehensions to keep code readable, simple, maintainable and therefore hopefully as bug-free as possible.

With this I get a timeit of 208 ms ± 682 µs per loop (mean ± std. dev. of 7 runs, 1 loop each) when calculating primes up to 100000

versus 4.28 s ± 52 ms per loop (mean ± std. dev. of 7 runs, 1 loop each) with yours.


pipe 2 linux commands in C I am writing a simpel program that can basicly do the same as piping two linux commands can do. For example, ls -l | grep vars.sh, this can be done on the Linux console. I need to write a programm in C that can do the same. I think I got it working but I want to know if I am doing it rite and if there are some modifications I can do on my code.

Here is my code:

#include <stdio.h> // Voor verschillende I/O's, macro's, declaraties.
#include <stdlib.h> // Om de system call te activeren.
#include <unistd.h> // Laden van constanten en types.

int main()
{
    pid_t pid;
    int pipefd[2];
    pipe(pipefd);
    pid = fork();

    if(pid==-1){
        perror("pipe");
        exit(EXIT_FAILURE);
    }

    if(pid>0){
        close(pipefd[0]);
        dup2(pipefd[1],1);
        execlp("ls","ls","-l",NULL);
    }
    else{
        close(pipefd[1]);
        dup2(pipefd[0],0);
        execlp("grep","grep",".bashrc",NULL);
        exit(EXIT_FAILURE);
    }
}

Any feedback,rewrites or modifications are welcome.

The only other way that I could see this can be done in POSIX would be to use popen() rather than using raw pipes.  It also returns a stream pointer that can be used with the C standard I/O functions like fprintf(), fgets(), etc. to read and/or write to the process on the other-side of the pipe created by popen().  According to the POSIX specification, with popen(),

  The environment of the executed
  command shall be as if a child process
  were created within the popen() call
  using the fork() function, and the
  child invoked the sh utility using the
  call:

  execl(shell path, "sh", "-c", command,
  (char *)0);

  where shell path is an unspecified
  pathname for the sh utility.

So it's basically an easier, more straight-forward way to work with pipes rather than having to fork child processes yourself and close the un-used portions of the pipe so that you don't end up with accidental blocking reads or lost writes because there are still active file-descriptors on each end of the pipe that you've accidentally forgotten to close in either the parent or child process.

With your example, using popen() would look like the following:

#include <stdio.h>
#define BUFFERSIZE 128

FILE *ls_proc, *grep_proc;
ls_proc = popen("ls -l", "r");
grep_proc = popen("grep .bashrc", "w");

char buffer[BUFFERSIZE];

//read from one child process and write to the other
while (fgets(buffer, BUFFERSIZE, ls_proc) != NULL)
    fprintf(grep_proc, "%s", buffer);

pclose(ls_proc);
pclose(grep_proc);


Replacing an application service class with multiple command handlers I have a Controller which uses an application service to accomplish its tasks. The service class is starting to grow big and developing multiple dependencies. So I am thinking of replacing the single big service class with multiple command handlers, each handling a single responsibility. This is how the old and the new code will look like (simplified):

Old Code

@Component
@Path("/app/jobs")
public class JobController {

    @GET
    @Produces(MediaType.APPLICATION_JSON)
    public List<JobDto> getJobs() throws JobException {
         return jobService.getJobs();
    }

    @PUT
    @Path("{job_id}/assigned_to/{user_id}")
    public Response assignTo(
            @PathParam("job_id") @NotNull String jobId,
            @PathParam("user_id") @NotNull String userId
            ) throws JobException {

        jobService.assignJobTo(jobId, userId);
        return Response.status(Response.Status.CREATED).build();
    }
    ...
}

New Code

@Path("/app/jobs")
public class JobController {

    @GET
    @Produces(MediaType.APPLICATION_JSON)
    public List<JobDto> getJobs() throws JobException {
         return new GetJobListCommandHandler(
                new GetJobListCommand()
        ).handle();
    }

    @PUT
    @Path("{job_id}/assigned_to/{user_id}")
    public Response assignTo(
            @PathParam("job_id") @NotNull String jobId,
            @PathParam("user_id") @NotNull String userId
            ) throws JobException {

        new AssignJobCommandHandler(
                new AssignJobCommand(jobId, userId)
        ).handle();
        return Response.status(Response.Status.CREATED).build();
    }
    ...
}

I am basically following this tutorial in using command handlers. This patterns isolates each responsibility into one class which is great. But now, since my controller methods create the command handler objects, the handlers have become difficult to mock in unit tests. How do I write unit tests for the controller? Earlier the controller class had just a mocked version of the service object. One option could be to break the controller to service one uri (GetJobsController, AssignJobController, etc). But I don't want to do that. I like seeing the hierarchy of resources together and the path annotations work well like this. Is this the right way of going about this? What are the best practices or this approach and how do I write my unit test?

My application is a Spring+Jersey application.

What's wrong

First of all read the tutorial again, you missed his whole point.

You cannot use decorators with your current design. Why?

new AssignJobCommandHandler(
        new AssignJobCommand(jobId, userId)
).handle();

You use new therefore you cannot use a decorated version. You have a AssignJobCommandHandler and it is not a TransactionCommandHandlerDecorator.
You pass the command to the constructor. Constructors are not polymorphic. You cannot customize how a command is handle if you pass it to a constructor, (or equivalently a static method)
You don't pass the dependencies in the CommandHandlers, so you would have to use the Service Locator, Evil Singleton anti-patterns. I'm guessing it doesn't currently work at all.

Getters are not command handlers. A request is either a query (GET, etc) or a command (POST, etc), never both. Make sure your service methods follow CQS. Don't do this:

return new GetJobListCommandHandler(
                new GetJobListCommand()
        ).handle();

What to do

Depend on abstractions.

Create said the interface:

public interface CommandHandler<TCommand> {
    void handle(TCommand command);
}

and use it like this.

@Path("/app/jobs")
public class AssignJobController {

    @Autowired
    CommandHandler<AssignJobCommand> handler;

    @PUT
    @Path("{job_id}/assigned_to/{user_id}")
    public Response assignTo(
            @PathParam("job_id") @NotNull String jobId,
            @PathParam("user_id") @NotNull String userId
            ) throws JobException {

        handler.handle(new AssignJobCommand(jobId, userId));

        return Response.status(Response.Status.CREATED).build();
    }
}

You can use decorators like this, for example:

@Configuration
public class Config {
    @Autowired
    JobDAO jobDAO;

    @Bean
    public CommandHandler<AssignJobCommand> assignJobCommandHandler() {
        // pass in the dependencies as shown in the tutorial
        // I use a DAO instead of a UnitOfWork, as they are more common with Spring applications.
        CommandHandler<AssignJobCommand> handler = new AssignJobCommandHandler(jobDAO);

        // you can use multiple decorators
        // this kind of decorators do not need to use reflection, proxies, bytecode manipulation etc magic. They are plain-old java objects.
        handler = new TransactionCommandHandlerDecorator(handler);
        handler = new DeadlockRetryCommandHandlerDecorator(handler);
        ...
        ...
        return handler;
    }
}

Testing

Now that we pass the dependencies in the handlers, we can test them.

One way to do it is using a mocking framework, there are others.

Test may look like this, for example:

@RunWith(MockitoJUnitRunner.class)
public class AssignJobCommandHandlerTest {

    @Mock
    JobDAO jobDAO;

    AssignJobCommandHandler handler;

    @Before
    public void setUp() {
        handler = AssignJobCommandHandler(jobDAO);
    }

    @Test
    public void assignsAJobToAUser() {
        long jobId = 123L;
        long userId = 456L;

        Job job = new Job();

        when(jobDAO.getJob(jobId)).thenReturn(job);

        handler.handle(new AssignJobCommand(jobId, userId));

        assertThat(job.getAssignedUserId(), is(userId));
    }
}


Avoiding repetitive code for timestamping and updating status This is some boilerplate I found myself writing on a new abstract class. It seems ripe for refactoring, but I'm not sure the best way to implement it more effectively. Is there a pattern for these sort of time-stamping/status-updating methods?

def create() {
    tsCreated = new DateTime()
    userCreated = springSecurityService.currentUser
    status = RequestStatus.OPEN
}

def accept() {
    tsAccepted = new DateTime()
    userAccepted = springSecurityService.currentUser
    status = RequestStatus.ACCEPTED
}

def send() {
    tsSent = new DateTime()
    userSent = springSecurityService.currentUser
    status = RequestStatus.SENT
}

def complete() {
    tsCompleted = new DateTime()
    userCompleted = springSecurityService.currentUser
    status = RequestStatus.COMPLETED
}

def cancel() {
    tsCanceled = new DateTime()
    userCanceled = springSecurityService.currentUser
    status = RequestStatus.CANCELED
}

No idea about groovy, but it looks like a case for an EnumMap<RequestStatus, Data> with the map storing the (last) corresponding time and user. You need a single method

def action(RequestStatus status) {
    map.put(status, new Data(new DateTime(), springSecurityService.currentUser))
    this.status = status
}

If you really need 5 methods, you can define them trivially via action.


Multitenant Cassandra Query in Clojure The following code handles querying for user data in a multitenant Cassandra database.  Each tenant has their own keyspace and within that keyspace data about each tenants users is stored.

This code will run as part of a webservice.

I'm looking on comments for how I handled managing the session so that connection pooling is used and only one session is made.

(defn- session []
  (cc/connect ["localhost"]
              {:credentials
               {:username ""
                :password ""}}))

(def memo-session (memoize session))

(defn get-recs-by-user-id [client-name client-id user-id]
    (cql/use-keyspace (memo-session) client-name)
    (cql/select (memo-session) "user_recommendations"
                (where [[= :client_id client-id] [= :user_id user-id]])
                (limit 1)))

This doesn't feel idiomatic to me.

I would prefer to use an atom or a dynamic variable if you must store the session outside, but really you should probably be passing the session in on each call.


Accessing deep properties and methods of an object with using a path I have a code snippet that I have been using and works. It is used a lot in my application and I need to speed it up. Is there a faster better way of deep accessing objects using a string path. It needs to able to access properties and methods, see my examples below:

     function get(obj, path) {
        var paths = path.split('.'),
        curProp = obj;
        for(var i=0;i<paths.length;i++){
            if (!curProp[paths[i]]) return
            curProp = (typeof curProp[paths[i]] !== "function") ? curProp[paths[i]] : curProp[paths[i]]() ;
        }
        return curProp;
    }

The snippet gets the property via the path for the object passed

var obj = {contact:{name:"john"}};
console.log(get(obj, "contact.name"));

If the object has a method that returns an object it can return that too

var obj = {contact:function(){return {name:"john"}}};
console.log(get(obj, "contact.name"));

If we're talking functional-programming, I'd use .forEach() instead of a for loop. It abstracts the iteration process.

if (!curProp[paths[i]]) return does not seem quite right to me, because it will work funny with falsy values. It's better to rely on object.hasOwnProperty(), or Object.keys() depending on the way you want it to work.

Example:

var obj = {};
obj["property1"] = 0;
obj["property2"] = "";

obj["property1"] ? obj["property1"] : "No property1 is available";
  // --> "No property1 is available", isn't right from the humans perspective noramally

obj["property2"] ? obj["property2"] : "No property2 is available";
  // --> "No property2 is available"

I think, it's always good to spell out the return value. In other words, return undefined is better than return in this function.

Slightly different variant:

function get(targetObject, targetPropertyPath) {
    const pathPartList = targetPropertyPath.split('.');
    let currentProperty = targetObject;

    pathPartList.forEach(pathPart => {
        if (!currentProperty.hasOwnProperty(pathPart)) return undefined;

        const property = currentProperty[pathPart];
        currentProperty = (typeof property !== 'function') ? property : property();
    });

    return currentProperty;
}


Instantiating if null I often have static classes which uses the DataContext somehow, and they all make a new DataContext, but often I already have one instantiated elsewhere, so why not use that?

public static bool SignIn(string email, string password, DataContext db = null)
{

    bool disposeDb = false;
    if (db == null)
    {
        disposeDb = true;
        db = new DataContext();
    }

    //Sign In stuff, or any other stuff...

    if (disposeDb) db.Dispose();
}

But is there a cool trick to do this, or am I just coding it wrong?  Or is it ok?

Update

The DataContext is not expensive to instantiate.  I guess I just leave it be.

But here is another example:

public static User GetUser(DataContext db = null)
{
    if (HttpContext.Current.Request.Cookies["Id"] == null)
    {
        return null;
    }
    else
    {
        bool disposeDb = false;
        if (db == null)
        {
            disposeDb = true;
            db = new DataContext();
        }

        int id = Convert.ToInt32(HttpContext.Current.Request.Cookies["Id"].Value);
        string cookieKey = HttpContext.Current.Request.Cookies["CookieKey"].Value;

        User user = db.Users.FirstOrDefault(x => x.Id == id && x.CookieKey == cookieKey);

        if (disposeDb) db.Dispose();
        return user;
    }
}

This is again nice because if I already have a context, I can provide it, and then I won't have to attach the entity (User) to another context if I want to modify it:

User user = MyClass.GetUser(db);
user.Email = "asdsasd";
db.SaveChanges();

How can I improve it?  Or is it okay?

Update 2

The DataContext should only be instantiated once per page request. And here is a neat way to do it, that actually allows me to access it even from static methods.

At the very least, you should place the second if statement in a finally block.  As things stand, the code is not generally correct from a purely mechanical perspective.  I would also phrase it as:

bool dbDispose = db == null;
db = db ?? new DataContext();

From a design point of view: yes, I'd say this violates the single responsibility principle.  You have a method that not only does certain calculations, but also creates a DataContext if necessary.  If creation has to be modified at any point, then you'll have many places to change that.  (Unlikely if it's default-constructed like this, but I doubt it's that simple in practice.)

It looks to me like SignIn should actually be a member function of DataContext.


What is the best way to conditionally add to an immutable collection in Scala? I find myself frequently trying to build up Seq's based on some conditionals, but the code I end up with feels verbose and clunky. An example:

def generateSeq(foo: Option[String]) = {
  Map("bar" -> "baz") ++ ( if (foo.isDefined) {
      Map("foo" -> foo.get)
    } else {
      Nil
    } )
}

If it were Python, I would simply do:

def generateObject(foo):
    obj = {}
    obj["bar"] = "baz"
    if foo: obj["foo"] = foo
    return obj

Is there a cleaner way to write the Scala code (especially as I add more logic and conditionals involved in building the map)?

You can use the fact that Map() ++ None == Map() along with Map() ++ Some((k,v)) == Map((k,v)) and Option.map to get the following pattern:

def generateObject(foo: Option[String]) = {
  Map("bar" -> "baz") ++ foo.map("foo" -> _)
}


Converting string to dict using Python ast library I have been trying to convert a string of dictionary objects as given below

"{'Cmd': None, 'Hostname': None, 'Entrypoint': None, 'Env': None,     'OpenStdin': False,
'Tty': False, 'Domainname': None, 'Image': u'nginx:latest', 'HostConfig':
{'Binds': None, 'RestartPolicy': {}, 'NetworkMode': u'bridge', 'Dns': None,
'Links': [], 'PortBindings': {}, 'DnsSearch': None, 'Privileged': False,
'VolumesFrom': []}, 'ExposedPorts': {}, 'User': None}"

to python dictionary.

But the solutions came up while searching were suggesting to use python ast library for the purpose. as

import ast
ast.literal_eval(string)

And it works fine. But is ast library is built indented to solve problems like this ? Is there any good approach other than this to solve the problem ?

To answer your question, yes ast is ok to be used for this. Per the documentation for literal_eval:

Safely evaluate an expression node or a Unicode or Latin-1 encoded string containing a Python literal or container display. The string or node provided may only consist of the following Python literal structures: strings, numbers, tuples, lists, dicts, booleans, and None.
This can be used for safely evaluating strings containing Python values from untrusted sources without the need to parse the values oneself. It is not capable of evaluating arbitrarily complex expressions, for example involving operators or indexing.


Testing a Collatz Conjecture Conjecture (Python) I had an intuition that the maximal trajectory lengths for collatz conjecture numbers would be produced by numbers of the form (2**n)-1 so I wrote a program to test it (I was wrong). I thought I would put it up for review to catch any coding sins I committed, and see how others would improve my code.
Orginal Code
'''
*    I had a conjecture that maximal trajectory lengths for the collatz conjecture were yielded by numbers of the form (2**n)-1. This program attempts to test it.
*    "conjecture()" finds the number with the highest trajectory lengths in a given range. The ranges are of the form [2**n, 2**(n+1)).
'''

import math, pprint

f = open("CollatzConjecture.txt")
text =  f.read()

def key(value, dct):
    for key in dct.keys():
        if dct[key] == value:
            return key
    return None

def conjecture(text):
    traj = {int(j.split(" ")[0]): int(j.split(" ")[1]) for j in text.split("\n")}    #Processes the text file into a dict mapping numbers to their trajectory length.
    result = {}
    bound = math.floor(math.log(len(traj),2))
    traj2 = {num: traj[num] for num in list(traj.keys())[:2**bound+1]}    #Slices the dictionary to receive the section of interest.
    for st in (2**x for x in range(1, bound+1)):        #Generator for powers of 2.
        slce = list(traj2.items())[int((math.log(st,2)-1)**2):st]    #Slices "traj2" into powers of 2.
        result[st] = key(max([n[1] for n in slce]), traj2)
    return pprint.pformat(result)

print(conjecture(text))

"CollatzConjecture.txt" is a text file containing number trajectoryLength on separate lines. I wanted to find the number corresponding to the maximum trajectory length between successive powers of 2 in order to test my hypothesis.

def key(value, dct):
    for key in dct.keys():
        if dct[key] == value:
            return key
    return None

This is really inefficient and not how dictionaries should be used.

The point of a dict is O(1) lookup, this is essentially O(n) and it doesn't make sense
You should remove your blockcomments, and create a nice docstring. That will make the purpose of the code clearer
You don't close the file, a nice addition is using the with statement to automatically open/close files PEP343


Python Readability for SQL query I have this piece of code in Python 2.7:
As you can see is very hard to read the SQL query, what is the best way to re-write it?

    try:
        if title and url:
            sql_query = "INSERT INTO news (title, author, description, " \
                        "content, url, url_to_image, source_id, source, " \
                        "campaign, published_at, score, magnitude, sentiment, " \
                        "inserted_at ) VALUES ('%s','%s','%s','%s'," \
                        "'%s','%s','%s','%s','%s','%s', %s, %s, '%s', '%s')" \
                        % ( title.replace("'", "''"),
                            author.replace("'", "''"),
                            description.replace("'", "''"),
                            content[:settings.content_size].replace("'", "''"),
                            url,
                            url_to_image,
                            source_id,
                            source,
                            campaign,
                            published_at,
                            score,
                            magnitude,
                            sentiment,
                            DB_NOW)
            db = Db.Db()
            db.initialize(dsn=settings.SQLALCHEMY_DSN)
            return db.insert_content(sql_query, 'news_id')
    except psycopg2.ProgrammingError as exception:
        log.exception(exception)`enter code here`

You can use multiline sting """ and string.format

try:
    if title and url:
        sql_query = """
        INSERT INTO news (
        title, author, description, content, url, url_to_image, source_id,
        source, campaign, published_at, score, magnitude, sentiment, inserted_at
         ) VALUES (
         '{title}','{author}','{description}','{content}', '{url}',
         '{url_to_image}','{source_id}','{source}', '{campaign}',
         '{published_at}', {score}, {magnitude}, '{sentiment}', '{DB_NOW}'
         )
        """.format(
            title=title.replace("'", "''"),
            author=author.replace("'", "''"),
            description=description.replace("'", "''"),
            content=content[: settings.content_size].replace("'", "''"),
            url=url,
            url_to_image=url_to_image,
            source_id=source_id,
            source=source,
            campaign=campaign,
            published_at=published_at,
            score=score,
            magnitude=magnitude,
            sentiment=sentiment,
            DB_NOW=DB_NOW,
        )

        db = Db.Db()
        db.initialize(dsn=settings.SQLALCHEMY_DSN)
        return db.insert_content(sql_query, "news_id")
except psycopg2.ProgrammingError as exception:
    log.exception(exception)`enter code here`

If you'd use python 3.6+, you could even simpler use f-strings

string interpolation

As noted by @Ludisposed, string interpolation is not the way to go link
Since it is unclear what the Db.Db is, I assume it is something you wrote yourself. You will have to adapt the insert_content code to accept the arguments, and then do something like this:

try:
    if title and url:
        sql_query = """
        INSERT INTO news (
        title, author, description, content, url, url_to_image, source_id,
        source, campaign, published_at, score, magnitude, sentiment, inserted_at
         ) VALUES (
         %(title)s,%(author)s,%(description)s,%(content)s, %(url)s,
         %(url_to_image)s,%(source_id)s,%(source)s, %(campaign)s,
         %(published_at)s, %(score)s, %(magnitude)s, %(sentiment)s, %(DB_NOW)s
         )
        """
        arguments = dict(
            title=title,
            author=author,
            description=description,
            content=content[: settings.content_size],
            url=url,
            url_to_image=url_to_image,
            source_id=source_id,
            source=source,
            campaign=campaign,
            published_at=published_at,
            score=score,
            magnitude=magnitude,
            sentiment=sentiment,
            DB_NOW=DB_NOW,
        )

        db = Db.Db()
        db.initialize(dsn=settings.SQLALCHEMY_DSN)
        return db.insert_content(sql_query, arguments, "news_id")
except psycopg2.ProgrammingError as exception:
    log.exception(exception)


Checking days from now I'm trying to respect the Ruby principle of keeping my code dry; however, in my conditional, I have different constants set to different days. But that's the only thing different. They're pretty much the same code minus the evaluation of days from_now.

if term_type == TERM_MONTH
  subscription.suspended_at && subscription.suspended_at < MONTHLY_WARNING_1.from_now && suspended_email_1_sent_at.nil? && subscription.suspended_at >= Date.today && is_system == true && paid_at.nil?
else
  subscription.suspended_at && subscription.suspended_at < ANNUAL_WARNING_1.from_now && suspended_email_1_sent_at.nil? && subscription.suspended_at >= Date.today && is_system == true && paid_at.nil?
end

I tried using the ternary operator; however, it didn't produce the results I was expecting as it was only setting the days based on term_type yet not sending the mail based on it.

days = term_type == TERM_MONTH ? MONTHLY_WARNING_1 : ANNUAL_WARNING_1
subscription.suspended_at && subscription.suspended_at < days.from_now && suspended_email_1_sent_at.nil? && subscription.suspended_at >= Date.today && is_system == true && paid_at.nil?

Should I stick with the if block?

Some notes:

More declarative: x.nil? -> !x
More declarative, idiomatic and compact: is_system == true -> is_system.

A first obvious refactor is DRYing the common conditions using a local variable (a temptative name, you'll know what's more fitting):

unpaid_subscription_suspended = subscription.suspended_at &&
    !suspended_email_1_sent_at && subscription.suspended_at >= Date.today &&
    is_system && !paid_at

if term_type == TERM_MONTH
  unpaid_subscription_suspended && subscription.suspended_at < MONTHLY_WARNING_1.from_now
else
  unpaid_subscription_suspended && subscription.suspended_at < ANNUAL_WARNING_1.from_now
end

A second refactor is to refactor the conditional and move all the expressions to the if/elsif predicates:

case
when !(subscription.suspended_at && !suspended_email_1_sent_at &&
       subscription.suspended_at >= Date.today && is_system && !paid_at)
  false
when term_type == TERM_MONTH && subscription.suspended_at < MONTHLY_WARNING_1.from_now
  true
when term_type == TERM_YEAR && subscription.suspended_at < ANNUAL_WARNING_1.from_now
  true
else
  false
end

As an additional advice, you should group those conditions into methods. For example the first chain of conditions may be abstracted as Subscription#unpaid_subscription_suspended?.


Track the existence of process I have an api endpoint that, when called, initiates the generation of a report. This is time consuming process and response could come pretty late. So, we came up with caching architecture and once finished the response is saved to redis.
However, on the frontend side we dicided to make a request once in a while, until it is ready. To face the issue of overlapping queries when one report is initialized several times in a row, I decided to make a temporary file, that exists only when the report is being generated, and the api response is "This report is already being generated". Once the generation is finished, file is deleted and response is already cached.
This is mockup of what I am doing
def post(self):
   serialized_data = self.serializer().deserialize({
            'task': json.loads(request.values.get('task')),
            'media_company': json.loads(request.values.get('mediaCompany'))
    })

   if not os.path.exists('/root/progress_data'):
        os.mkdir('/root/progress_data')

    self.build_range(serialized_data)

    serialized_data['media_company']['date_from'] = \
                                        self.date_from.strftime("%Y-%m-%d")
    serialized_data['media_company']['date_to'] = \
                                        self.date_to.strftime("%Y-%m-%d")
    progress_hash = hashlib.sha256(str(serialized_data).encode()).hexdigest()

    if self.progress_check(progress_hash):
        return json_response({
            "success": False,
            'result': "This report is already being generated",
        })

    file = open(f'/root/progress_data/{progress_hash}', 'w+')
    file.close()
    try:
        report = self.generate_report(serialized_data)
    except:
        os.remove(f'/root/progress_data/{progress_hash}')
        return json_response({
            "success": False,
            'result': "Error while generating report",
        })
    os.remove(f'/root/progress_data/{progress_hash}')
    return json_response({
        "success": True,
        'data': report,
    })

I think that this is not really production-ready solution, but I could not come up with something better.
Also, there are some holes where the file is not being deleted in all cases.
Could you show me potential holes and may be another way of checking the progress

There is nothing I can test here, so I will only make a couple superficial remarks.
You do not offer a lot of insight into your application. If you say you have a problem with overlapping queries, then you should address the root of the problem. I have the impression that you simply devised a workaround. It may do the job but at the expense of performance or reliability. It could also create more problems.
Repetition
What is immediately apparent is that you have a hardcoded path repeated multiple times across your code.
At the top of your code you should define a variable for '/root/progress_data'. Then if you decide to change the file name or path you will have only one line to change.
But Python has a library for temp files: https://docs.python.org/3/library/tempfile.html. This is what you should be using.
The date format "%Y-%m-%d" could be made a variable too.
Security
Does your program really need to run as root ?
Maximize your try/catch/finally blocks
You repeat this twice:
os.remove(f'/root/progress_data/{progress_hash}')

To ensure that the file gets removed at the end of the process you could place this line of code in a finally block.
Same goes for the return value:
return json_response({
    "success": False,
    'result': "Error while generating report",
})

return json_response({
    "success": True,
    'data': report,
})

Keep only one return but use variables for the response depending on the outcome of the execution.
Thus your code will be shorter, more straightforward and easier to comprehend. But I still think the idea is wrong.
I would probably use a database instead of temp files. It can be a small self-contained DB file like SQLite. You need a simple table with details of the job, some kind of unique identifier and a column to update the status of the job. Then your API can query the table and say "ready" when the column has been to updated to the finished status.
This looks clumsy to me. I would ditch the idea and go back to the drawing board. If you are stuck my suggestion would be to post on Stack Overflow for guidance but add more context to your question, explain how the API is used, what your constraints are.


Generic method for HTTP GET/POST parameter conversion I just made a method that should handle GET and POST parameters (primitive types) sent by AJAX requests to the server:

protected T GetParam<T>(string key)
{
    string value = HttpContext.Current.Request[key];

    if (string.IsNullOrEmpty(value))
        return default(T);

    return (T)Convert.ChangeType(value, typeof(T));
}

Do you have any suggestions regarding improvements?

First point, small but important: give your method a proper name. GetParam doesn't mean much and doesn't tell what it returns. Take a name like GetValueFromParameter or GetHttpRequestValue.

Secondly, did you test this method properly? I changed the code a bit to be able to test it, I removed the HttpContext value and just use the parameter as value:

protected T GetHttpRequestValue<T>(string value)
{
    if (string.IsNullOrEmpty(value))
        return default(T);

    return (T)Convert.ChangeType(value, typeof(T));
}

When I call it like this:

var longFromString = GetHttpRequestValue<long?>("0");

I get following exception:

  Invalid cast from 'System.String' to 'System.Nullable`1[[System.Int64, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089]]'.

I went looking for a cause and it seems that Convert.ChangeType has problems with nullable types. You have to use the underlying type of the nullable in order to make it work:

protected T GetHttpRequestValue<T>(string key)
{
    string value = HttpContext.Current.Request[key];

    if (string.IsNullOrEmpty(value))
        return default(T);

    var type = Nullable.GetUnderlyingType(typeof(T)) ?? typeof(T);
    var convertedValue = (value == null) ? null : Convert.ChangeType(value, type);
    return (T)convertedValue ;
}

Now I get following results:

GetHttpRequestValue<long?>("0");    // 0
GetHttpRequestValue<long?>("");     // null
GetHttpRequestValue<long>("1");     // 1
GetHttpRequestValue<bool>("true");  // True
GetHttpRequestValue<bool>("");      // False

GetHttpRequestValue<long?>(null);   //null
GetHttpRequestValue<bool>(null);    //False

You also made your method dependent of an HttpContext, you cannot reuse it. Better would be to rename it to something like ConvertTo and leave out the HttpContext-line:

protected T ConvertTo<T>(string value)
{
    if (string.IsNullOrEmpty(value))
        return default(T);

    var type = Nullable.GetUnderlyingType(typeof(T)) ?? typeof(T);
    var convertedValue = (value == null) ? null : Convert.ChangeType(value, type);
    return (T)convertedValue;
}

Example usage:

protected void Page_Load(object sender,EventArgs e)
{
    var value = HttpContext.Current.Request["YourKey"];
    var converted = ConvertTo<long?>(value);
}

Your code is still clean and the method is reusable. Notice that I use var instead of explicitly declaring variables. This is not mandatory, I just prefer to use it. Hope this helps!

Update:

I updated the method to handle a fail on the conversion, in case you want to change qwerty to a bool for example. :)

protected T ConvertTo<T>(string value)
{
    if (String.IsNullOrEmpty(value))
        return default(T);

    var type = Nullable.GetUnderlyingType(typeof(T)) ?? typeof(T);
    T convertedValue = default(T);

    try
    {
        convertedValue = (T)Convert.ChangeType(value, type);
    }
    catch {}

    return convertedValue ;
}

This way of working swallows the exception. In case one occurs the default of T will be returned, otherwise the converted value. Results:

ConvertTo<int>("qwerty");    //0
ConvertTo<bool>("qwerty");   //False

Update:

As stated in the comments below, you can also take the code and throw it in an extension method. Also nice for reusability and your could will also look a bit cleaner. Here goes:

public static class Extensions
{
    public static T ConvertTo<T>(this string value)
    {
        if (String.IsNullOrEmpty(value))
            return default(T);

        var type = Nullable.GetUnderlyingType(typeof(T)) ?? typeof(T);
        T convertedValue = default(T);

        try
        {
            convertedValue = (T)Convert.ChangeType(value, type);
        }
        catch {}

        return convertedValue ;
    }
}

Usage:

var intFromString = "2".ConvertTo<int>();
//intFromString is an integer with value 2


Reverse Dictionary: Group from child array elements and get its minimum key Objective

I have following array and would like to group by each child and get its smallest Id:-

var list = new[]
{
    new { Id = 1, Childs = new [] { 'a', 'b' } },
    new { Id = 2, Childs = new [] { 'a' } },
    new { Id = 3, Childs = new [] { 'a', 'c' } },
    new { Id = 4, Childs = new [] { 'b' } },
    new { Id = 5, Childs = new [] { 'b' } },
    new { Id = 6, Childs = new [] { 'b', 'c' } },
};

Attempt

var groups = list
                .SelectMany(item => item.Childs)
                .Distinct() // for each unique child
                .ToDictionary(
                    child => child, // with each child as key
                    child => list.Where(item =>
                                item.Childs.Any(c =>
                                    c == child)) // get items with corresponding child
                                .Min(item => item.Id)); // extract min Id

foreach (var g in groups)
{
    Console.WriteLine($"{ g.Key }: { g.Value }");
}

// Prints
// a: 1
// b: 1
// c: 3

Rationale

a contains in Id 1, 2 and 3 and the smallest is 1.

Improvement

I'm trying to solve it with LINQ and thinking GroupBy would be the go-to solution but I'm stuck with splitting each child element as the key.

I would really appreciate if someone have any idea to solve it in a more elegant way!

Indeed, you could use GroupBy but I think it's easier to solve with the query syntax because we also need the item from the first list to sort by its Id. Without it, we would need to do additional lookups like you do.

var result =
    from item in items // from + from = SelectMany
    from child in item.Children
    orderby item.Id // sort in ascending order
    group item.Id by child into g // group by child
    select (Child: g.Key, MinId: g.First()); // the first item in each group is the min-id

If you use select g instead, you'll get IGrouping<char, int> which you can easily turn into a dictionary or a lookup.


Retirement-saving calculator I asked a math question and was hoping someone could have a look at the code for me as well. Unfortunately you might have to go look at the original question here.

protected void ButtonCalRetire_Click(object sender, EventArgs e)
{
double FVc = 0;
double FVd = 0;
double PV = double.Parse(TextBoxCurrentSave.Text);
double i = double.Parse(TextBoxRoR.Text) / 100;
double b = (double)(int.Parse(TextBoxRetireAge.Text) - int.Parse(TextBoxCurrentAge.Text));
double R1 = double.Parse(TextBoxCurrentMonthlyContribution.Text);
double g2 = double.Parse(TextBoxAnnuityGrowthRetire.Text) / 100;

FVc = PV * Math.Pow((1 + i / 12), (b * 12));

FVd = R1 * (Math.Pow((1 + i / 12), (b * 12)) - Math.Pow((1 + g2 / 12), (b * 12))) / ((i / 12) - (g2 / 12));

double totSaved = FVc + FVd;

double R2 = double.Parse(TextBoxCurrentMonthlyContribution.Text);
double n = (double)(int.Parse(TextBoxDeathAge.Text) - int.Parse(TextBoxRetireAge.Text));

double PVg = R2 * Math.Pow((1 + g2 / 12), (b * 12)) / (i / 12 - g2 / 12) * (1 - ((Math.Pow((1 + g2 / 12), (n * 12))) / (Math.Pow((1 + i / 12), (n * 12)))));

double diff = FVc + FVd - PVg;

double R3 = diff / (Math.Pow((1 + i / 12), (b * 12)) - Math.Pow((1 + g2 / 12), (b * 12))) / ((i / 12) - (g2 / 12));

LabelTotControNeeded.Text = (R3 + R2).ToString("R ### ##0.00");
LabelControNeeded.Text = R3.ToString("R ### ##0.00");
}

Edit:

If I debug correctly, the PVg above (line repeated below) returns 0

double PVg = R2 * Math.Pow((1 + g2 / 12), (b * 12)) / (i / 12 - g2 / 12) * (1 - ((Math.Pow((1 + g2 / 12), (n * 12))) / (Math.Pow((1 + i / 12), (n * 12)))));

Edit:

Here is the calculator: http://exceed.myib.co.za/calc

Naming conventions

As you will have noticed from the coloring: you're not following naming conventions. Variables inside a method are lowerCamelCase which means this

FVc

becomes this

fvC

Naming

Your variables have very unclear names. From the context I know that this is part of a math formula, but you might be able to clarify them. If possible, consider using full words that clearly explain what that variable means.

In that regard I would definitely change this

totSaved

into this

totalSaved

"Saving" 2 characters is nothing, but it lowers readability.

Parsing

You parse the same textbox fields many times. Instead, parse it once and put it a variable. Now you can reference that variable every time and it will be a lot easier to work with.

Formula clarity

You should also consider splitting up your long formulas (insofar a portion of a formula would make sense) to help you in case something needs to be debugged. It will be easier to check intermediate values and to keep the overview over what your formula represents.