English to Entean (V2) with maketrans I wrote a python script that converts English to Entean language from an anime called "The Devil is a part timer".

This is the second version of the script which was first posted here Translating from English to Entean. This version is compatible with both Python 2 and 3.This version uses the str.maketrans method which is faster and more Pythonic.

In the anime, the language in Ente Isla is called Entean, which is basically most of the consonants in the English alphabet switched around. Vowels and the consonants 'L', 'N', 'Q' keep their original position. ABCDEFGHIJKLMNOPQRSTUVWXYZ is changed to AZYXEWVTISRLPNOMQKJHUGFDCB. For example, 'human' is 'tupan', and 'world' becomes 'foklx'.

import sys

PY2 = sys.version_info[0] == 2 #Returns True if Python version is 2

if PY2:
    from string import maketrans
else:
    maketrans = str.maketrans

FULL_ENGLISH_ALPHABET = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
FULL_ENTEAN_ALPHABET =  "AZYXEWVTISRLPNOMQKJHUGFDCB"

FULL_ENGLISH_ALPHABET += FULL_ENGLISH_ALPHABET.lower()
FULL_ENTEAN_ALPHABET += FULL_ENTEAN_ALPHABET.lower()

tran_tab = maketrans(FULL_ENGLISH_ALPHABET, FULL_ENTEAN_ALPHABET)

def to_entean(text):
    return(text.translate(tran_tab))

def main(text=None, with_output=True):

    if text is None:
        if PY2:
            text = raw_input("Enter English text: ")
        else:
            text = input("Enter English text: ")

    if with_output:
        print(text)

    entean_text = to_entean(text)

    if with_output:
        print(entean_text)

if __name__ == "__main__":
    main()

If you interested the updated code can be found at https://github.com/Logmytech/English-to-Entean

As you've taken most of our advice to heart, there isn't much to complain on stylewise, but a few pointers from my personal opinion:

If possible, avoid doing function call for constants, and avoid redefining constants – You set FULL_ENGLISH_ALPHABET = FULL_ENGLISH_ALPHABET + FULL_ENGLISH_ALPHABET.lower(). Both the redefining of a constant, and doing the lower() on previous version could and should be avoided in my opinion. Just set it to the correct version directly, you know in advance what it is going to be.
Write out variable names – Type out in full the tran_tab variable, and since you're not going to change it, uppercase it, i.e make it ENGLISH_TO_ENTEAN which also encompasses the use instead of just translation_table which is kind of meaningless.
Add docstrings to functions and module – You could/should add documentation to your module, to help understand what this module does

Extend to translate both ways

A natural extension now is to extend it so that it can translate both ways. In addition to the to_english(text) function, I would also make a translate_to(language, text) function so that you can make functions which can choose which way you want to translate.

Look into unit testing and/or doctest

Another possible extension is to add testing of your methods, to ensure proper translation and handling of different cases. Loads of alternatives exists, but I'm going to point you towards unittest and doctest. Both of these are included in vanilla Python. The former can be useful for slightly more complex cases, but I've opted for doctest in my review, as it has a nice side effect of displaying with the docstrings how to use the different functions.

Doctest can be invoked on your module in a variety of ways, but the two versions I've been using the most are either to call it direcly from the command line using python -m doctest my_module.py or through calling a simple do_doctest() function from the main code. The former doesn't require anything besides the actual docstrings, and if you use python -m doctest -v my_module.py you get to see every test it performs (and not only failing tests) and the summary.

Try it out with code below, and change some of the stuff related to >>> lines, and response on following line.

Make main() useful

I know I was the one proposing the main() function you're using, but now that it is maturing, I would make it into a loop asking for which language to translate to and text to translate. This way you'll get a meaning full script to run, in addition to a meaningful module to use.

Note how I used a little trick to 'alias' input = raw_input if in Python 2, so I wouldn't need to do the if PY2 all over my script. Consider also looking into an extra module handling Python 2 and Python 3 like the six module.

Revamped code

Here is my version implementing all of the above suggestions:

"""
In the anime, "The Devil is a part timer", the language in Ente Isla is
called Entean, which is basically most of the consonants in the English
alphabet switched around. Vowels and the consonants 'L', 'N', 'Q' keep
their original position, whilst the rest is simply reversed.

The full alphabet then is that each letter of ABCDEFGHIJKLMNOPQRSTUVWXYZ
is changed to AZYXEWVTISRLPNOMQKJHUGFDCB. For example, 'human' is 'tupan',
and 'world' becomes 'foklx'. Other characters are kept as is.

Some typical usage situation offered by module:

    >>> to_entean("Text, to be translated, into to Entean!")
    'Hedh, ho ze hkanjlahex, inho ho Enhean!'

    >>> to_english("Tupan foklx!")
    'Human world!'

    >>> translate_to('entean', 'Kind of cool')
    'Rinx ow yool'
"""

import sys

# Set to True if Python major version is 2
PY2 = sys.version_info[0] == 2

if PY2:
    from string import maketrans

    # Alias input to simplify input routines
    input = raw_input
else:
    maketrans = str.maketrans

ENGLISH_ALPHABET = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
ENTEAN_ALPHABET = "AZYXEWVTISRLPNOMQKJHUGFDCBazyxewvtisrlpnomqkjhugfdcb"

ENGLISH_TO_ENTEAN = maketrans(ENGLISH_ALPHABET, ENTEAN_ALPHABET)
ENTEAN_TO_ENGLISH = maketrans(ENTEAN_ALPHABET, ENGLISH_ALPHABET)

def to_entean(text):
    """Return text translated from English to Entean.

    >>> to_entean("Some random text to translate, with punctuation chars!")
    'Jope kanxop hedh ho hkanjlahe, fiht munyhuahion ytakj!'

    >>> to_entean(ENGLISH_ALPHABET) == ENTEAN_ALPHABET
    True
    """
    return text.translate(ENGLISH_TO_ENTEAN)

def to_english(text):
    """Return text translated from English to Entean.

    >>> to_english("Noh jo eajc ho unxekjhanx, kivth?")
    'Not so easy to understand, right?'

    >>> to_english(ENTEAN_ALPHABET) == ENGLISH_ALPHABET
    True
    """
    return text.translate(ENTEAN_TO_ENGLISH)

ALL_TRANSLATIONS = {
    'entean' : to_entean,
    'english' : to_english
    }

def translate_to(language, text):
    """Return translated text according to mode.

    >>> translate_to('english', 'Tupan foklx')
    'Human world'

    >>> translate_to('entean', 'Human world')
    'Tupan foklx'

    >>> translate_to('norwegian', 'This will fail!')
    Traceback (most recent call last):
    ValueError: Unknown translation language.
    """

    translate_function = ALL_TRANSLATIONS.get(language, None)

    if translate_function:
        return translate_function(text)
    else:
        raise ValueError("Unknown translation language.")

def do_doctest():
    """Utility method to run all doctests from within script."""

    import doctest
    doctest.testmod()

def main():
    """Loop asking for language and text to translate."""

    print(' Translation utility '.center(72, '*'))
    print('\nLegal languages: {}'.format(', '.join(ALL_TRANSLATIONS)))

    while True:
        to_language = input("Language to translate to (or 'quit'): ").lower().strip()

        if to_language == 'quit':
            break

        text = input("Enter text to translate: ")

        print('Translated text: {}'.format(translate_to(to_language, text)))

if __name__ == "__main__":
    do_doctest()
    main()


Files.walkTree is causing Assignment Branch Condition too high My code is working correctly, but is resulting in a "Assignment branch condition too high" warning with a score of 12.08 on CodeBeat.

Codebeat is an automated code review utility that helps developers write clean code. I have been using it lately in order to monitor the quality of my code since clean code is being a necessity in today's word. Codebeat calculates the complexity of the code written and assigns a score, so it seems that the complexity of this method seems a bit high that usual. I would be grateful if someone can tell me what is complex about this method.

private void populateDictionary() {
    try {
        Files.walkFileTree(Paths.get(rootDirectory.getCanonicalPath()), EnumSet.of(FileVisitOption.FOLLOW_LINKS), Integer.MAX_VALUE, new SimpleFileVisitor<Path>() {
            @Override
            public FileVisitResult visitFile(Path path, BasicFileAttributes attrs) throws IOException {
                addToDictionary(path);
                return FileVisitResult.CONTINUE;
            }
        });
    }catch(IOException e) {
        System.out.println(e.getMessage());
    }
    System.out.println(this.indexedFileContents.keySet().size() + " files read in directory " + this.rootDirectory);
}

Can anyone tell me what am I doing wrong and how I can improve my code?
Thanks in advance.

Well I figured out a way to refactor my code, and obtained a non-complex code by creating a method that returns the visitor named getSimpleFileVisitor(). I also created another method that will allow me to print the results that I named printFinalResults(). I have also refactored the method's name to walkFileTree().
Maybe my question was a little bit ambiguous, sorry for any inconvenience.

private void walkFileTree() {
    try{
        Files.walkFileTree(Paths.get(rootDirectory.getCanonicalPath()), EnumSet.of(FileVisitOption.FOLLOW_LINKS), Integer.MAX_VALUE, getSimpleFileVisitor());
    }catch(IOException e) {
        System.out.println(e.getMessage());
    }
    printFinalResults();
}

private SimpleFileVisitor<Path> getSimpleFileVisitor(){
    SimpleFileVisitor<Path> simpleFileVisitor = new SimpleFileVisitor<Path>() {
        @Override
        public FileVisitResult visitFile(Path path, BasicFileAttributes attrs) throws IOException {
            addToDictionary(path);
            return FileVisitResult.CONTINUE;
        }
    };
    return simpleFileVisitor;
}

private void printFinalResults() {
    System.out.println(this.indexedFileContents.keySet().size() + " files read in directory " + this.rootDirectory);
}


Interface for Spotify API I'm trying to make a scala interface to the spotify API. I'm using it to learn Scala. I come from mostly a python background.

import dispatch._, Defaults._

trait Request {
  val name: String
  val endpoint: String
}

case class Track(name: String) extends Request {
  override val endpoint = "track.json"
}

case class Album(name: String) extends Request {
  override val endpoint = "album.json"
}

case class Artist(name: String) extends Request {
  override val endpoint = "artist.json"
}

object Spotify {

  def getResponse(req: Request) = {
    val params = Map("q" -> req.name)
    val spotifyUrl = host("ws.spotify.com")

    spotifyUrl / "search"/ "1" / req.endpoint <<? params

  }

  val foo = Album("foo")
  val songs = Http(getResponse(foo) OK as.String)

}

I feel like getResponse should be in the Request trait, or something since it's generic enough to only need the overridden endpoint - but not sure.
Any general recommendations on structuring this sort of thing would be helpful.

Decouple everything.

I am certain that Scala veterans will have more to say than I do and possibly comment over my comments.

Here are some changes and respective comments to your code.

import dispatch._, Defaults._

// decouple from any "master" class.
case class Track(name: String)

case class Album(name: String)

case class Artist(name: String)

// This is called a structural type. We are using a def
// because a val restricts extensibility.
type Named = {def name: String}

// Separate out the endpoints into their own category
object endpointResolver {

  // Use a partial function which you can re-use in order parts
  // of your program. You may use this in your testing as well.
  val endPoints: PartialFunction[Named, String] = {
    case _: Track => "track.json"
    case _: Album => "album.json"
    case _: Artist => "artist.json"
  }

  // use an implicit class. This allows you to add methods
  // to existing classes seamlessly.
  implicit class nrEndpoint(namedRequest: Named) {
    def endpoint: String =
      endPoints apply namedRequest
  }

}

object Spotify {

  // Specify the return type. I was not sure what it was initially.
  def getResponse(req: Named): Req = {

    val params = Map("q" -> req.name)

    val spotifyUrl = host("ws.spotify.com")

    // import and utilise this implicit class
    import endpointResolver.nrEndpoint

    spotifyUrl / "search"/ "1" / req.endpoint <<? params

  }

  val foo = Album("foo")

  // OK is another example of an implicit being used. Use an IDE
  // such as IntelliJ IDEA to navigate through the vast depths
  // of Scala source.
  val songs = Http(getResponse(foo) OK as.String)

}


Making text field taggable I'm new to JavaScript and new to jQuery plugin development. My plugin is really easy I just want to taggify a

    (function($) {
        var allTags;
        $.fn.taggify = function(options) {
            allTags = this;
            _create(allTags);
            return this;
        };

        function _create(theElement) {
            var inputWrapper = $('

');
            var input = $('<input></input>');
            input.attr('type', 'text');
            input.attr('autocomplete', 'off');
            input.css('border', 'none');
            input.css('outline', 'none');
            inputWrapper.html(input)

            allTags.append(inputWrapper);
            _event(input);
        }

        function _event(theText) {
            theText.keyup(function(e) {
                if(e.keyCode === 13) {
                    var aTag = $('

');
                    aTag.addClass('tag-choice');

                    var aSpan = $('');
                    aSpan.addClass('tag-label');

                    var tagText= theText.val();
                    aSpan.html(tagText);
                    aTag.html(aSpan);

                allTags.prepend(aTag);
                theText.val('');
            }
        });
    }

})(jQuery);

Your code doesn't look too shabby! A few comments.

Declaring but then not using options made me stumble. If you don't have any options, don't make a parameter called options.
In my opinion there's not much point in separating _create and _event into two separate functions. If you do, you should rename _event so that it's more like a function name. Functions should be named such that they start with a verb; try something like _attachListener.
It's fine to use a closure-scope variable, but you're kind of half-using it. If you combine your two functions, you'll be able to get rid of it entirely.
_create isn't using its theElement parameter.
You should take advantage of method chaining when creating input. Also, consider naming jQuery variables with a leading $ to make it very explicit what type they are.
The .html() function does not take a jQuery object as a valid parameter; it takes either an htmlString or a function. Instead you should use inputWrapper.append(input);.
In fact, rather than creating an inputWrapper at all, use the .wrap() function.
Do you really want to use aSpan.html(tagText);, or do you want to use .text()? This depends on your use case but usually people will expect the behaviour of .text() (e.g. not having to escape their angle brackets).
For consistency, either name them both anInput and aSpan or drop the prefix.
Note that using function chaining, you can in fact completely eliminate the need for having any variables at all, if you prefer.

Here's my proposed rewrite:

(function($) {
  $.fn.taggify = function(options) {
    create(this);
    return this;
  };

  function create($theElement) {
    var $input = $('<input></input>')
      .attr('type', 'text')
      .attr('autocomplete', 'off')
      .css('border', 'none')
      .css('outline', 'none')
      .wrap('

');

    $input.on('keyup', function(e) {
      if (e.keyCode === 13) {
        var tagText = $input.val();
        var $span = $('');

        $span.text(tagText).wrap('

');
        $theElement.prepend($span.parent());
        $input.val('');
      }
    });

    $theElement.append($input.parent());
  }
})(jQuery);

$(function() {
  $('ul').taggify();
});
ul {
  background: #ccc;
  padding: 1em;
}


Controller scope inside Directive with Angular I decided to write a simple runnable example to try and express my concern:

index.html

      <prize-directive ng-repeat="prize in prizes"></prize-directive>

Inside my shopController I have set a static prize list just for the purpose of keeping it simple.

ShopController.js

angular.module('app.controllers').controller('ShopController',
   ['PrizeService', '$scope', function (PrizeService, $scope) {

    $scope.prizes = [
      {name: 'First Prize', cost: 10,
         image: 'upload/prize/image1.png', note: 'This is a note'},
      {name: 'Second Prize', cost: 20,
         image: 'upload/prize/image2.png', note: 'This is a note'},
      {name: 'Third Prize', cost: 30,
         image: 'upload/prize/image3.png', note: 'This is a note'}
    ];

    $scope.reclaim = function (prize) {
      console.log(prize);
    };
}]);

In the original code I use the PrizeService to load a list of available prizes, but back to my main problem:

PrizeDirective

angular.module('app.directives').directive('prizeDirective', function () {
  return {
    restrict: 'E',
    templateUrl: 'js/directives/PrizeDirective.html'
  };
});

HTML:

{{prize.name}}

    Pontos: {{prize.cost}}

{{prize.note}}

<reclaim-directive></reclaim-directive>

Here in the HTML portion of the directive, I'm using a variable name that was defined in the index.html. Although it bothers me, I'm willing to let it go because the variable name was defined inside the ng-repeat code, which is inside the prize-directive directive. But when we go to the reclaim-directive, things get a little bit more heated:

angular.module('app.directives').directive('reclaimDirective', function () {
  return {
    restrict: 'E',
    template: '<button ng-click="reclaim(prize)">Resgatar</button>'
  };
});

Note: I'm actually using the templateUrl here, but again, I'm simplifying it just to show my point.

Now I'm using a function name defined inside ShopController in the Reclaim Directive. Differently from OOP, this is not really an inheritance of an abstract method which even the IDE can tell you that something is wrong and, to me, it seems like a bad practice.

I would love to hear people's thoughts on this approach and if there is anything that I'm missing that could help clean up this code.

Now I'm using a function name defined inside ShopController in the Reclaim Directive... it seems like a bad practice.

Yep, this kind of code will be hard to debug and maintain.

You're depending on the existence of reclaim to be somewhere in the scope chain, on some controller enclosing your directive.
There's no indication whenever you use the directive that reclaim is missing in the scope. The directive will fail only if you click it.
Without proper tools like Batarang, in a highly nested controller structure, reclaim will be hard to find when you don't know which controller it resides.

If <reclaim-directive> isn't that huge, and is not used anywhere else independently, then it's contents should belong to <prize-directive>.

If <reclaim-directive> is big and independent enough to be a separate directive, then consider making it a sibling of <reclaim-directive>. That way, you avoid the extra nested level:

<div ng-controller="ShopController as shop">
  <prize-directive ng-repeat="prize in prizes"></prize-directive>
  <reclaim-directive></reclaim-directive>
</div>

Most importantly, instead of depending on implicit existence of reclaim in the scope, why not provide it to the directive explicitly? Construct a scope for the directive, which accepts a binding of reclaim. I believe this is the way to do it:

<prize-directive ng-repeat="prize in prizes" on-reclaim="reclaim"></prize-directive>

angular.module('app.directives').directive('prizeDirective', function () {
  return {
    restrict: 'E',
    templateUrl: 'js/directives/PrizeDirective.html',
    scope: {
      onReclaim: '='
    }
  };
});

<!-- onReclaim assigned to on-reclaim comes from the prizeDirective -->
<reclaim-directive on-reclaim="onReclaim"></reclaim-directive>

angular.module('app.directives').directive('reclaimDirective', function () {
  return {
    restrict: 'E',
    template: '<button ng-click="onReclaim(prize)">Resgatar</button>',
    scope: {
      onReclaim: '='
    }
  };
});


XML Schema File that validates my XML File I just want to know if my schema file is correct:

XML File:

  <?xml version="1.0" encoding="UTF-8"?>
  <xs:books xmlns="http://www.w3.com/1999/XML/prjXMLValidation"
   xmlns:xs="http://www.w3.org/2001/XMLSchema/prjXMLValidation"
      xs:noNamespaceSchemaLocation="collection.xsd">
   <xs:book title="A Lesson Before Dying" author="Ernest J. Gaines" version="1">
<xs:stars>
     5
    </xs:stars>
    <xs:description>
  This is a story about a young man that was wrongly
  accused of killing a white man in the south at a time
  when blacks were not given the same rights as whites.
  This is the story of how Jefferson becomes a man.
    </xs:description>
<xs:thoughts>
    Great book!
</xs:thoughts>
  </xs:book>
</xs:books>

Here is my XSD File:

<?xml version="1.0" encoding="utf-8"?>
<xs:schema elementFormDefault="qualified" xmlns:xs="http://www.w3.org/2001/XMLSchema"
       targetNamespace="http://www.w3.org/2001/XMLSchema/prjXMLSchema">
  <xs:element name="books">
    <xs:complexType>
      <xs:element name="book">
        <xs:complexType>
          <xs:sequence minOccurs="1" maxOccurs="unbounded">
            <xs:element name="stars">
              <xs:complexType>
                <xs:choice minOccurs="0" maxOccurs="1">
                  <xs:element ref="1" />
                  <xs:element ref="2" />
                  <xs:element ref="3" />
                  <xs:element ref="4" />
                  <xs:element ref="5" />
                </xs:choice>
              </xs:complexType>
            </xs:element>
            <xs:element name="description" type="xs:string" />
            <xs:element name="thoughts" type="xs:string" />
          </xs:sequence>
          <xs:attribute name="title" type="xs:string" use="required" />
          <xs:attribute name="author" type="xs:string" use="required" />
          <xs:attribute name="version" type="xs:string" />
        </xs:complexType>
      </xs:element>
    </xs:complexType>
  </xs:element>
</xs:schema>

Overall your structures look OK. Your actual XML document could be indented more neatly, but it's OK.

I would recommend that you change your stars element to have an attribute. Using the PCDATA space is not a great place to put a numeric value.... Actually, I would recommend adding a stars attribute to the thoughts element (seems like a logical place to put it)....

<xs:thoughts stars="5" >
    Great book!
</xs:thoughts>

This brings up the validation of the stars value too.... you have broken references in your XSD:

          <xs:complexType>
            <xs:choice minOccurs="0" maxOccurs="1">
              <xs:element ref="1" />
              <xs:element ref="2" />
              <xs:element ref="3" />
              <xs:element ref="4" />
              <xs:element ref="5" />
            </xs:choice>
          </xs:complexType>

The references 1 .... 5 are not declared. This is a problem. But, you should be using references (just not those ones....). You should be separating out your XSD types and then merging them up in complex-types with ref references. You should also be declaring the types outside of their usage... which makes things easier to manage.

Consider the following (which changes the stars validation, and location).

<xs:simpleType name="startype">
  <xs:restriction base="xs:string">
    <xs:pattern value="[1-5]"/>
  </xs:restriction>
</xs:simpleType>

.....
<xs:element name="thoughts">
  <xs:complexType>
    <xs:simpleContent>
      <xs:extension base="xs:string">
        <xs:attribute name="stars" type="startype" />
      </xs:extension>
    </xs:simpleContent>
  </xs:complexType>
</xs:element>


Factory design pattern with music classes I found a definition of the factory design pattern as:

  Define an interface for creating an object, but let the subclasses decide which class to instantiate.

I understood the first part "defining interface for creation an object", but I am not able to understand the second part of "let subclasses decide which class to instantiate".

Is the following example a correct implementation of factory design pattern? If yes, please help to me to understand second part of definition with respect to the example.

        #include <iostream>
    using namespace std;

    enum genre_e{ROCK,POP, REGGAE, INVALID};

    /*Base Class*/
    class Music {
    public:
     virtual void song() = 0;
    };

    /*Derived class Rock from Music*/
    class Rock: public Music
    {
    public:
     void song()
     {
      cout<<"Nirvana: Smells like a teen spirit\n";
     }
    };

    /*Derived class Pop from Music*/
    class Pop: public Music
    {
    public:
     void song()
     {
      cout<<"Michael Jackson: Billie Jean\n";
     }
    };

    /*Derived class Reggae from Music*/
    class Reggae: public Music
    {
    public:
     void song()
     {
      cout<<"Bob Marley: No woman, No cry\n";
     }
    };

    /*Factory Class*/
    class MusicFactory
    {
    public:
     /*Factory Method*/
     Music *getMusic(genre_e genre)
     {
      Music *music = NULL;

      /*Logic based on Genre*/
      switch(genre)
      {
      case ROCK:
       music = new Rock();
       break;
      case POP:
       music = new Pop();
       break;
      case REGGAE:
       music = new Reggae();
       break;
      default:
       music = NULL;
       break;
      }
      return music;
     }
    };

    int main()
    {
     /*Create factory*/
     MusicFactory *musicFactory = new MusicFactory();

     /*Factory instantiating an object of type ROCK*/
     Music *music = musicFactory->getMusic(ROCK);

     cout<<"Song: ";
     if(music)
      music->song();
     else
      cout<<"Wrong selection dude/dudette !!";
    }

You have implemented is a factory pattern.
What you describe in the first paragraph is an Abstract factory pattern. Slightly different.
An abstract factory allows you to plug in one of multiple different factory's in at run-time to get different situations.
As a big note. Stop using pointers like that. That is a complete no no in C++

Do you need to create the factory dynamically.
A local object will probably do.

Even though the factory probably needs to create the objects dynamically you don't want to return pointers (but rather a smart pointer or a reference depending on how you want to handle ownership semantics (there are several good articles on SO about this so go have a read)).

Example:
class MusicFactory
{
    public:
        virtual std::unique_ptr<Music> getMusic(genre_e genre) = 0;
};

class AsianMusicFactory: public MusicFactory
{
    public:
        virtual std::unique_ptr<Music> getMusic(genre_e genre);
};
class AfricanMusicFactory: public MusicFactory
{
    public:
        virtual std::unique_ptr<Music> getMusic(genre_e genre);
};
class EuropeanMusicFactory: public MusicFactory
{
    public:
        virtual std::unique_ptr<Music> getMusic(genre_e genre);
};
class SouthAmericannMusicFactory: public MusicFactory
{
    public:
        virtual std::unique_ptr<Music> getMusic(genre_e genre);
};
class NorthAmericannMusicFactory: public MusicFactory
{
    public:
        virtual std::unique_ptr<Music> getMusic(genre_e genre);
};
// Special factory used when running unit tests.
class UnitTestMusicFactory: public MusicFactory
{
    public:
        virtual std::unique_ptr<Music> getMusic(genre_e genre);
};

So you have a bunch of different factories that all implement the same interface. But how do you know which one you should use. A common way is for a single creation point. Just ask the system it will create one for the lifetime of the application.
MusicFactory& getMusicFactory()
{
     // Note the static here.
     // It will only be created and initialized once (on first call).
     static std::unique_ptr<MusicFactory>  factory = createMusicFactory();
     return *factory;
}
// Never call this method directly.
// This function should only be called by getMusicFactory().
std::unique_ptr<MusicFactory> createMusicFactory()
{
     // Get info from user configuration.
     // Command line arguments. IP information etc whatever helps you
     // Make the decision on the actual factory that should be instantiated.
     // So we can decide what factory should be used by the application.
     std::unique_ptr<MusicFactory>  result(new XXXXX);
     return result;
}

int main()
{
    // Get the factory.
    MusicFactory&          fac   = getMusicFactory();

    // Use the factory to create the music.
    // At compile time we don't know what continents the music is from
    // as this is decided at run-time by he system and what factory is
    // created.
    std::unique_ptr<Music> music = fac.getMusic(ROCK);

    cout<<"Song: ";
    music->song();   // Should test for NULL
}


Composing a CSS transformation I have a function that creates a css transform property string
based on some global variables. This string can be copied later by a user, so I would prefer to keep the output short.

This is my code:

var createTransformation = () => {
        var final_transformation = "";
        if (x)
            final_transformation += "rotateX(" + x + "deg)";
        if (y)
            final_transformation += " rotateY(" + y + "deg)";
        if (z)
            final_transformation += " rotateZ(" + z + "deg)";
        if (scalex !== 1 || scaley !== 1)
            final_transformation += " scale(" + scalex + "," + scaley + ")";
        if (translatex !== 0 || translatey !== 0 || translatez !== 0) {
            if (translatex !== 0 && translatey == 0 && translatez == 0)
                final_transformation += " translateX(" + translatex + "px)";
            else if (translatey !== 0 && translatex == 0 && translatez == 0)
                final_transformation += " translateY(" + translatey + "px)";
            else if (translatez !== 0 && translatex == 0 && translatey == 0)
                final_transformation += " translateZ(" + translatez + "px)";
            else if (translatex == 0)
                final_transformation += " translateY(" + translatey + "px) translateZ(" + translatez + "px)";
            else if (translatez == 0)
                final_transformation += " translateX(" + translatex + "px) translateY(" + translatey + "px)";
            else if (translatey == 0)
                final_transformation += " translateX(" + translatex + "px) translateZ(" + translatez + "px)";
            else
                final_transformation += " translateX(" + translatex + "px) translateY(" + translatey + "px) translateZ(" + translatez + "px)";
        }
        //if scalex != 1 || scaley != 1
        //If the skew's sliders have been changed e.g, moved from 0 to 1 or from 0 to 100 add skew property
        if (skewX || skewY)
            final_transformation += " skew(" + skewX + "deg," + skewY + "deg)";
        return final_transformation;
    };

As you can see there are a lot of if statements. I was wondering, is there anything I can do to improve this code?

I would strongly consider storing the various variables that define the transformation in a structured object like this:

let transformConfig = {
    rotate: {
        x: ...,
        y: ...,
        z: ...
    },
    scale: {
        x: ...,
        y: ...
    },
    translate: {
        x: ...,
        y: ...,
        z: ...
    },
    skew: {
        x: ...,
        y: ...
    }
};

That gives you the ability to easily iterate over this object to create transformation strings.  An example of this iteration might be:

function createTransformString(transformConfig) {
    // a variable to store array of individual transform strings
    var transformStrings = [];

    // store functions that build strings for each transform type
    // not shown here but you may want to consider ECMA6 templating
    var buildString = {
        rotate: function(rotate) {
            for (dim in rotate) {
                transformStrings.push( 'rotate' + dim + '(' + rotate[dim] + 'deg)' );
            }
        },
        scale: function(scale) {
            scale.x = scale.x || 1;
            scale.y = scale.y || 1;
            transformStrings.push( 'scale(' + scale.x + ',' + scale.y + ')' );
        },
        translate: function(translate) {
            for (dim in translate) {
                transformStrings.push( 'translate' + dim + '(' + translate[dim] + 'px)' );
            }
        },
        skew: function(skew) {
            skew.x = skew.x || 0;
            skew.y = skew.y || 0;
            transformStrings.push( 'skew(' + skew.x + 'deg,' + skew.y + 'deg)' );
        }
    };

    // iterate the config object to generate strings
    for (transformAction in transformConfig) {
        let transform = transformConfig[transformAction];
        buildString[transformAction](transform);
    }
    return transformStrings.join(' ');
};

Usage:

let transformConfig = { ... };
let transformString = createTransformString(transformConfig);

This approach would limit the repeat code you have for each transformation string and would also take care of your if concern in that only those transformation types specified in the config would be executed.

This also more directly injects the transformation configuration information into the function that generates the strings, meaning you no longer have to rely on variables that may or may not be present in inherited scope.  This means you could now have your createTransformString() function declared in a wholly separate include file from the code where the actual transformation(s) values are generated, promoting code re-use.

Since you are using ECMA6, I would also consider building this as a class (or classes).

Finally, from a stylistic standpoint, I would recommend sticking with camelCase instead of snake_case, as this is pretty much the de facto standard for javascript. At a minimum, don't intermingle the two. Additionally, I would suggest watching your line length. You should strive to keep lines of code under ~80 characters per line to improve code readability.


Find plane (walls, floor, ceiling) that a falling car will land on I have a function that takes a falling car's location & velocity as well as the world gravity. The world has a floor, ceiling, and walls that are in fixed positions. The car's x & y velocity can't change once it's airborne, but gravity does force the car down. I'm trying to find exactly which plane the car will land on first.
This is my current solution, which actually works just fine:
#include <math.h>

// Vector stuff

typedef struct vector
{
    double x;
    double y;
    double z;
} Vector;

static inline Vector add(Vector vec1, Vector vec2)
{
    return (Vector){vec1.x + vec2.x, vec1.y + vec2.y, vec1.z + vec2.z};
}

static inline Vector multiply(Vector vec1, Vector vec2)
{
    return (Vector){vec1.x * vec2.x, vec1.y * vec2.y, vec1.z * vec2.z};
}

static inline double dot(Vector vec1, Vector vec2)
{
    return vec1.x * vec2.x + vec1.y * vec2.y + vec1.z * vec2.z;
}

static inline double magnitude(Vector vec)
{
    return sqrt(dot(vec, vec));
}

static inline Vector double_to_vector(double num)
{
    return (Vector){num, num, num};
}

// find the falling car's landing plane

int find_landing_plane(Vector car_location, Vector *car_velocity, double *gravity)
{
    Vector l = car_location;
    Vector v = *car_velocity;
    Vector V_simulation_dt = double_to_vector(simulation_dt);
    double g = *gravity * simulation_dt;

    if (fabs(l.y) >= 5120)
        return 5; // this is a special exception

    while (1)
    {
        if (magnitude(v) < 2300) // 2300 = terminal velocity
            v.z = v.z + g;
        l = add(l, multiply(v, V_simulation_dt));

        // there must be a better way to do this...
        if (l.x >= 4080)
            return 0; // wall
        if (l.x <= -4080)
            return 1; // wall
        if (l.y >= 5110)
            return 2; // wall
        if (l.y <= -5110)
            return 3; // wall
        if (l.z >= 2030)
            return 4; // ceiling
        if (l.z <= 20)
            return 5; // floor
    }
}

I'm new to C, but I do have a few years of programming experience... SURELY there's a better way to do this? I was thinking perhaps something to do with the trajectory formula, or just a way to make the if statements nicer to look at would also be great.

Don't hardcode values
There are lots of magic values that you are using in your code, from the terminal velocity, the wall coordinates to the numbering of the walls. Try to give everything a name: declare constants for the coordinates and terminal velocity, and create an enum for the six faces of the enclosing box:
enum Face {
    FACE_RIGHT,
    FACE_LEFT,
    FACE_BACK,
    FACE_FRONT,
    FACE_TOP,
    FACE_BOTTOM,
};

static const double terminal_velocity = 2300;
static const double right_wall_position = 4080;
...

It's a little bit more typing, but there are several benefits. In particular, it makes the code more self-documenting. Consider the return type of find_landing_plane():
enum Face find_landing_plane(...) {
    ...
    if (l.x > right_wall_position)
        return FACE_RIGHT;
    ...
}

You also don't need to add a comment to this line anymore:
if (magnitude(v) < terminal_velocity)

Consider using arrays to avoid repetition
You could get rid of the repeated if-statements by treating the vector as an array, and also having an array to encode the positions of the planes. It would look like this:
static const double plane_positions[6] = {4080, -4080, 5110, -5110, 2030, 20};
...
enum Face find_landing_plane(Vector car_location, Vector *car_velocity, double *gravity)
{
    ...
    while (1)
    {
        // update l
        ...

        double *l_vec = &l.x;

        for (int i = 0; i < 6; i++) {
            if (i % 2 == 0 ? l_vec[i / 2] >= plane_positions[i] : l_vec[i / 2] <= plane_positions[i])
                return i;
        }
    }
}

However, I'm not sure it is all that better, as it uses a complex expression and no longer uses the enum values by name, making it harder to understand and easier to make mistakes.
Solve the problem analytically
Finding the plane the car hits by updating the position in a loop is inefficient. The problem can be solved analytically. The trajectory the car follows is a parabola up to the point the terminal velocity is reached. You can solve for time the parabola intersects each of the the six planes, and also solve for the time the terminal velocity is reached. If the earliest time found that is not in the past is a plane, you know that plane was hit first. If the earliest time is when the terminal velocity is reached, you then now that the rest of the trajectory is a straight line, so you can then do the even easier step of solving for the intersection of a straight line with six planes.
This is more code to write, although here you could also make an array with the parameters describing each of the six planes, and use a for-loop to check for the intersection with each of them. The advantage is that it should be much more efficient and much more precise (consider that you have a bias in your algorithm when the car hits very close to a corner).
Add a better description of the special exception
Why is fabs(l.y) > 5120 special? You commented that it is an exception, but that still doesn't tell me why. Why is it 5120 (consider making a constant for that as well), and why do you need to handle this case at all, when it looks like the rest of the code would handle that fine?


Rotate matrix 90 degrees I recently did problem 1.7 Rotate Matrix in Cracking the Coding Interview. I realized my code is very different than what is in the book and also from what I'm finding online (I'm having trouble following the code in the book). I just want to know if the code I wrote would be appropriate in an interview setting or would I be expected to write code like the solution in the book.
The problem states

Rotate Matrix: Given an image represented by an N×N matrix, where each
pixel in the image is 4 bytes, write a method to rotate the image by
90 degrees. Can you do this in place?

Here is my code:
def transpose(the_array):
    res = []
    for i in range(len(the_array)):
        transposed = []
        for col in the_array:
            transposed.append(col[i])
        transposed.reverse()
        res.append(transposed)
    return res

print(transpose([[1, 2, 3], [4, 5, 6], [7, 8, 9]]))

Also, would the reverse method be ok to use in an interview?
I just realized my solution may not be considered in place.

I just want to know if the code I wrote would be appropriate in a interview setting or would I expect to write code like the solution in the book.

As we cannot see the solution form the book, this is hard for us to answer.

Your solution seems to work which is ok for learning purposes, the biggest I can think of with your current code is List comprehension.

for col in the_array:
    transposed.append(col[i])
transposed.reverse()

Could be

transposed = [col[i] for col in the_array][::-1]

Note how instead of .reverse() I use the slice annotation to reverse the list [::-1]

We could even make the first for loop another list comprehension (essentially creating a double list comprehension)

def transpose2(the_array):
    return [
        [col[i] for col in the_array][::-1]
        for i in range(len(the_array))
    ]

I should note though that is is a rather elaborate way of writing

def transpose3(arr):
    return zip(*arr[::-1])

Where the zip rotates the reversed list with argument unpacking


Test if a string is a palindrome I got this out of an article on interviews / code challenges. The challenge text goes like this:

  A palindrome is a word, phrase, number, or other sequence of
  characters which reads the same backward or forward. Allowances may be
  made for adjustments to capital letters, punctuation, and word
  dividers. Examples in English include “A man, a plan, a canal,
  Panama!”, “Amor, Roma”, “race car”, “stack cats”, “step on no pets”,
  “taco cat”, “put it up”, “Was it a car or a cat I saw?” and “No ‘x’ in
  Nixon”.

  Write the most efficient function you can that determines whether a
  given string is a palindrome.

  Your function should accept a string as a parameter and return a
  boolean (true if the string is a palindrome, false if it is not).

  Assume that this code will be put into a real production system and
  write accordingly.

Article in question: https://blog.devmastery.com/how-to-win-the-coding-interview-71ae7102d685

My Implementation:

Even though the article provides a way to solve it, my implementation is different in a few ways, so I would like your reasoning on it.

/*
 * Reverses a given string of characters
 *
 * @param {string} string - the string to reverse
 * @returns {string} - the reversed string
 *
 */
const reverseString = string => string.split('').reverse().join('');

/*
 * Function to test if a given string is a palindrome
 *
 * @param {string} string - the string to test
 * @returns {boolean | string} - returns true/false based on the string, being a palindrome,
 *                               or string if there is an error
 *
 */
const isPalindrome = string => {
    if (string) {
      let regexReplacer = /\.|\,| |\!|\?|\'|\"|\/|\\|\-|\_|\+|\‘|\’/g;
      let cleanString = string.replace(regexReplacer, '');
      return (cleanString.toLowerCase() === reverseString(cleanString).toLowerCase());
    }
    return '[isPalindrome]: string not supplied, exiting!';
};

/*
 * Basic function to test equality, poor-man's unit test
 *
 * @param {string} test - the string to test
 * @param {string | boolean} reponse - the expected result
 * @param {function} [func = isPalindrome] - the function to test, defaults to isPalindrome
 *
 */
const expectToEqual = (test, reponse, func = isPalindrome) => {
    if (func(test) === reponse) { console.info('Test for "' + test + '" passed!'); }
    else { console.error('Test for "' + test + '" failed!'); }
};

expectToEqual('A man, a plan, a canal, Panama!', true);

expectToEqual('raceca', false);

expectToEqual('No \‘x\’ in Nixon', true);

expectToEqual('', '[isPalindrome]: string not supplied, exiting!');

Link to jsbin: https://jsbin.com/hilatev/edit?js,console

Questions:

Regex - I'm using /\.|\,| |\!|\?|\'|\"|\/|\\|\-|\_|\+|\‘|\’/g to replace non word characters versus this /[^a-z0–9]/ig which is kind of obvious. But would still like your input.
String testing - The implementation provided in the article goes though the string in a while loop and tests each character. Is this favorable vs how I just compared the strings ? Why ?
Returns - The palindrome function returns true/false based on the string, but also returns a string if there is no string supplied. Is this favorable or I should of just thrown an error ?
Edge cases - In hindsight I'm missing and obvious big case, after the regex replace, what if you end up with an empty string? Besides this did I miss anything else major ? Or minor ?
Tests - I've written my own small test function vs in the article it just prints out the expected result (Ex: console.log(isPalindrome("racecar") + " = true");). Is this favorable? Why?
Other - Things that I may have missed ?

I suggest you whitelist allowable characters instead of blacklisting invalid characters. It's easier to list what's allowed than to chase all possible invalid characters.
Suggesting you move out the regex from the function to avoid recreating it on every call. The pattern is actually an object. Also, I'd suggest throwing an error instead of returning a message. Usually better to test.
The function checks if a string is a palindrome or not. Boolean is the correct return type. Errors should only be used when encountering something unexpected.
A lot, especially when you're blacklisting instead of whitelisting.
Small scale, yes. On larger apps, suggesting the use of a full test framework for more options.


2-sum problem in C++ I'm a beginner at C++; all my previous programming experience is mainly through PLCs in industrial maintenance.

I have to accept 10 integers and a target value, then print all the pairs that sum to the target.

The pairs cannot be of the same integer.  For example, if the target value is 10 then we cannot have a pair of 5 + 5. This code basically executes well enough for me, however I get an overloaded int out at the end; I'm curious why that might be?

Also I understand there are much easier ways to accomplish this task. I feel pretty good with what I was able to come up with given the amount of time I have devoted so I certainly am fine with it, however I wouldn't mind seeing some examples how much more seasoned programmers would have attempted this, if anyone out there is bored.

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

int main()
{
    int run, a, b, ints[11], out[45];

    cout << "\n\t\tAfter inputting 11 integers, this program will evaluate\n\tthe first 10 integers, then will display the pairs of said integers\n\twhich when added together the total will equal the 11th integer.";
    cout << "\n\n\tPlease enter 11 integers: \n\n";

    for (run = 0; run <= 10; run++)
    {
        cout << "\t\t"; cin >> (ints[run]);
    }
    system("cls");
    run = 0;
    for (a = 0; a < 10 && run <= 10; a++)
    {
        for (b = 1; b < 10 && run <= 10; b++)
        {
            if (ints[10] == (ints[a] + ints[b]) && ints[a] != ints[b] && run <= 10)
            {
                out[run] = ints[a];
                run++;
                out[run] = ints[b];
                run++;
            }
        }
    }
    cout << "\n\t\tPerforming critical calculations . . .\n\t\t";
    system("pause");

    cout << "\n\t\t";
    if (*max_element(out, out + 10) == 0)
        cout << "No sum of integers found to equal desired total. ";
    else
    {
        cout << "Pairs of integers that equal the desired total are as follows; ";
        for (int index = 0; index < 11; index++)
        {
            if (out[index] > 0)
            {
                cout << "(" << out[index++] << " , " << out[index] << ")";
                if (index < 11)
                    cout << ", ";
                else if (index == 11)
                    break;
            }

        }
    }
    system("pause");
    return 0;
}
```

Algorithm:
It's brute-force, but for ten elements that's fine.  However, I don't think it will work correctly - it's going to stop at five pairs regardless of how many were generated.

Also, the outer loop should stop before the last element and the inner loop should start one after the current value of the outer loop.  Otherwise you're just making the same comparison multiple times.

The main thing I would suggest that there are other data structures besides basic arrays.  In this case, a vector of pairs would work well.

std::vector<std::pair<int,int>> results;

...

results.push_back(std::make_pair(input[a], input[b]));

...

if(results.size() == 0)
{
    std::cout << "No sum of integers found to equal desired total.\n";
}

...

for(int i = 0; i < results.size(); ++i)
{
    std::cout << '(' << results[i].first << ", " << results[i].second << ')';

.
.
Style:
Always put brackets around your blocks, especially when there's a matching block that does have them (the if/else for showing the results).

Give variables meaningful names, and don't re-use them just because they're the same type.  Also, you don't need to (and usually shouldn't) declare them all at the top of the method; declare them where they're used.

Separate the target number as its own variable, having it as the last item of the input array is less clear.

If you're not calling them anything more specific, i and j are the usual convention for loop variables.

The fake delay for "critical calculations" that in fact are already done is a bit silly. :P


Optimize OrderBy Generic Method Here is my OrderBy method which includes concatenated column names.
Can this be achieved more efficiently?

public static IQueryable<T> Sort<T>(this IQueryable<T> query, List<SortationDto> sortations)
    where T : BaseEntity
{
    sortations.ForEach(s =>
    {
        Expression value = null;
        ParameterExpression parameter = Expression.Parameter(query.ElementType, "item");

        // if sortation is a concatenation of two columns
        if (s.Concatenated.Count > 0)
        {
            Expression concat = null;
            s.Concatenated.ForEach(c =>
            {
                //get first property
                value = c.Column.Split('.').Aggregate((Expression)parameter, Expression.PropertyOrField);

                //if first property add as initial property
                if (concat == null)
                {
                    concat = value;
                }
                else //else concatenate with first property
                {
                    MethodInfo concatMethod = typeof(String).GetMethod("Concat", new Type[] { typeof(string), typeof(string), typeof(string) });
                    value = Expression.Call(concatMethod, concat, Expression.Constant(" "), value);
                }
            });
        }
        else //else use one and only property
        {
            value = s.Column.Split('.').Aggregate((Expression)parameter, Expression.PropertyOrField);
        }

        //build lambda expression
        LambdaExpression predicate = Expression.Lambda(value, parameter);

        //define orderby method name based on current iteration of sortations
        string methodName = (sortations.IndexOf(s) == 0 ? "Order" : "Then") + (s.Direction == "asc" ? "By" : "ByDescending");

        //call method
        MethodCallExpression orderBy = Expression.Call(typeof(Queryable), methodName, new Type[] { query.ElementType, value.Type }, query.Expression, Expression.Quote(predicate));

        //add final expression to query
        query = query.Provider.CreateQuery<T>(orderBy);
    });

    return query;
}

Removed unnecessary concatenation.

public static IQueryable<T> Sort<T>(this IQueryable<T> query, List<SortationDto> sortations)
    where T : BaseEntity
{
    List<SortationDto> combined = sortations.Where(s => s.Concatenated.Count == 0).Union(sortations.SelectMany(so => so.Concatenated)).ToList();

    combined.ForEach(s =>
    {
        ParameterExpression parameter = Expression.Parameter(query.ElementType, "item");
        Expression value = s.Column.Split('.').Aggregate((Expression)parameter, Expression.PropertyOrField);
        LambdaExpression predicate = Expression.Lambda(value, parameter);

        string methodName = (combined.IndexOf(s) == 0 ? "Order" : "Then") + (s.Direction == "asc" ? "By" : "ByDescending");
        MethodCallExpression orderBy = Expression.Call(typeof(Queryable), methodName, new Type[] { query.ElementType, value.Type }, query.Expression, Expression.Quote(predicate));
        query = query.Provider.CreateQuery<T>(orderBy);
    });

    return query;
}


Finite state machine for CSV data I want to read a file containing comma-separated values, so have written a finite state machine:

private IList<string> Split(string line)
{
    List<string> values = new List<string>();
    string value = string.Empty;
    ParseState state = ParseState.Initial;
    foreach (char c in line)
    {
        switch (state)
        {
            case ParseState.Initial:
                switch (c)
                {
                    case COMMA:
                        values.Add(string.Empty);
                        break;
                    case QUOTE:
                        state = ParseState.Quote;
                        break;
                    default:
                        value += c;
                        state = ParseState.Data;
                        break;
                }
                break;
            case ParseState.Data:
                switch (c)
                {
                    case COMMA:
                        values.Add(value);
                        value = string.Empty;
                        state = ParseState.Initial;
                        break;
                    case QUOTE:
                        throw new InvalidDataException("Improper quotes");
                    default:
                        value += c;
                        break;
                }
                break;
            case ParseState.Quote:
                switch (c)
                {
                    case QUOTE:
                        state = ParseState.QuoteInQuote;
                        break;
                    default:
                        value += c;
                        break;
                }
                break;
            case ParseState.QuoteInQuote:
                switch (c)
                {
                    case COMMA:
                        values.Add(value);
                        value = string.Empty;
                        state = ParseState.Initial;
                        break;
                    case QUOTE:
                        value += c;
                        state = ParseState.Quote;
                        break;
                    default:
                        throw new InvalidDataException("Unpaired quotes");
                }
                break;
        }
    }

    switch (state)
    {
        case ParseState.Initial:
        case ParseState.Data:
        case ParseState.QuoteInQuote:
            values.Add(value);
            break;
        case ParseState.Quote:
            throw new InvalidDataException("Unclosed quotes");
    }

    return values;
}

Yes, I know the advice about CSV parsers is "don't write your own", but

I needed it quickly and
our download policy at work would take several days to allow me to
get open source off the 'net.

Hey, at least I didn't start with string.Split() or, worse, try using a Regex!

And yes, I know it could be improved by using a StringBuilder, and it's restrictive on quotes in the data, but

performance is not an issue and
this is only to generate well-defined test data in-house,

so I don't care about those.

What I do care about is the apparent trailing block at the end for mopping up all the data after the final comma, and the way that it's starting to look like some sort of an anti-pattern down there, which was exactly the sort of thing that "good" patterns such as a FSM were supposed to avoid.

So my question is this: is this block at the end some sort of anti-pattern, and is it something that's going to come back to bite me in the future?

So my question is this: is this block at the end some sort of anti-pattern, and is it something that's going to come back to bite me in the future?

Not necessarily.
It depends on whether line can contain end-of-line markers.

If line can contain an EOL character that marks the end of the CSV record for you,
then you could move the logic from the last switch into case EOL: for the state-specific handling as appropriate,
and stop further processing.
Or you want to support multi-line CSV records (newline characters embedded within quotes), then you could continue processing the next line.

If the full length of line is expected to be the complete CSV record,
then it's fine to not terminate it with an explicit EOL and handle it in the extra switch as you did.


grouping people with the same score I made the code underneath to sort my array and group people with the same score to show who all got 1st, 2nd and 3rd.
the "points" variable looks like this
points: {
  '204255221017214977': 3,
  '208993609255485441': 5,
  '382197942762602496': 4,
  '664606841409634324': 5,
  '779857709155090432': 4,
  '381084508276916225': 5,
  '794200859612545075': 4,
  '155149108183695360': 4,
  '438978127973318656': 3,
  '304028235082104843': 2,
  '174548757151481856': 1
}

I was wondering if there is a much easier way to sort and put together the string like I did underneath with much easier code. Cuz I'll be honest this code looks and feels aweful.
example endresult:
:first_place: @Excanator had 5 points
:first_place: @zerow had 5 points
:first_place: @AnimeHotOwO had 5 points

:second_place: @christmas beans had 4 points
:second_place: @ExcavatorGames had 4 points
:second_place: @Dyno had 4 points
:second_place: @LogiBot Premium had 4 points

:third_place: @Restarter v3 had 3 points
:third_place: @YAGPDB.xyz had 3 points

@aNt had 2 points
@Kaori had 1 point

const sorted = Object.keys(points).sort((a, b) => {
    return points[b] - points[a]
})

var results = `\n`

var firstPlace = points[sorted[0]]
var secondPlace = null
var thirdPlace = null
var leftover = null

for(const key of sorted) {
    var amount = points[key]
    if(leftover){
        results += `<@${key}> had ${amount} point${amount === 1 ? '' : 's'}\n`
    } else if(thirdPlace) {
        if(thirdPlace === amount) {
            results += `:third_place: <@${key}> had ${amount} point${amount === 1 ? '' : 's'}\n`
        } else {
            results += `\n`
            results += `<@${key}> had ${amount} point${amount === 1 ? '' : 's'}\n`
            leftover = amount
        }
    } else if(secondPlace) {
        if(secondPlace === amount) {
            results += `:second_place: <@${key}> had ${amount} point${amount === 1 ? '' : 's'}\n`
        } else {
            results += `\n`
            results += `:third_place: <@${key}> had ${amount} point${amount === 1 ? '' : 's'}\n`
            thirdPlace = amount
        }
    } else if(firstPlace) {
        if(firstPlace === amount) {
            results += `:first_place: <@${key}> had ${amount} point${amount === 1 ? '' : 's'}\n`
        } else {
            results += `\n`
            results += `:second_place: <@${key}> had ${amount} point${amount === 1 ? '' : 's'}\n`
            secondPlace = amount
        }
    }
}
```

Functions
Always write code as functions. Even when its just an example, writing code as a function gives it a name, and make you think in terms of that name.
Functions should be as simple as possible doing one thing only. (see rewrite)
Review

USE SEMICOLONS!!!
Don't add code that is redundant or superfluous. Eg  var secondPlace = null can be var secondPlace.
Keep it DRY. (Don't Repeat Yourself). You have a lot of repeated code that does similar or the same. Using function will reduce a lot of repeated code.
Use constants to define magic values. (see rewrite)
Use an Array to hold lists of strings then use Array.join to join them using the delimiter (see rewrite)
Use appropriate names. Names define the abstractions you use, poor naming make code harder to understand especially when a single abstract concept gets many names, or the same name represent multiple abstractions.

points As records should be results
key could be id.
amount could be score

Rewrite
Rewrite is just an example as there are many ways to do this.
Functions
The rewrite breaks the task into 3 parts.

groupByScore groups by score. Using a Map indexed by score to hold arrays of ids with each score. Returns the groups as the Map
sortByScore Takes the grouped results and sorts by score returning an array
resultsByPlace Takes an array of grouped results and returning an array of strings each containing the place, id, and score.

Constants
There are two constants that remove magic values from the code and moves them to one place at the top of the code.
One of the constants is defined as a function score2Str and created the string eg " has 1 point"
Util functions
Common tasks can be written as utility functions. These function can be part of a library of functions you can use within other projects. There is one Util function pluralize that adds a "s" when a value is not != 1. It is defined as a tagged template and can be used as a tagged template
pluralize` point${score}`

Or called as a function
pluralize([` point`], score)

As modules
Thus we get the code as
File strings/utils.jsm
const pluralize = (strs, value) => strs[0] + (value != 1 ? "s" : "") + (strs[1] ? strs[1]: "");
export {pluralize};

File studentResults.jsm
import {pluralize} from "./strings/utils.jsm";

const PLACINGS = ":First Place: ,:Second Place: ,:Thrid Place: ".split(",");
const score2Str = score => " has " + score +  pluralize` point${score}`;

function groupByScore(students) {
    const scoreGroups = new Map();
    Object.entries(students).forEach(([id, score]) => {
        const group = scoreGroups.get(score);
        if (group) { group.push(id) }
        else { scoreGroups.set(score, [id]) }
    });
    return scoreGroups;
}
function sortByScore(scoreGroups) {
    return [...scoreGroups.entries()].sort((gA, gB) => gB[0] - gA[0]);
}
function resultsByPlace(sorted, cats = PLACINGS, scoreStr = score2Str) {
    var idx = 0;
    const results = [];
    for (const [score, group] of sorted) {
        const cat = idx < cats.length ? cats[idx ++] : (idx ++, "");
        results.push(...group.map(id => cat + id + scoreStr(score)));
    }
    return results;
}
export {groupByScore, sortByScore, resultsByPlace};

As running example.
As CodeReview snippets do not support modules below is a working example compatible with CodeReview snippets.
Code

const pluralize = (strs, value) => strs[0] + (value != 1 ? "s" : "") + (strs[1] ? strs[1]: "");
const tag = (tag, props = {}) => Object.assign(document.createElement(tag), props);
const append = (par, ...sibs) => sibs.reduce((p, sib) => (p.appendChild(sib), p), par);

// Rewrite
const studentScores = {'Adam': 3, 'Bill': 5,'Cindy': 4, 'Dafney': 5, 'Eric': 4, 'Fred': 5, 'Graham': 4, 'Hillary': 4, 'Ian': 3, 'Jill': 1, 'Kev': 0};
const PLACINGS = ":First Place: ,:Second Place: ,:Thrid Place: ".split(",");
const score2Str = score => " has " + score +  pluralize` point${score}`;

const results = resultsByPlace(sortByScore(groupByScore(studentScores)));
append(document.body, ...results.map(result => tag("div",{textContent: result})));
function groupByScore(students) {
    const scoreGroups = new Map();
    Object.entries(students).forEach(([id, score]) => {
        const group = scoreGroups.get(score);
        if (group) { group.push(id) }
        else { scoreGroups.set(score, [id]) }
    });
    return scoreGroups;
}
function sortByScore(scoreGroups) {
    return [...scoreGroups.entries()].sort((gA, gB) => gB[0] - gA[0]);
}
function resultsByPlace(sorted, cats = PLACINGS) {
    var idx = 0;
    const results = [];
    for (const [score, group] of sorted) {
        const cat = idx < cats.length ? cats[idx ++] : (idx ++, "");
        results.push(...group.map(id => cat + id + score2Str(score)));
    }
    return results;
}


Multiple audio player - arrow keys & space I am creating a VueJS component of an audio player.
I am having some difficulties in finding the ideal way to get the arrow keys and space bar to work.
I have found a solution, however I think it is not the best, nor the most effective. I've added an event listener to the DOM in the component, but the problem is that I will have multiple players along the page, so I will have multiple DOM listeners.
Is it a good solution or is there a better one?
    // component code
    ...

     document.addEventListener( 'keydown', event => {
        if ( !state.isPlaying )
            return;

        switch ( event.keyCode ) {
            case 32:
                pause();
                break;
            case 37:
                audio.currentTime -= 2;
                break;
            case 39:
                audio.currentTime += 2;
                break;
            default:
                break;
        }
    });
```

Determining the key pressed
The code contains this line:

switch ( event.keyCode ) {

Looking at the MDN documentation for keyCode we see:

Deprecated
This feature is no longer recommended. Though some browsers might still support it, it may have already been removed from the relevant web standards, may be in the process of being dropped, or may only be kept for compatibility purposes. Avoid using it, and update existing code if possible; see the compatibility table at the bottom of this page to guide your decision. Be aware that this feature may cease to work at any time.

There is the related StackOverflow post .keyCode vs. .which from 2010 and the accepted answer has been updated with recent information about using code though browser support isn't complete so you should consider which browsers your users might use (e.g. IE?)
The VueJS documentation for Event handling even has a section about Key Codes.

The use of keyCode events is deprecated and may not be supported in new browsers.

Using keyCode attributes is also permitted:
<input v-on:keyup.13="submit">

Vue provides aliases for the most commonly used key codes when necessary for legacy browser support:

.enter
.tab
.delete (captures both “Delete” and “Backspace” keys)
.esc
.space
.up
.down
.left
.right

Those could be used in the template instead of the script section.
<player @keyup.space="pause" @keyup.right="audio.currentTime -= 2" ...>

Perhaps a method could be created to add (or subtract time) for the left and right arrow keys - e.g.:
methods: {
    addTime: function(delta) {
        audio.currentTime += delta
    }

Then that method can be used in the template:
<player @keyup.space="pause" @keyup.left="addTime(-2)" @keyup.right="addTime(2)">

Multiple handlers
If there is a need to have multiple players and a single event handler then an Event bus might be an ideal solution. There are multiple guides for creating an Event Bus (e.g. on digitalocean.com, medium.com, vuejsdevelopers.com) but I don't see one in the vueJS documentation.


Preparing form data for a POST request with fetch Acquiring a form’s data for asynchronous POST requests has been a long-standing problem. There is the FormData API, but there are still some obstacles when the form’s data is encoded as x-www-form-urlencoded (the default).

My current use case deals with arbitrary forms which might encode data either way; hence, I tried to generalize the way I construct the request’s body. Note that it’s intentional the form is submitted as an asynchronous request as the response is going to be rendered inside the current document.

Can this generalization be simplified further?

document.querySelector('form').addEventListener('submit', performPostRequest);

/**
 * Handles submit events that are about to perform a POST request.
 *
 * @param {Event} event
 */
function performPostRequest(event) {
  // Prevent the default action of sending a regular POST request.
  event.preventDefault();

  const form = event.target;

  fetch(form.action, {
    method: 'post',
    headers: {
      'Content-Type': `${form.enctype}; charset=UTF-8`
    },
    body: constructRequestBody(form)
  })
    .then(console.log)
    .catch(console.error);
}

/**
 * Encodes a form’s data for a POST request’s body.
 *
 * Supported encoding types:
 *
 * - `application/x-www-form-urlencoded`
 * - `multipart/form-data`
 *
 * @param {HTMLFormElement} form
 * @returns {FormData|URLSearchParams}
 */
function constructRequestBody(form) {
  const formData = new FormData(form);

  if (form.enctype === 'multipart/form-data') {
    return formData;
  }

  const requestBody = new URLSearchParams();

  for (const [name, value] of formData) {
    requestBody.append(name, value);
  }

  return requestBody;
}

Can this generalization be simplified further?

return new URLSearchParams(formData)


Converting all cells to text Microsoft Excel cells basically have two values: the formatted value that is shown to the user, and the actual value of the cell. These two values can be extremely different when the cell has certain formatting types such as date, datetime, currency, numbers with commas, etc.

I am writing a script that cleans up database data. It uses find, replace, RegEx, and other functions that act on strings of text. I need to be able to convert data into very specific formats so that my upload tool will accept the data. Therefore I need all of the cells in my Excel spreadsheets to be TEXT. I need the displayed values to match the actual values.

This is surprisingly hard to do in Excel. There is no paste special option that I could find that pastes what you see on your screen as text. "Paste Special -> Values" pastes, predictably, unformatted values. So if you try to "Paste Special -> Values" into cells formatted as TEXT, and they are coming from cells formatted as DATES, you might get something like this:

Old Cell: 4/7/2001

New Cell: 36988

If you paste normally, then the paste goes in as 4/7/2001, but in date format. So if I create a RegEx that looks for 4/7/2001 and changes it to 4/7/01, it will not work, because the underlying value is 36988.

Anyway, I discovered the Cell.Text property and wrote some code that converts all cells in a spreadsheet from formatted to text. But the code has two major problems:

1) It does not work on cells with more than 255 characters. This is because Cell.Text literally displays what the cell is actually showing to the user, and if the column is too narrow, it will display pound signs (########), and columns have a maximum width of 255 characters.

2) It is extremely slow. In my 1.5 million cell test sheet, code execution took 394 seconds.

I'm looking for ideas to optimize the code's speed, and to make the code work on all display value lengths.

Function ConvertAllCellsToText()

    ' Convert all cells from non-text to text
    ' This is helpful in a variety of ways
        ' Dates and datetimes can now be manipulated by replace, RegEx, etc., since their value is now 4/7/2001 instead of 36988
        ' Phone numbers and zip codes won't get leading zeros deleted if we try to add those

    StartNewTask "Converting all cells to text"

    ' first draft of the code, does not work as intended, dates get scrambled
    'Cells.Select
    'Selection.Copy
    'Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlNone, SkipBlanks _
        :=False, Transpose:=False
    'Application.CutCopyMode = False

    Dim MyRange As Range
    Set MyRange = ActiveSheet.UsedRange

    ' prevent Cell.Text from being ########
    MyRange.ColumnWidth = 255

    Dim i As Long
    Dim CellText As String
    For Each Cell In MyRange
        If Cell.Text <> Cell.Value And Len(Cell.Value) <= 255 Then
            CellText = Cell.Text
            Cell.NumberFormat = "@"
            Cell.Value = CellText
        End If

        i = i + 1
        If i Mod 2500 = 0 Then
            RefreshScreen
        End If
    Next

    MyRange.ColumnWidth = 20

    ' set all cells to Text format
    Cells.Select
    Selection.NumberFormat = "@"

End Function

This code has basically the same performance issue as your last question, and the performance solution is pretty much the same.  Other things to note:

This is more of a nag than anything, but you should really remove
dead code that is commented out.
The variable Cell is never declared. You should put Option Explicit at the top of all of your code modules. See Why should I
dimension my variables in VBA
really? over on SO.
You don't return a value, so this should really be a Sub, not a
Function.  If you don't want it to appear in the macro list,
declare it as Private. I'd get into the habit of always
explicitly declaring your scope.
You have a bug.  You explain pretty clearly in your question that
the .Value of a cell can differ dramatically from the .Text of a
cell depending on the formatting. However, when you test the length
of the cell values, you're checking against the Value that you
already point out is not dependant on the formatting.  You need to
test the Text against your width.
You have another possible bug (not addressed in the code below) -
for proportional width fonts, the value referred to by ColumnWidth
is based on the width of the character '0' (see the
documentation).
If you happen to be using a font where any character is wider than
'0' (for example 'X' in my browser's default font), you can't reliably use the ColumnWidth as a proxy for the
length of a string that it can display.
255 is the magic number, yes it is. Since you're testing the maximum
width of a text rendered cell to this value, and using it in
multiple places in your code to provide formatting information, make
it a constant and give it an appropriate name.
I'm assuming that the call to RefreshScreen is to provide some
sort of indication to the user that something is actually happening
instead of a hung script.  I'd remove it completely.  If you call
DoEvents Excel will have a chance to update the screen buffer.
This is not only more performant (it avoids a function call), but it
will also be clearer to the reader what is going on. Even better
would be to simply omit it, as redrawing the screen is what is
killing your in performance in the first place (see below).
Personally, I would cache the column widths and reset them when you
are done instead of using the arbitrary value '20'.  This upholds
the principle of least astonishment.
You set the NumberFormat to text for the entire Worksheet at the
end of the Function.  Setting it for individual cells inside your
loop is not only redundant, but forces a ScreenUpdate. This is
killing your performance. Basically, what is happening is that you are updating the display once for every cell in the UsedRange.
This is incredibly expensive.  Turn ScreenUpdating off instead.
As I mentioned in comments to your previous question, updating cells
is more expensive in terms of performance than you might guess.
When you are working with large ranges of cells, you should pull
them into an array if at all possible, then write the entire array
back at once. This lowers your "Excel overhead" almost
quadratically.
You should avoid using the Cells collection (it is entirely
dependant on what object is active) and the Selection object
(almost every operation can be performed with the Range you
already have a reference to).
Use With blocks to your advantage, especially in loops.  Every
 '.' operator that you see in your code causes an object deference
 call.  Wrapping sections where the same object is used repeatedly
 in a With block essentially caches the result of the deference
 instead of repeatedly performing it.
Similar note with pulling cell values from a Range.  Accessing
 the object is much more expensive than storing it.  If you have to
 reference the same value twice (looking at you, If Cell.Text <> Cell.Value And Len(Cell.Value) <= 255 Then), cache it.

The above notes basically boil down to something more like this:

Const MaxTextLength = 255

Private Sub ConvertAllCellsToText()
    StartNewTask "Converting all cells to text"
    Application.ScreenUpdating = False

    Dim MyRange As Range
    Set MyRange = ActiveSheet.UsedRange
    Dim cache() As Long
    cache = GetColumnWidths(MyRange)

    With MyRange
        .ColumnWidth = MaxTextLength
        Dim Values() As Variant
        ReDim Values(.Rows.Count, .Columns.Count)

        Dim col As Long
        Dim row As Long

        For row = 0 To UBound(Values, 1)
            For col = 0 To UBound(Values, 2)
                Dim temp As String
                temp = .Cells(row + 1, col + 1).Text
                If Len(temp) <= MaxTextLength Then
                    Values(row, col) = temp
                End If
            Next col
        Next row
        .NumberFormat = "@"
    End With

    MyRange = Values
    SetColumnWidths MyRange, cache
    Application.ScreenUpdating = True
End Sub

Private Function GetColumnWidths(Target As Range) As Long()
    Dim output() As Long
    ReDim output(1 To Target.Columns.Count)
    Dim index As Long
    For index = 1 To Target.Columns.Count
        output(index) = Target.Columns(index).ColumnWidth
    Next index
    GetColumnWidths = output
End Function

Private Sub SetColumnWidths(Target As Range, widths() As Long)
    Dim index As Long
    For index = LBound(widths) To UBound(widths)
        Target.Columns(index).ColumnWidth = widths(index)
    Next index
End Sub

NOTE: I didn't benchmark this, but I'd ballpark about 100 times faster.

EDIT: Almost forgot.  If you have a bunch of numbers in your sheet, you'll want to ignore the numbers stored as text errors to get another nominal performance gain.  Call it before you turn ScreenUpdating back on.


"Longest Palindromic Substring" challenge solution is inefficient I am trying to solve the Longest Palindromic Substring problem on LeetCode, and I have come up with a DP solution pasted below:

public class Solution {
    String      string;
    boolean[][] visited;
    String[][]  results;

    private boolean isPalindrome(String s){
        String test = new StringBuilder(s).reverse().toString();
        if(test.equals(s)) return true; else return false;
    }

    private String maxLengthString(String... args){
        String result = new String();
        for(String arg:args){
            if(arg.length() > result.length()) result = arg;
        }
        return result;
    }

    private String findLongestPalindrome(int start,int end){
        if(start >= end) return new String();
        else if(start >= string.length() || start < 0) return new String();
        else if(end <= 0 || end > string.length()) return new String();
        else{
            if(visited[start][end-1] == true) return results[start][end-1];
            String result = new String();
            if(this.isPalindrome(string.substring(start,end)))
                result = maxLengthString(string.substring(start,end),
                                        findLongestPalindrome(start+1,end),
                                        findLongestPalindrome(start,end-1));
            else result = maxLengthString(findLongestPalindrome(start+1,end),
                                          findLongestPalindrome(start,end-1));
            visited[start][end-1] = true;
            results[start][end-1] = result;
            return result;
        }
    }

    public String longestPalindrome(String s) {
        // Start typing your Java solution below
        // DO NOT write main() function

        string = s;
        visited = new boolean[s.length()][s.length()];
        results = new String[s.length()][s.length()];

        return findLongestPalindrome(0,s.length());
    }
}

The algorithm is quite straightforward, but the online judge complains about it not being efficient enough. I am just wondering if this algorithm is slow by nature (in which case I have to find another algorithm) or if it's something about my implementation.

Why is it slow?

The input string S can be up to 1000 characters.

Your DP solution considers all \$\mathcal{O}(|S|^2)\$ substrings of S and you're checking for each of these substrings, if it is a palindrome, in linear time in terms of the length of this substring.

Faster solutions

You can do it easily in \$\mathcal{O}(|S|^2)\$ using a polynomial hashing technique.

There are of course more sophisticated and linear solutions based on the famous Manacher algorithm.


Recursive Implementation of Merge Sort C++ I am currently reading an algorithms book and I read on the insertion and merge sorts. I implemented the merge sort in C++, The first function MERGE is responsible for the merging of the two subarrays part and the function MERGE_SORT is the actual algorithm. I am interested to know whether or not my code has a good readability factor and if It can be optimized further or not?

#include <iostream>
#include <vector>
#include <limits>

using namespace std;

#define itr vector<int>::iterator
void MERGE(vector<int>& vec,itr left, itr mid, itr right){
    //Make vector left with elements from left to mid inclusive
    vector<int> left_vec;
    for (itr i=left; i!=vec.end()&&i<=mid;i++){
        left_vec.push_back(*i);
    }
    left_vec.push_back(numeric_limits<int>::max());//sentinel card
    //make vector right with elements from mid+1 to right inclusive
    vector<int> right_vec;
    for (itr i=mid+1; i!=vec.end() && i<=right; i++){
        right_vec.push_back(*i);
    }
    right_vec.push_back(numeric_limits<int>::max());//sentinel card
    //Now add them in a sorted manner to vector from left to right

    itr l=left_vec.begin(); itr r=right_vec.begin();
    for (itr vec_itr=left;vec_itr!=vec.end()&&vec_itr<=right;vec_itr++){
        if (*l<=*r){
            *vec_itr=*l;
            l++;
        }
        else{
            *vec_itr = *r;
            r++;
        }
    }
}
void MERGE_SORT(vector<int>& vec, itr left, itr right){
    if(left<right){
        itr mid = left + (right-left)/2;
        MERGE_SORT(vec,left,mid);
        MERGE_SORT(vec,mid+1,right);
        MERGE(vec, left,mid,right);
    }
    return;
}
int main() {
    int a[] = {10,8,9,7,6,3,3,9,15,4,3,2,1};
    vector<int> vec(a,a+13);
    MERGE_SORT(vec,vec.begin(),vec.end());
    for (itr i=vec.begin();i!=vec.end();i++)cout << *i << " ";
    return 0;
}

First off, I'd use typedef rather than #define.

You don't really need to reference vec.end(); you know mid <= right <= vec.end() and the fact that you're copying the same number of elements back in as you copied out, means you don't need to compare vec_itr to vec.end(). What this really means is that you don't need the vec parameter to MERGE.

vector<T> has a constructor that copies from an iterator. Your push_back for loop could be simplified to

vector<int> left_vec(left, mid);
vector<int> right_vec(mid, right);

The push_back of max() as a sentinel is not strictly needed. Instead, you can just loop until you hit the end of either left_vec or right_vec and then copy the remainder of the vectors to the left to right range.

Your variable names might be easier to understand.

typedef itr vector<int>::iterator;

void MERGE(itr left, itr mid, itr right){
    //Make vector left with elements from left to mid-1 inclusive
    vector<int> left(left, mid);

    //make vector right with elements from mid to right-1 inclusive
    vector<int> right(mid, right);

    // Now concurrently loop over the left and right vectors
    // and add the values back to left to right. Stop when
    // either iterator runs out of elements.
    itr left_src=left.begin();
    itr right_src=right.begin();
    itr dest = left;
    while (left_src != left.end() && right_src != right.end()) {
        // pick the iterator with the smallest current value
        itr& least_src = (*left_src < *right_src) ? left_src : right_src;
        *dest = *least_src;
        least_src ++;
        dest ++;
    }

    // copy remainder of left (maybe 0 elements)
    copy(left_src, left.end(), dest);

    // copy remainder of right (maybe 0 elements)
    copy(right_src, right.end(), dest);
}


Lock-free queue with doubly linked list correctness I need a lock-free queue which is implemented by doubly linked list.

Is my code correct?  Are there any errors?  Are there any improvements to be made?

linkedlist.h

/*
 * linkedlist.h
 *
 *  Created on: 2014. 5. 10.
 *      Author: dlaru_000
 */

#ifndef LINKEDLIST_H_
#define LINKEDLIST_H_

/*
 * circular doubly linked list
 * PushBack / PopFront: single-producer, single-customer lock-free queue
 * Erase: thread-unsafe
 */

#include <stddef.h>
#include <stdint.h>

typedef struct tagLinkedListNode
{
    struct tagLinkedListNode * volatile pNext;
    struct tagLinkedListNode * volatile pPrev;
} LinkedListNode;

typedef struct tagLinkedList
{
    LinkedListNode dummy;

    /*
     * It can be invalid while some function is running on the other thread.
     * However it must valid after/before the function is finished.
     */
    uint32_t size;
} LinkedList;

void ckLinkedListInit(LinkedList *pList);

static inline LinkedListNode *ckLinkedListHead(LinkedList *pList) { return pList->dummy.pNext; }
static inline LinkedListNode *ckLinkedListTail(LinkedList *pList) { return pList->dummy.pPrev; }

void ckLinkedListPushBack(LinkedList *pList, void *pNode);
LinkedListNode *ckLinkedListPopFront(LinkedList *pList);

void ckLinkedListErase(LinkedList *pList, void *pNode);

#endif /* LINKEDLIST_H_ */

linkedlist.c

/*
 * linkedlist.c
 *
 *  Created on: 2014. 5. 10.
 *      Author: dlaru_000
 */

#include "linkedlist.h"

void ckLinkedListInit(LinkedList *pList)
{
    // circular linked list
    pList->dummy.pNext = &pList->dummy;
    pList->dummy.pPrev = &pList->dummy;
    pList->size = 0;
}

void ckLinkedListPushBack(LinkedList *pList, void *pNode)
{
    __sync_add_and_fetch(&pList->size, 1);

    LinkedListNode *dummy = &pList->dummy;

    LinkedListNode *ptr = (LinkedListNode *)pNode;
    LinkedListNode *tail;

    ptr->pNext = dummy;

    while (1)
    {
        tail = dummy->pPrev;
        ptr->pPrev = tail;
        if (__sync_bool_compare_and_swap(&dummy->pPrev, tail, ptr))
        {
            tail->pNext = ptr;
            break;
        }
    }
}

LinkedListNode *ckLinkedListPopFront(LinkedList *pList)
{
    LinkedListNode *dummy = &pList->dummy;

    LinkedListNode *ptr, *next, *dnext;

    ptr = dummy->pNext;
    if (ptr == dummy)
    {
        return NULL;
    }

    while (1)
    {
        next = ptr->pNext;
        if (__sync_bool_compare_and_swap(&next->pPrev, ptr, dummy))
        {
            dnext = dummy->pNext;
            __sync_bool_compare_and_swap(&dummy->pNext, dnext, next);
            break;
        }
    }

    __sync_sub_and_fetch(&pList->size, 1);
    return ptr;
}

void ckLinkedListErase(LinkedList *pList, void *pNode)
{
    LinkedListNode *ptr = (LinkedListNode *)pNode;
    LinkedListNode *prev = ptr->pPrev;
    LinkedListNode *next = ptr->pNext;

    prev->pNext = next;
    next->pPrev = prev;
    pList->size--;
}

The type of the second parameter of PushBack is void *, because the list actually contains such a thing like the following:

struct SomeData
{
    LinkedListNode _node;
    ...
};

I'm using GCC 4.8.2 with cygwin.

Um, I've noticed that "__sync" functions, which the program is using, are legacy, and there are new functions __atomic. How can I migrate to __atomic?

C11 atomic library

I you want to avoid the "legacy" functions, one solution would be to use the C11 atomic operations library. It is not widely supported yet. I know that GCC has been working on it, but I don't know if the 4.9 release already implements all the operations. Anyway, had you a compliant compiler, you could use standard atomic types and atomic operations.

For example, here are your functions ckLinkedListPushBack and ckLinkedListPopFront rewritten with the C11 atomic module:

void ckLinkedListPushBack(LinkedList *pList, void *pNode)
{
    atomic_fetch_add(&pList->size, 1);

    LinkedListNode *dummy = &pList->dummy;

    LinkedListNode *ptr = (LinkedListNode *)pNode;
    LinkedListNode *tail;

    ptr->pNext = dummy;

    while (1)
    {
        tail = dummy->pPrev;
        ptr->pPrev = tail;
        if (atomic_compare_exchange_strong(&dummy->pPrev, tail, ptr))
        {
            tail->pNext = ptr;
            break;
        }
    }
}

LinkedListNode *ckLinkedListPopFront(LinkedList *pList)
{
    LinkedListNode *dummy = &pList->dummy;

    LinkedListNode *ptr, *next, *dnext;

    ptr = dummy->pNext;
    if (ptr == dummy)
    {
        return NULL;
    }

    while (1)
    {
        next = ptr->pNext;
        if (atomic_compare_exchange_strong(&next->pPrev, ptr, dummy))
        {
            dnext = dummy->pNext;
            atomic_compare_exchange_strong(&dummy->pNext, dnext, next);
            break;
        }
    }

    atomic_fetch_sub(&pList->size, 1);
    return ptr;
}

In order for it to work, you would have to declare size to be an instance of atomic_uint_least32_t (fixed size types are not provided for atomics).

typedef struct

You can make clearer the LinkedListNode declaration by separating the typedef from the actual declaration:

typedef struct tagLinkedListNode LinkedListNode;

struct tagLinkedListNode
{
    LinkedListNode * volatile pNext;
    LinkedListNode * volatile pPrev;
};